Bug 512771 - Saving in compare editor doesn't work if swapping left and right view is enabled
Summary: Saving in compare editor doesn't work if swapping left and right view is enabled
Status: CLOSED WONTFIX
Alias: None
Product: Platform
Classification: Eclipse Project
Component: Compare (show other bugs)
Version: 4.6   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Platform-Compare-Inbox CLA
QA Contact:
URL:
Whiteboard: stalebug
Keywords: needinfo
Depends on:
Blocks:
 
Reported: 2017-02-27 11:05 EST by Sebastian Priebe CLA
Modified: 2021-01-04 00:14 EST (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Sebastian Priebe CLA 2017-02-27 11:05:05 EST
Saving changes in the compare editor doesn't work if the swap sides preference is activated (i.e. "Swap left and right view").

If the compare editor is closed without saving it asks for saving but it is not saved if "Yes" is selected.

Also Strg+S doesn't save the performed changes.

Saving works without problems if swapping is disabled (i.e. "Swap left and right view" is not selected).


-- Configuration Details --
Product: Eclipse 4.6.2.20161208-0625 (org.eclipse.epp.package.cpp.product)Installed Features:
 org.eclipse.platform 4.6.2.v20161124-1529
Comment 1 Andrey Loskutov CLA 2017-03-02 02:37:20 EST
Can you please provide exact steps to reproduce?

I've tried this right now with 4.6.3 RC3 and it works fine for me if I (with .properties or .java files):

1) Change local file - add a comment
2) Git Compare with -> Head
3) In the compare editor, on the right (local) side, add another comment
4) Save via "Ctrl+S"
Comment 2 Sebastian Priebe CLA 2017-03-02 02:59:52 EST
I'm using the Subclipse plugin.

1) Compare With -> Base Revision
2) Use the button "Copy current change from Left to Right"
3) Save via "Ctrl+S"
4) Close the compare editor
5) Open file which was changed -> Still the old content, changes not saved
Comment 3 Andrey Loskutov CLA 2017-03-02 09:27:21 EST
(In reply to Sebastian Priebe from comment #2)
> I'm using the Subclipse plugin.
> 
> 1) Compare With -> Base Revision
> 2) Use the button "Copy current change from Left to Right"
> 3) Save via "Ctrl+S"
> 4) Close the compare editor
> 5) Open file which was changed -> Still the old content, changes not saved

A-ha. It works with egit and local history too. Can you please try if you can simply edit a local file, and see same behavior if you do "compare with local history" and then same steps as above? Probably the problem is in subclipse?
Comment 4 Sebastian Priebe CLA 2017-03-02 11:18:44 EST
For me it also doesn't work with local history.

If I have the local file opened and I hit Ctrl+S in the compare editor the file gets marked dirty (with a *) but it is not saved.

The same if the file isn't open. If I open it after hitting Ctrl+S in the compare editor it is marked dirty, too.

