From 33e1742b3ec4aa6b80a57563e7ad21d2bd591664 Mon Sep 17 00:00:00 2001 From: Philipp SALZGEBER Date: Wed, 21 Oct 2026 17:23:30 +0200 Subject: [PATCH] Sort by the undecorated label of styled decorating label providers ViewerComparator.getLabel unwraps a DecoratingLabelProvider (Bug 364735) but sorts by the decorated text of a DecoratingStyledCellLabelProvider, the label provider of every CommonViewer. Decorations are computed in a background job and may change the label while a sort runs, which violates the comparator contract and lets TimSort throw "Comparison method violates its general contract". Fixes #4516 Signed-off-by: Philipp SALZGEBER --- .../jface/viewers/ViewerComparator.java | 12 +- .../ViewerComparatorDecoratedLabelTest.java | 161 ++++++++++++++++++ 2 files changed, 171 insertions(+), 2 deletions(-) create mode 100755 tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/viewers/ViewerComparatorDecoratedLabelTest.java diff --git a/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/ViewerComparator.java b/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/ViewerComparator.java index 81d739c8f69..939b99fa852 100644 --- a/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/ViewerComparator.java +++ b/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/ViewerComparator.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2006, 2016 IBM Corporation and others. + * Copyright (c) 2006, 2026 IBM Corporation and others. * * This program and the accompanying materials * are made available under the terms of the Eclipse Public License 2.0 @@ -12,6 +12,7 @@ * IBM Corporation - initial API and implementation * Lars Vogel - Bug 430873 * Andrey Loskutov - Bug 364735 + * Philipp Salzgeber (Bachmann electronic GmbH) - Issue #4516 ******************************************************************************/ package org.eclipse.jface.viewers; @@ -146,7 +147,14 @@ private String getLabel(Viewer viewer, Object e1) { } else { IBaseLabelProvider prov = ((ContentViewer) viewer) .getLabelProvider(); - if (prov instanceof ILabelProvider lprov) { + if (prov instanceof DelegatingStyledCellLabelProvider delegating) { + // Issue #4516: like the DecoratingLabelProvider case below, use + // the styled string provider to avoid the decorations of a + // DecoratingStyledCellLabelProvider, which the Common Navigator + // uses. + StyledString text = delegating.getStyledStringProvider().getStyledText(e1); + name1 = text == null ? null : text.getString(); + } else if (prov instanceof ILabelProvider lprov) { if (lprov instanceof DecoratingLabelProvider dprov) { // Bug 364735: use the real label provider to avoid unstable // sort behavior if the decoration is running while sorting. diff --git a/tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/viewers/ViewerComparatorDecoratedLabelTest.java b/tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/viewers/ViewerComparatorDecoratedLabelTest.java new file mode 100755 index 00000000000..3b6e7968523 --- /dev/null +++ b/tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/viewers/ViewerComparatorDecoratedLabelTest.java @@ -0,0 +1,161 @@ +/******************************************************************************* + * Copyright (c) 2026 Bachmann electronic GmbH and others. + * + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + * + * Contributors: + * Philipp Salzgeber (Bachmann electronic GmbH) - initial API and implementation, Issue #4516 + *******************************************************************************/ +package org.eclipse.jface.tests.viewers; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.eclipse.jface.viewers.ContentViewer; +import org.eclipse.jface.viewers.DecoratingLabelProvider; +import org.eclipse.jface.viewers.DecoratingStyledCellLabelProvider; +import org.eclipse.jface.viewers.DelegatingStyledCellLabelProvider.IStyledLabelProvider; +import org.eclipse.jface.viewers.IDecorationContext; +import org.eclipse.jface.viewers.ILabelDecorator; +import org.eclipse.jface.viewers.ILabelProvider; +import org.eclipse.jface.viewers.ILabelProviderListener; +import org.eclipse.jface.viewers.ISelection; +import org.eclipse.jface.viewers.LabelDecorator; +import org.eclipse.jface.viewers.LabelProvider; +import org.eclipse.jface.viewers.StyledString; +import org.eclipse.jface.viewers.ViewerComparator; +import org.eclipse.swt.graphics.Image; +import org.eclipse.swt.widgets.Control; +import org.junit.jupiter.api.Test; + +/** + * Decorations are computed in a background job and may change the decorated + * label while a sort runs, so {@link ViewerComparator} must sort by the + * undecorated label (Bug 364735, Issue #4516). + */ +public class ViewerComparatorDecoratedLabelTest { + + /** + * Decorates like a SCM decorator: the element "b" gets a dirty prefix that + * sorts before letters, "a" stays undecorated. + */ + private static final class PrefixDecorator extends LabelDecorator { + @Override + public String decorateText(String text, Object element) { + return "b".equals(element) ? "> " + text + " [branch]" : text; + } + + @Override + public String decorateText(String text, Object element, IDecorationContext context) { + return decorateText(text, element); + } + + @Override + public Image decorateImage(Image image, Object element) { + return image; + } + + @Override + public Image decorateImage(Image image, Object element, IDecorationContext context) { + return image; + } + + @Override + public boolean prepareDecoration(Object element, String originalText, IDecorationContext context) { + return true; + } + + @Override + public void addListener(ILabelProviderListener listener) { + } + + @Override + public void dispose() { + } + + @Override + public boolean isLabelProperty(Object element, String property) { + return false; + } + + @Override + public void removeListener(ILabelProviderListener listener) { + } + } + + private static final class StyledLabelProvider extends LabelProvider implements IStyledLabelProvider { + @Override + public StyledString getStyledText(Object element) { + return new StyledString(String.valueOf(element)); + } + } + + /** + * Shaped like the Common Navigator's label provider: a styled decorating + * provider that is also an {@link ILabelProvider} returning the decorated + * text. + */ + private static final class NavigatorLikeLabelProvider extends DecoratingStyledCellLabelProvider + implements ILabelProvider { + NavigatorLikeLabelProvider(IStyledLabelProvider labelProvider, ILabelDecorator decorator) { + super(labelProvider, decorator, null); + } + + @Override + public String getText(Object element) { + return getStyledText(element).getString(); + } + + @Override + public Image getImage(Object element) { + return null; + } + } + + /** A content viewer without widgets; only its label provider matters here. */ + private static final class LabelProviderViewer extends ContentViewer { + @Override + public Control getControl() { + return null; + } + + @Override + public ISelection getSelection() { + return null; + } + + @Override + public void refresh() { + } + + @Override + public void setSelection(ISelection selection, boolean reveal) { + } + } + + private final ViewerComparator comparator = new ViewerComparator(); + + @Test + public void styledDecorationsAreIgnoredWhenSorting() { + LabelProviderViewer viewer = new LabelProviderViewer(); + viewer.setLabelProvider(new NavigatorLikeLabelProvider(new StyledLabelProvider(), new PrefixDecorator())); + + assertTrue(comparator.compare(viewer, "b", "a") > 0, + "b must sort after a although its decorated label starts with '>'"); + assertEquals(0, comparator.compare(viewer, "a", "a")); + } + + @Test + public void plainDecorationsAreIgnoredWhenSorting() { + LabelProviderViewer viewer = new LabelProviderViewer(); + viewer.setLabelProvider(new DecoratingLabelProvider(new LabelProvider(), new PrefixDecorator())); + + assertTrue(comparator.compare(viewer, "b", "a") > 0, + "b must sort after a although its decorated label starts with '>'"); + } +}