Bug 501752 - Change the default direction of the compare editor by default
Summary: Change the default direction of the compare editor by default
Status: NEW
Alias: None
Product: Platform
Classification: Eclipse Project
Component: Compare (show other bugs)
Version: 4.7   Edit
Hardware: All All
: P3 enhancement with 3 votes (vote)
Target Milestone: ---   Edit
Assignee: Lars Vogel CLA
QA Contact:
URL:
Whiteboard:
Keywords: noteworthy
: 549668 (view as bug list)
Depends on: 213780
Blocks:
  Show dependency tree
 
Reported: 2016-09-19 14:02 EDT by Marc-André Laperle CLA
Modified: 2019-10-10 09:37 EDT (History)
16 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Marc-André Laperle CLA 2016-09-19 14:02:08 EDT
Since bug 213780 was resolved, it is now possible to change the "Compare With" direction with a button and a preference. Since a lot of other diff viewing software (all?) use the other direction than Eclipse, I think it would be worth considering changing the default. A lot of people expect to see the order where the "older" document is on the left and thew "newer" document is on the right.

Once the default is changed and most people are OK with it, perhaps the button in the toolbar would not be necessary and only the preference in the dialog would be sufficient as this is a one time change that only a minority of people would have to do.
Comment 1 Lars Vogel CLA 2016-09-19 14:06:14 EDT
+1 for switching the default
Comment 2 Sergey Prigogin CLA 2016-09-19 14:07:36 EDT
+1