Please note: I'm working with version 4.6.2 with the CDT stuff.
Comment 5 Sebastian Priebe CLA 2017-03-02 11:37:55 EST
I just tried CDT 4.6.3 RC2 with the same results for Subclipse but it works with the local history.
Comment 6 Andrey Loskutov CLA 2017-03-02 11:45:42 EST
(In reply to Sebastian Priebe from comment #5)
> I just tried CDT 4.6.3 RC2 with the same results for Subclipse but it works
> with the local history.

Thanks, I will close this one as "not Eclipse". 

I can't reproduce with the tooling coming from eclipse.org, so most likely you should open a ticket at subclipse for investigation. If it turns out, the problem is in Eclipse code, please feel free to reopen this ticket but please provide detailed explanation why do you think the problem is in Eclipse.org code.
Comment 7 Tim Kosacek CLA 2018-07-31 12:56:08 EDT
I think there still might be an issue here.  We are extending CompareEditorInput and then using CompareUI.openCompareEditor(compareInput) to open the compare/merge editor.  If we set the right side editable but then either the "Swap left and right view" button is used in the compare/merge editor or the corresponding preference is enabled, when making a change and requesting a save, ContentMergeViewer.flushLeftSide() is checking getCompareConfiguration().isLeftEditable() to see if the left side is editable - this is false as was defined in the initial CompareConfiguration so the changes are NOT saved.  It seems like ContentMergeViewer should be using its own isLeftEditable() method to check whether the left side is editable - this method checks CompareConfiguration.isMirrored() and checks the configuration's left/right side for editable accordingly.  

Let me know if I'm wrong on this as I'm just not seeing how/where to handle the swap/mirror preference change if part of this is up to the user of these classes.
Comment 8 Andrey Loskutov CLA 2018-07-31 16:14:25 EDT
Tim, can you provide steps to reproduce, or example plugin? Do you want to contribute a fix? See https://wiki.eclipse.org/Platform_UI/How_to_Contribute.
Comment 9 Tim Kosacek CLA 2018-07-31 16:17:53 EDT
(In reply to Andrey Loskutov from comment #8)
> Tim, can you provide steps to reproduce, or example plugin? Do you want to
> contribute a fix? See https://wiki.eclipse.org/Platform_UI/How_to_Contribute.

Yes, I should hopefully be able to carve out some time this week to provide a simple example to reproduce the bug and a suggested fix.

Thanks,

Tim
Comment 10 Andrey Loskutov CLA 2018-07-31 16:38:57 EDT
(In reply to Tim Kosacek from comment #9)
> Yes, I should hopefully be able to carve out some time this week to provide
> a simple example to reproduce the bug and a suggested fix.

This would be great. BTW, I believe I've seen this in the EGit compare editor if comparing index with head, edits are not persisted if the sides are swapped (or working copy with index?). @EGit team: have you observed this too, is this known issue?
Comment 11 Thomas Wolf CLA 2018-08-01 15:36:09 EDT
(In reply to Andrey Loskutov from comment #10)
> @EGit team: have you observed this too, is this known issue?

I have not observed this, but then I basically never use this side switching. AFAIK there is no bug report in EGit relating to this. Just gave it try, and I cannot reproduce this with EGit.
Comment 12 Andrey Loskutov CLA 2019-01-14 10:16:50 EST
OK, looks like I've got this now:

1) add two lines to text file in two different places so that we have two text diffs
2) stage the file and open diff from the staging view by double clicking staged file
3) Make sure "Swap left and right" has toggled (pressed) state, so the new lines are on the right side
4) Place cursor on the first diff and click on "Copy current change from left to right". This should undo the change in index.
5) Save the "dirty" diff via Ctrl+S.
This *should* undo the change on the index and refresh the view so that we see a change in the working tree - but nothing changes at all (except the diff shows us no change in index). Commit now this change.
6) In the git history we see that both lines are committed.

If "Swap left and right" is NOT toggled, in step 5 we commit only one line, and working tree shows another change in the unstaged area.
Comment 13 Thomas Wolf CLA 2019-01-14 17:48:56 EST
(In reply to Andrey Loskutov from comment #12)
> OK, looks like I've got this now:
> 
> 1) add two lines to text file in two different places so that we have two
> text diffs
> 2) stage the file and open diff from the staging view by double clicking
> staged file
> 3) Make sure "Swap left and right" has toggled (pressed) state, so the new
> lines are on the right side
> 4) Place cursor on the first diff and click on "Copy current change from
> left to right". This should undo the change in index.
> 5) Save the "dirty" diff via Ctrl+S.
> This *should* undo the change on the index and refresh the view so that we
> see a change in the working tree - but nothing changes at all (except the
> diff shows us no change in index). Commit now this change.
> 6) In the git history we see that both lines are committed.
> 
> If "Swap left and right" is NOT toggled, in step 5 we commit only one line,
> and working tree shows another change in the unstaged area.

This appears to be fixed with the fix for bug 522245. I can reproduce it with     EGit nightly 5.3.0.201901061808, but EGit nightly 5.3.0.201901121419 works as expected.

The difference between these two EGit nightly versions are exactly the change done for bug 522245 plus a clean-up: https://git.eclipse.org/r/#/c/134955/ and https://git.eclipse.org/r/#/c/134956/ .
Comment 14 Eclipse Genie CLA 2021-01-04 00:14:32 EST
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. As such, we're closing this bug.

If you have further information on the current state of the bug, please add it and reopen this bug. 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.