Bug 432403

Comment 1 Vikas Chandra 2014-04-09 04:59:46 EDT

This is the fix

https://git.eclipse.org/r/24689

Comment 2 Vikas Chandra 2014-04-09 08:15:15 EDT

Amended  the previous commit to have undo-redo work for ExportPackageObject

Comment 3 Curtis Windatt 2014-04-09 12:10:13 EDT

The readded items aren't given focus, so undo may add items scrolled off the viewer and I have no idea whether the undo worked or not.  This is compounded when making changes to both required bundles and import packages then undoing them (unusual scenario).

More problematic if the fact that I can get duplicate entries in the exported packages list.  Nailing down steps to reproduce is difficult, try the following (or just try various combinations of adding/removing then undo/redoing)

1) Open org.eclipse.pde.ui manifest
2) Remove org.eclipse.pde.internal.ui
3) Add the package back (add...)
4) Ctrl-Z to undo the add
5) Add the package back (add...)
6a) Ctrl- Z to undo the add
6b) Ctrl-Z to undo the removal *** This press of Ctrl-Z does nothing, so the problem is likely here
7) Ctrl-Z to undo the removal
8) Ctrl-Y to redo the removal
9) Ctrl-Y to redo the add
10) Ctrl-Y to redo the removal...
This is broken and instead of removing the existing entry, it adds a second one
Also, the undo/redo list is broken and will continue to add duplicates

Debugging through it, step 6b), there is likely a problem with the model events being recorded as it thinks the next undo step is an insert, not a removal.

In the end, having some undo/redo support is better than none, so would still merge this change, but if possible please take a look.

Comment 4 Vikas Chandra 2014-04-14 10:36:23 EDT

I understood what is happening for the scenario 1- 10

The current design is all the UNDOs are ignored as far as list of operations performed is concerned

So
Step 2 - operation =(remove}
Step 3 - operation = {remove, add}
Step 4 - operation = {remove, add}  *IGNORED* because UNDO
Step 5 -  operation = {remove, add, add}
Step 6a ) - undo last operation - Undo add  ( op[2])
Step 6b)  - undo the next one in line - Undo add AGAIN op[1]).. Nothing present..Nothing remove
Step 7 - Undo op[0] - meaning adding that package.


So either the events of UNDO should be added in operations and counter not decreased OR the UNDO should remove the operation from the operation list whose UNDO was done.

I tried the former approach and it works fine
		ignoreChanges = true;
		openRelatedPage(op);
		execute(op, true);
		cursor--;
		updateActions();

but if ignoring changes is at certain of undo design ( as seen above), I  can do the latter.

Can you please give your views?

Can y

Comment 5 Curtis Windatt 2014-04-14 11:54:06 EDT

(In reply to Vikas Chandra from comment #4)
> So either the events of UNDO should be added in operations and counter not
> decreased OR the UNDO should remove the operation from the operation list
> whose UNDO was done.

Would removing the op from the list prevent REDO from working?  Or does the UNDO operation add another op that the REDO can use?

As I assume all the changes would be in the PDE undo/redo manager, this is a bug, not part of the design.

In my mind, there should be a list of operations (with some max number stored) and undo/redo would simply move the pointer in the list.  If another operation was done after some undos (counter is earlier in the list), then all ops after the pointer would be deleted, and the new op added at the end (pointer now points to the end).  I believe this is how undo/redo typically works, where if you undo a few times then start making edits, you can no longer redo.

I'd have to look at the patch in more detail, but I think your approach is reasonable.

Comment 6 Curtis Windatt 2014-05-13 17:50:40 EDT

http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=6c1974e401c328481d97f6218e678f9a8a00c117

I approved the gerrit change as we should have undo/redo support in the editor.

However, in 4.5 (or in late 4.4 if there is time) we should look at:
1) Fixing the selection
2) Solving the broken case from comment 3 (may apply to more than just the plug-in editor)

Comment 7 Vikas Chandra 2016-04-01 04:23:05 EDT

Since lot of other items are there for PDE in 4.6, I am moving this out of 4.6. Also some part of this is already fixed by the commit in previous comment.

Comment 8 Eclipse Genie 2019-01-12 12:19:13 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.

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.

Comment 9 Lars Vogel 2019-09-02 15:01:40 EDT

This bug was marked as stalebug a while ago. Marking as worksforme.

If this report is still relevant for the current release, please reopen and remove the stalebug whiteboard tag.

Comment 10 Lars Vogel 2019-09-02 15:02:08 EDT

This bug has been marked as stalebug a while ago without any further interaction.

If this report is still relevant for the current release, please reopen and remove the stalebug whiteboard flag.