In our company we deployed Eclipse with the default preference flipped and users seem happy with it.
Comment 3 Andrey Loskutov CLA 2016-09-19 14:08:23 EDT
+1
Comment 4 Dennis Hendriks CLA 2016-09-19 14:17:14 EDT
+1
Comment 5 Alvaro Sanchez-Leon CLA 2016-09-19 14:20:45 EDT
+1 for Switching defaults and keep the switch in the preferences
Comment 6 Andrey Loskutov CLA 2016-09-19 14:48:10 EDT
(In reply to Alvaro Sanchez-Leon from comment #5)
> +1 for Switching defaults and keep the switch in the preferences

Yep, the preferences switch is a must. There will be always people used Eclipse for 10+ years and never used any tool where the direction is "wrong" (if coming from this point of view). Product owners will also need to make sure their products go fine with the change.

Marc-Andre or Sergey: do you plan to provide a patch? Just in case we don't get a -1 from Dani :-)

We should do this (if no one disagrees) as early as possible so that we can catch as much feedback as possible during 4.7 release cycle.
Comment 7 Sergey Prigogin CLA 2016-09-19 15:15:48 EDT
(In reply to Andrey Loskutov from comment #6)
> Marc-Andre or Sergey: do you plan to provide a patch? Just in case we don't
> get a -1 from Dani :-)

I intend to write a patch if nobody vetoes this change.
Comment 8 Marc Khouzam CLA 2016-09-19 15:17:37 EDT
+1

I'm still curious to know why eclipse shows the new version on the left.  All other diff tools I know show it on the right. Anyone?
Comment 9 Sergey Prigogin CLA 2016-09-19 15:20:26 EDT
(In reply to Marc Khouzam from comment #8)

Let's focus on the bright future, not the ugly past :-).
Comment 10 Stephan Herrmann CLA 2016-09-19 16:16:44 EDT
Do you have an idea, how to signal to users, that sides have swapped?

I'd argue that *every* existing Eclipse user will be very surprised and make mistakes at first, UNLESS clearly informed about the change.

(I had a number of such accidents 2 months back - not happy).
Comment 11 Alex Vassiliev CLA 2016-09-19 18:40:33 EDT
+1
Comment 12 Andrey Loskutov CLA 2016-09-20 09:05:45 EDT
(In reply to Stephan Herrmann from comment #10)
> Do you have an idea, how to signal to users, that sides have swapped?
> 
> I'd argue that *every* existing Eclipse user will be very surprised and make
> mistakes at first, UNLESS clearly informed about the change.

Very good point. I'm not aware if there is any possibility to show a warning header in the diff editor (with a link to disable it), but this would be one possible solution. Another one (definitely less effort) would be to show a popup first time the compare editor is shown in the current installation, for 4.7 release only, with some explanation that diff order is different now.
Comment 13 Steffen Brüntjen CLA 2016-09-26 04:45:24 EDT
+1
Comment 14 Dani Megert CLA 2016-09-28 11:31:40 EDT
-1 for simply changing the default, but +1 for a solution where the user either can choose upfront or has a very easy way to switch back to the long existing default.
Comment 15 Dani Megert CLA 2016-09-29 07:44:31 EDT
(In reply to Dani Megert from comment #14)
> has a very easy way to switch back to the long existing default.

That's already there with the toolbar button, but what's needed is to make the user aware, that the default has changed (see comment 10).
Comment 16 Sergey Prigogin CLA 2016-09-30 16:34:54 EDT
(In reply to Dani Megert from comment #15)
> (In reply to Dani Megert from comment #14)
> > has a very easy way to switch back to the long existing default.
> 
> That's already there with the toolbar button, but what's needed is to make
> the user aware, that the default has changed (see comment 10).

The best solution in my view would be an interstitial popup shown once in the Compare editor. Does anybody know of existing interstitial popups in Eclipse? If not, I'll file a bug for creating them.
Comment 17 Dani Megert CLA 2016-10-02 10:33:20 EDT
(In reply to Sergey Prigogin from comment #16)
> (In reply to Dani Megert from comment #15)
> > (In reply to Dani Megert from comment #14)
> > > has a very easy way to switch back to the long existing default.
> > 
> > That's already there with the toolbar button, but what's needed is to make
> > the user aware, that the default has changed (see comment 10).
> 
> The best solution in my view would be an interstitial popup shown once in
> the Compare editor. Does anybody know of existing interstitial popups in
> Eclipse? If not, I'll file a bug for creating them.

The Compare editor has some code that allows to show a warning (e.g. to switch to another compare algorithm). Maybe that can be (re-) used.
Comment 18 Andrey Loskutov CLA 2017-02-19 17:39:39 EST
(In reply to Dani Megert from comment #17)
> (In reply to Sergey Prigogin from comment #16)
> > (In reply to Dani Megert from comment #15)
> > > (In reply to Dani Megert from comment #14)
> > > > has a very easy way to switch back to the long existing default.
> > > 
> > > That's already there with the toolbar button, but what's needed is to make
> > > the user aware, that the default has changed (see comment 10).
> > 
> > The best solution in my view would be an interstitial popup shown once in
> > the Compare editor. Does anybody know of existing interstitial popups in
> > Eclipse? If not, I'll file a bug for creating them.
> 
> The Compare editor has some code that allows to show a warning (e.g. to
> switch to another compare algorithm). Maybe that can be (re-) used.

Thanks for the hint, I've just stumbled over this again: this is unfortunately hardcoded via labelOptimized and recomputeLink in org.eclipse.compare.internal.CompareContentViewerSwitchingPane.

If someone wants to add something like this extra buttons here, one can either try to generalize them (which would mean we will still have only one info and we will need a decision which from many infos we want show) or change the layout so that we can have multiple "info + action" lines in the header, or provide something like "Multiple warnings: please hover over to see all" label.
Comment 19 Andrey Loskutov CLA 2019-07-30 14:15:07 EDT
*** Bug 549668 has been marked as a duplicate of this bug. ***
Comment 20 Pierre-Yves Bigourdan CLA 2019-07-30 14:40:33 EDT
If it's a temporary warning in place for 4.13 but removed in 4.14, why not, but it's not ideal: if users migrate from 4.12 (or older) to 4.14 (or newer) without going through 4.13, they wouldn't be covered. Making the warning permanent is probably even worse in my opinion: it's the default behaviour in pretty much any other tool, warning about something that's natural/expected is confusing.

As I'm guessing any warning/popup would require more work (which explains why this has been stalled for several years now?), would the pragmatic option be to just go ahead and change the default? True, in the short-term some users may be surprised at first, but the benefits in the mid/longer term outweigh this in my opinion. Clearly communicating about it (N&N, Holder's new release videos, Tweeting, etc.) would go a long way about informing the users beforehand and minimising the impact.
Comment 21 Sergey Prigogin CLA 2019-07-30 17:35:12 EDT
(In reply to Pierre-Yves B. from comment #20)

+100

I hope, Dani is reading this.
Comment 22 Pierre-Yves Bigourdan CLA 2019-08-13 04:19:25 EDT
(In reply to Sergey Prigogin from comment #21)
> (In reply to Pierre-Yves B. from comment #20)
> 
> +100
> 
> I hope, Dani is reading this.

Maybe the next step is to prepare a patch to make progress on this and keep the discussion going?
Comment 23 Lars Vogel CLA 2019-08-13 04:43:52 EDT
(In reply to Pierre-Yves B. from comment #22)
> Maybe the next step is to prepare a patch to make progress on this and keep
> the discussion going?

Yes, please provide Gerrit. Please let me know if you need help with Gerrit.
Comment 24 Pierre-Yves Bigourdan CLA 2019-08-13 05:06:17 EDT
Sergey, you wanted to provide a patch of I'm not mistaken? I won't have access to a proper dev environment in the next couple of weeks.
Comment 25 Lars Vogel CLA 2019-08-27 11:22:10 EDT
The PMC agree that we try out the switch in 4.14 M1. See https://wiki.eclipse.org/Eclipse/PMC
Comment 26 Eclipse Genie CLA 2019-08-27 11:32:31 EDT
New Gerrit change created: https://git.eclipse.org/r/148467
Comment 27 Sravan Kumar Lakkimsetti CLA 2019-09-19 06:41:33 EDT
(In reply to Eclipse Genie from comment #26)
> New Gerrit change created: https://git.eclipse.org/r/148467

When I ran locally I got these failures. Also one of the test thrown a popup and it is waiting for user input. I think that is the reason for timeout

testModifyRight(org.eclipse.compare.tests.TextMergeViewerTest)  Time elapsed: 0.528 s
testCopyEmptyLeftToRightAndModify(org.eclipse.compare.tests.TextMergeViewerTest)  Time elapsed: 0.47 s  <<< ERROR!
java.lang.NullPointerException
	at org.eclipse.compare.tests.TextMergeViewerTest.testCopyEmptyLeftToRightAndModify(TextMergeViewerTest.java:352)

testCopyRightToEmptyLeft(org.eclipse.compare.tests.TextMergeViewerTest)  Time elapsed: 0.528 s
testCopyRightToLeft(org.eclipse.compare.tests.TextMergeViewerTest)  Time elapsed: 0.436 s  <<< FAILURE!
org.junit.ComparisonFailure: expected:<[hi there]> but was:<[some text]>
	at org.eclipse.compare.tests.TextMergeViewerTest.testCopyRightToLeft(TextMergeViewerTest.java:242)

testCopyEmptyLeftToRight(org.eclipse.compare.tests.TextMergeViewerTest)  Time elapsed: 0.407 s  <<< FAILURE!
java.lang.AssertionError: expected null, but was:<org.eclipse.compare.tests.TextMergeViewerTest$EditableTestElement@3bc20984>
	at org.eclipse.compare.tests.TextMergeViewerTest.testCopyEmptyLeftToRight(TextMergeViewerTest.java:287)

testCompareFilter(org.eclipse.compare.tests.TextMergeViewerTest)  Time elapsed: 20.335 s
testDocumentAsTypedElement(org.eclipse.compare.tests.TextMergeViewerTest)  Time elapsed: 0.329 s
testModifyLeft(org.eclipse.compare.tests.TextMergeViewerTest)  Time elapsed: 0.197 s
testCopyLeftToEmptyRight(org.eclipse.compare.tests.TextMergeViewerTest)  Time elapsed: 0.42 s
testCopyEmptyRightToLeftAndModify(org.eclipse.compare.tests.TextMergeViewerTest)  Time elapsed: 0.34 s  <<< ERROR!
java.lang.NullPointerException
	at org.eclipse.compare.tests.TextMergeViewerTest.testCopyEmptyRightToLeftAndModify(TextMergeViewerTest.java:338)

testCopyEmptyRightToLeft(org.eclipse.compare.tests.TextMergeViewerTest)  Time elapsed: 0.528 s  <<< FAILURE!
java.lang.AssertionError: expected null, but was:<org.eclipse.compare.tests.TextMergeViewerTest$EditableTestElement@6c96403e>
	at org.eclipse.compare.tests.TextMergeViewerTest.testCopyEmptyRightToLeft(TextMergeViewerTest.java:299)

testCopyLeftToRight(org.eclipse.compare.tests.TextMergeViewerTest)  Time elapsed: 0.451 s  <<< FAILURE!
org.junit.ComparisonFailure: expected:<[hi there]> but was:<[some text]>
	at org.eclipse.compare.tests.TextMergeViewerTest.testCopyLeftToRight(TextMergeViewerTest.java:254)
Comment 28 Lars Vogel CLA 2019-09-19 06:43:25 EDT
Thanks Sravan, I will have a look
Comment 29 Andrey Loskutov CLA 2019-09-30 09:20:23 EDT
(In reply to Lars Vogel from comment #28)
> Thanks Sravan, I will have a look

Lars, do you plan to work on that?
Comment 30 Lars Vogel CLA 2019-09-30 09:26:46 EDT
(In reply to Andrey Loskutov from comment #29)
> Lars, do you plan to work on that?

I'm currently busy with customer work, so if the above means that you want to take over, that is fine. Otherwise, I will pick this up, once I have again time (> 3-4 weeks).
Comment 31 Dani Megert CLA 2019-10-10 09:37:27 EDT
Removed milestone as no one reacted to https://www.eclipse.org/lists/eclipse-dev/msg11217.html.