Community
Participate
Working Groups
The Refactoring Preview dialog shows refactored source of the left when the "Swap sides" preference is enabled. This behavior is undesirable because showing the new code on the left is inconsistent with comparing with local history where the newer code is shown on the right when the the "Swap sides" preference is enabled. The Refactoring Preview dialog does not need to support swapping of sides since it has always been showing the new code on the right, which is a common convention adapted by Gerrit and several other systems. The proposed solution is to allow the Refactoring Preview dialog to suppress the "Swap sides" preference and the associated button.
New Gerrit change created: https://git.eclipse.org/r/85838
New Gerrit change created: https://git.eclipse.org/r/85840
*** Bug 504474 has been marked as a duplicate of this bug. ***
Gerrit change https://git.eclipse.org/r/85838 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.team.git/commit/?id=3faf9a6c45cf2d8f3f7dfca712ef31c70a964826
This now disables the "Swap sides" preference for some comparisons. You should think a feature to the end before starting to add/change APIs. The established policy in Eclipse compare editors was: 1) Left: Local workspace version; Right: Remote version 2) When (1) is not applicable because no local version is being compared: Left: Older version; Right: Newer version The goal of the new preference now apparently is to change policy (1) in some way. APIs that establish a new behavior should be named after the new behavior. CompareConfiguration#isMirrored()/#isMirroringEnabled() do not reflect the intended new behavior. Introducing a new behavior should not force unaffected old clients to implement a new API. LTK UI will not accept https://git.eclipse.org/r/#/c/85840/ . Please modify the "Swap sides" feature so that existing compare clients don't get broken by a preference change.
The new preference allows users to fix editors (like CVS and egit) that weren't conforming to the platform spec (which you describe in comment 5, and which the patch comment describes more precisely). However, the preference is *breaking* editors which conform to the spec such as the ltk preview pane. This patch allows call sites to opt out of the preference if they already conform to the spec. Call sites which didn't conform to the spec get the preference by default, so that users who are used to the old behavior still get an opt-out. > Introducing a new behavior should not force unaffected old clients > to implement a new API We either have to make a change to LTK or we have to change all the team plugins (CVS, Egit, etc.). One went with old version on the left and the other went with old version on the right. There's no way for the compare framework itself to identify which version is which unless the caller identifies it somehow, so some old call sites need to change. > Please modify the "Swap sides" feature so that existing compare clients > don't get broken by a preference change. How else would you suggest doing so besides the approach proposed here?
(In reply to Markus Keller from comment #5) > The established policy in Eclipse compare editors was: > > 1) Left: Local workspace version; Right: Remote version > > 2) When (1) is not applicable because no local version is being compared: > Left: Older version; Right: Newer version The problem is that this policy leads to an inconsistent behavior of the compare view with itself and also with most code review systems. Consider the following scenario: 1. Preview of a refactoring change - the new code is shown on the right 2. The same code compared with HEAD in EGit - the new code is shown on the left (the same when comparing with local history and in CVS) 3. The same code reviewed in Gerrit - the new code is on the right. A more consistent policy was proposed in https://bugs.eclipse.org/bugs/show_bug.cgi?id=504474#c3 but implementing it across all clients of the compare view will take considerable time. The "Swap left and right view" option was introduced to alleviate user suffering in meantime. The change https://git.eclipse.org/r/85840 is necessary to make sure that user suffering is in fact reduces.
I think we agree that the TextEditChangePreviewViewer correctly uses the compare APIs to set the left and right sides of the comparison. If the addition of a new preference is breaking old clients that conformed to the old API, then something is wrong with the way the new preference was added. You seem to assume that the refactoring preview is the only old client that needs to be updated for the new preference. That assumption is wrong by definition, since you cannot know the set of clients that use a public API. In the Eclipse project, we don't break APIs just because that looks like it's the easiest way to hack a feature. If you want to change an established rule, you have to convince clients to opt in to the new rule. I'm afraid I don't have time to perform the necessary investigations and write a spec for your new feature. You should start with clarifying what exactly the feature should do. Out of that, you should be able to come up with a name that better describes the intention of the feature. A global "Swap sides" or "mirror" is apparently not what you intended to implement. I quickly checked what CVS and Local History did in 3.8.2, and I was surprised that Compare with Each Other on two versions showed the newer version on the left. I.e. that doesn't match my assumption, so if I had to implement this feature, I would have to go back and see if I can find out why is was implemented like that. Maybe the assumption was that the local version is often the newest version, and then they extrapolated [left: local; right: remote] into [left: newer; right: older]. Maybe the option should be "Show latest/local on the right". Note that the word "maybe" tells you that this is just a hint, not a definitive advice.
I'm not sure it will still make sense to have a button in the compare viewer that toggles the global preference. Every button makes a UI harder to use, and rarely-used options should be put on a preference page. However, to support users working with a plug-in that should consume the new preference but doesn't do that yet, a "swap sides" button in the compare viewer could be helpful. It would be nice to store the state of that toggle per "compare implementation". I don't know the compare code structure by heart, but I guess it should be possible to find an object that could be used to identify different usage contexts like refactoring preview, local history compare editor, CVS compare editor, EGit compare editor, etc. (maybe the class of a compare viewer input or a viewer implementation). If you really want to keep the global preference toggle in the compare viewer, then it could go into a drop-down menu of the local "swap sides" toolbar button.
(In reply to Markus Keller from comment #9) > I'm not sure it will still make sense to have a button in the compare viewer > that toggles the global preference. Every button makes a UI harder to use, > and rarely-used options should be put on a preference page. Agree. The only reason the button was put in the compare viewer was to make the new option easier to discover. > However, to support users working with a plug-in that should consume the new > preference but doesn't do that yet, a "swap sides" button in the compare > viewer could be helpful. Ideally, all plug-ins that use compare viewer should be changed to use a consistent set of rules for deciding what goes on the right and what on the left side. The "Swap sides" option was created only because a consistent set of rules appeared unattainable due to disagreements over consistency criteria. It makes sense to make another attempt at reaching consensus on a consistent set of rules. I'm going to use bug 508257 for that. Since there is no way to make all third-party plugins comply to the new set of rules in short term, it still makes sense to give users an ability to swap compare viewer sides when they find the current representation unnatural. Once and if we agree on a set of consistent rules, I will propose a mechanism for limiting the scope of the "Swap side" preference to affect only the compare viewer clients that did not assert compliance with the rules. > It would be nice to store the state of that toggle per "compare > implementation". I don't know the compare code structure by heart, but I > guess it should be possible to find an object that could be used to identify > different usage contexts like refactoring preview, local history compare > editor, CVS compare editor, EGit compare editor, etc. (maybe the class of a > compare viewer input or a viewer implementation). I also thought of something like that. Looks pretty hacky, but would allow user to adjust appearance of the compare view on case by case basis. Before getting deep into this let's try to come up with an agreement on the set of consistent rules. I'm reverting the change that introduced isMirroringAllowed/setMirroringAllowed methods to focus on the consistent rules first.
New Gerrit change created: https://git.eclipse.org/r/86543
Gerrit change https://git.eclipse.org/r/86543 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.team.git/commit/?id=97118c23b0193698f0b34f1468117c4070a797dc
What's the status of this?
Sergey and I had a bunch of discussions and worked out a proposal for a consistent set of criteria for compare editors. I have a lot of notes on it but need time to put them together into a coherent proposal. Honestly, though, I'm swamped with index work and this much lower priority... so I won't have time to do it for 4.7.
(In reply to Stefan Xenos from comment #14) > Sergey and I had a bunch of discussions and worked out a proposal for a > consistent set of criteria for compare editors. I have a lot of notes on it > but need time to put them together into a coherent proposal. > > Honestly, though, I'm swamped with index work and this much lower > priority... so I won't have time to do it for 4.7. Could you at least dump the high-level conclusions?
I think I filed a bug for it already. Let me try to dig it up...
bug 516450 reflects the proposal we came up with. If it can be formalized somehow, then dependent bugs like this one can be filed to bring the various bits of UI into conformance.
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
