Bug 577400 - fix RefreshScopeComparator Comparator contract RefreshScopeComparator and SourceLocatorMementoComparator where not null safe and did not yield compare(x,y)=-compare(y,x) as it never did return 1, but -1. Change-Id: I6af76fc4abe94fc461f56cdbb05a09431cddd382 Signed-off-by: Joerg Kubitz <jkubitz-eclipse@gmx.de> Reviewed-on: https://git.eclipse.org/r/c/platform/eclipse.platform.debug/+/187986 Tested-by: Platform Bot <platform-bot@eclipse.org> Reviewed-by: Sarika Sinha <sarika.sinha@in.ibm.com>
diff --git a/org.eclipse.debug.core/core/org/eclipse/debug/internal/core/RefreshScopeComparator.java b/org.eclipse.debug.core/core/org/eclipse/debug/internal/core/RefreshScopeComparator.java index a875896..3a88dc3 100644 --- a/org.eclipse.debug.core/core/org/eclipse/debug/internal/core/RefreshScopeComparator.java +++ b/org.eclipse.debug.core/core/org/eclipse/debug/internal/core/RefreshScopeComparator.java
@@ -13,6 +13,7 @@ *******************************************************************************/ package org.eclipse.debug.internal.core; +import java.util.Arrays; import java.util.Comparator; import org.eclipse.core.resources.IResource; @@ -27,26 +28,21 @@ * @since 3.6 */ public class RefreshScopeComparator implements Comparator<String> { + private static IResource[] toResources(String memento) { + try { + return RefreshUtil.toResources(memento); + } catch (CoreException e) { + return null; + } + } + + private static final Comparator<IResource> RESOURCE = Comparator.nullsFirst(Comparator.comparing(r -> r.toString())); + private static final Comparator<IResource[]> ARRAY = Comparator.nullsFirst((IResource[] s1, IResource[] s2) -> Arrays.compare(s1, s2, RESOURCE)); + private static final Comparator<String> MEMENTO = Comparator.nullsFirst(Comparator.comparing(m -> toResources(m), ARRAY)); @Override public int compare(String o1, String o2) { - String m1 = o1; - String m2 = o2; - try { - IResource[] r1 = RefreshUtil.toResources(m1); - IResource[] r2 = RefreshUtil.toResources(m2); - if (r1.length == r2.length) { - for (int i = 0; i < r2.length; i++) { - if (!r1[i].equals(r2[i])) { - return -1; - } - } - return 0; - } - } catch (CoreException e) { - return -1; - } - return -1; + return MEMENTO.compare(o1, o2); } }
diff --git a/org.eclipse.debug.core/core/org/eclipse/debug/internal/core/sourcelookup/SourceLocatorMementoComparator.java b/org.eclipse.debug.core/core/org/eclipse/debug/internal/core/sourcelookup/SourceLocatorMementoComparator.java index 1be7686..5ec80ab 100644 --- a/org.eclipse.debug.core/core/org/eclipse/debug/internal/core/sourcelookup/SourceLocatorMementoComparator.java +++ b/org.eclipse.debug.core/core/org/eclipse/debug/internal/core/sourcelookup/SourceLocatorMementoComparator.java
@@ -24,6 +24,15 @@ @Override public int compare(String o1, String o2) { + if (o1 == null && o2 == null) { + return 0; + } + if (o1 == null) { + return -1; + } + if (o2 == null) { + return 1; + } String m1 = o1; String m2 = o2; int i1 = 0, i2 = 0; @@ -32,7 +41,7 @@ i2 = skipWhitespace(m2, i2); if (i1 < m1.length() && i2 < m2.length()) { if (m1.charAt(i1) != m2.charAt(i2)) { - return -1; + return m1.charAt(i1) - m2.charAt(i2); } i1++; i2++; @@ -40,6 +49,9 @@ if (i2 < m2.length()) { return -1; } + if (i1 < m1.length()) { + return 1; + } return 0; } }
diff --git a/org.eclipse.debug.tests/src/org/eclipse/debug/tests/launching/RefreshTabTests.java b/org.eclipse.debug.tests/src/org/eclipse/debug/tests/launching/RefreshTabTests.java index 0c2c142..669bd0a 100644 --- a/org.eclipse.debug.tests/src/org/eclipse/debug/tests/launching/RefreshTabTests.java +++ b/org.eclipse.debug.tests/src/org/eclipse/debug/tests/launching/RefreshTabTests.java
@@ -27,6 +27,7 @@ import org.eclipse.core.runtime.CoreException; import org.eclipse.debug.core.RefreshUtil; import org.eclipse.debug.internal.core.RefreshScopeComparator; +import org.eclipse.debug.internal.core.sourcelookup.SourceLocatorMementoComparator; import org.eclipse.debug.tests.TestsPlugin; import org.eclipse.debug.ui.RefreshTab; import org.eclipse.jface.viewers.ISelectionProvider; @@ -180,11 +181,30 @@ * * @throws CoreException */ + @SuppressWarnings("restriction") @Test public void testRefreshScopeComparator() throws CoreException { String oldStyle = "${working_set:<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<launchConfigurationWorkingSet factoryID=\"org.eclipse.ui.internal.WorkingSetFactory\" name=\"workingSet\" editPageId=\"org.eclipse.ui.resourceWorkingSetPage\">\n<item factoryID=\"org.eclipse.ui.internal.model.ResourceFactory\" path=\"/RefreshTabTests/some.file\" type=\"1\"/>\n</launchConfigurationWorkingSet>}"; //$NON-NLS-1$ String newStyle = "${working_set:<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<resources>\n<item path=\"/RefreshTabTests/some.file\" type=\"1\"/>\n</resources>}"; //$NON-NLS-1$ assertEquals("Comparator should return 0", 0, new RefreshScopeComparator().compare(oldStyle, newStyle)); //$NON-NLS-1$ + String s1 = "${working_set:<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<resources>\n<item path=\"/RefreshTabTests/some.file1\" type=\"1\"/>\n</resources>}"; //$NON-NLS-1$ + String s2 = "${working_set:<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<resources>\n<item path=\"/RefreshTabTests/some.file2\" type=\"1\"/>\n</resources>}"; //$NON-NLS-1$ + assertEquals("Comparator should return 0", 0, new RefreshScopeComparator().compare(s1, s1)); //$NON-NLS-1$ + assertEquals("Comparator should return 0", 0, new RefreshScopeComparator().compare(s2, s2)); //$NON-NLS-1$ + assertEquals("Comparator should return -1", -1, new RefreshScopeComparator().compare(s1, s2)); //$NON-NLS-1$ + assertEquals("Comparator should return 1", 1, new RefreshScopeComparator().compare(s2, s1)); //$NON-NLS-1$ + assertEquals("Comparator should return 1", 1, new RefreshScopeComparator().compare(s1, null)); //$NON-NLS-1$ + assertEquals("Comparator should return -1", -1, new RefreshScopeComparator().compare(null, s1)); //$NON-NLS-1$ + assertEquals("Comparator should return 0", 0, new RefreshScopeComparator().compare(null, null)); //$NON-NLS-1$ + String o1 = "${working_set:<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<launchConfigurationWorkingSet factoryID=\"org.eclipse.ui.internal.WorkingSetFactory\" name=\"workingSet\" editPageId=\"org.eclipse.ui.resourceWorkingSetPage\">\n<item factoryID=\"org.eclipse.ui.internal.model.ResourceFactory\" path=\"/RefreshTabTests/some.file1\" type=\"1\"/>\n</launchConfigurationWorkingSet>}"; //$NON-NLS-1$ + String o2 = "${working_set:<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<launchConfigurationWorkingSet factoryID=\"org.eclipse.ui.internal.WorkingSetFactory\" name=\"workingSet\" editPageId=\"org.eclipse.ui.resourceWorkingSetPage\">\n<item factoryID=\"org.eclipse.ui.internal.model.ResourceFactory\" path=\"/RefreshTabTests/some.file2\" type=\"1\"/>\n</launchConfigurationWorkingSet>}"; //$NON-NLS-1$ + assertEquals("Comparator should return 0", 0, new SourceLocatorMementoComparator().compare(o1, o1)); //$NON-NLS-1$ + assertEquals("Comparator should return 0", 0, new SourceLocatorMementoComparator().compare(o2, o2)); //$NON-NLS-1$ + assertEquals("Comparator should return -1", -1, new SourceLocatorMementoComparator().compare(o1, o2)); //$NON-NLS-1$ + assertEquals("Comparator should return 1", 1, new SourceLocatorMementoComparator().compare(o2, o1)); //$NON-NLS-1$ + assertEquals("Comparator should return 1", 1, new SourceLocatorMementoComparator().compare(o1, null)); //$NON-NLS-1$ + assertEquals("Comparator should return -1", -1, new SourceLocatorMementoComparator().compare(null, o1)); //$NON-NLS-1$ + assertEquals("Comparator should return 0", 0, new SourceLocatorMementoComparator().compare(null, null)); //$NON-NLS-1$ } /** @@ -197,6 +217,7 @@ IResource[] resources = new IResource[] { getProject(), getProject().getFile("not.exist"), getProject().getFile("some.file") }; //$NON-NLS-1$ //$NON-NLS-2$ String memento = RefreshUtil.toMemento(resources); IResource[] restore = RefreshUtil.toResources(memento); + assertNotNull(resources); assertEquals(resources.length, restore.length); assertEquals(resources[0], restore[0]); assertEquals(resources[1], restore[1]);