Fix ctrl z textfield #11421

koppor · 2024-06-24T12:08:47Z

Fixes #11420

However, this is a hard one. Do we need to implement undo/redo for textfields for ourselves?

Mandatory checks

Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

Siedlerchr · 2024-06-24T15:25:36Z

This was working until recently. I wonder what happend to it.

I would suggest adding a text field to the reproducer and try it out

src/main/java/org/jabref/gui/frame/MainMenu.java

Siedlerchr · 2024-07-07T13:16:05Z

ctrl + y still produces exception when I play around with multiple crtl+z and ctrl+y in comment field

javax.swing.undo.CannotRedoException
	at java.desktop/javax.swing.undo.UndoManager.tryUndoOrRedo(UndoManager.java:469)
	at java.desktop/javax.swing.undo.UndoManager.redo(UndoManager.java:453)
	at org.jabref@100.0.0/org.jabref.gui.undo.CountingUndoManager.redo(CountingUndoManager.java:37)
	at org.jabref@100.0.0/org.jabref.gui.undo.RedoAction.execute(RedoAction.java:21)
	at org.jabref@100.0.0/org.jabref.gui.actions.JabRefAction.lambda$new$1(JabRefAction.java:25)
	at org.controlsfx.controls@11.2.1/org.controlsfx.control.action.Action.handle(Action.java:423)
	at org.controlsfx.controls@11.2.1/org.controlsfx.control.action.Action.handle(Action.java:64)
	at javafx.base@22.0.1/com.sun.javafx.event.CompositeEventHandler.dispatchBubblingEvent(CompositeEventHandler.java:86)
	at javafx.base@22.0.1/com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:232)
	at javafx.base@22.0.1/com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:189)
	at javafx.base@22.0.1/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:58)
	at javafx.base@22.0.1/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
	at javafx.base@22.0.1/com.sun.javafx.event.EventUtil.fireEventImpl(EventUtil.java:74)
	at javafx.base@22.0.1/com.sun.javafx.event.EventUtil.fireEvent(EventUtil.java:49)
	at javafx.base@22.0.1/javafx.event.Event.fireEvent(Event.java:198)
	at javafx.controls@22.0.1/javafx.scene.control.MenuItem.fire(MenuItem.java:459)
	at javafx.controls@22.0.1/com.sun.javafx.scene.control.GlobalMenuAdapter.lambda$bindMenuItemProperties$2(GlobalMenuAdapter.java:150)
	at javafx.base@22.0.1/com.sun.javafx.event.CompositeEventHandler.dispatchBubblingEvent(CompositeEventHandler.java:86)
	at javafx.base@22.0.1/com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:232)
	at javafx.base@22.0.1/com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:189)
	at javafx.base@22.0.1/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:58)
	at javafx.base@22.0.1/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
	at javafx.base@22.0.1/com.sun.javafx.event.EventUtil.fireEventImpl(EventUtil.java:74)
	at javafx.base@22.0.1/com.sun.javafx.event.EventUtil.fireEvent(EventUtil.java:54)
	at javafx.base@22.0.1/javafx.event.Event.fireEvent(Event.java:198)
	at javafx.controls@22.0.1/javafx.scene.control.MenuItem.fire(MenuItem.java:459)
	at javafx.graphics@22.0.1/com.sun.javafx.tk.quantum.GlassSystemMenu$1.action(GlassSystemMenu.java:237)

koppor · 2024-07-07T13:17:39Z

ctrl + y still produces exception when I play around with multiple crtl+z and ctrl+y in comment field

This is not the original issue. The original issue is an NPE. - I will look into that nevertheless.

Update: The thing triggered according to the exception is a menu item; not through the text field.

koppor · 2024-07-07T15:04:48Z

javax.swing.undo.CannotRedoException
at java.desktop/javax.swing.undo.UndoManager.tryUndoOrRedo(UndoManager.java:469)

Nice find! Was because of my OO refactoring. I did not copy the full branch for RedoAction. 😅

koppor · 2024-07-07T15:07:02Z

Follow up:

Have menu items disabled if there is no undo / redo
Implement undo/redo for "Clear, Change Case, Normalize"

calixtus · 2024-07-07T15:18:49Z

Please do not verschlimmbesser the undoredo stuff anymore. Small refactorings ok, but the whole concept of undo/redo has to be redone.

koppor · 2024-07-08T06:11:54Z

Please do not verschlimmbesser the undoredo stuff anymore. Small refactorings ok, but the whole concept of undo/redo has to be redone.

Aim for this PR: Get it working for users. Do as less changes as possible. Keep changes local. If new code is required, add clean code. Do not add hacky code. -- Therefore, I needed to make UndoRedoAction abstract and exchange the hacky if to real classes. (I don't find the name of the resolved anti pattern here)

calixtus · 2024-07-08T06:42:44Z

You can probably remove the now abstract undoredo class anyway and make the undo and the redo class directly extend simplecommand.

…before)

koppor · 2024-07-08T07:13:23Z

You can probably remove the now abstract undoredo class anyway and make the undo and the redo class directly extend simplecommand.

Done in 765e643 (#11421). I think, this code duplication is OK...

koppor · 2024-07-08T08:33:59Z

Have menu items disabled if there is no undo / redo

Discussed with @calixtus: Very hard. -- I also tried it locally, but the dependency of tab -> undomanger -> canUndo drives me crazy. I was searching for Bindings.andThen. Chain of bindings. But I did not find any solution.

koppor · 2024-07-08T08:38:15Z

* Implement undo/redo for "Clear, Change Case, Normalize"

I checked in this branch. Works.

koppor · 2024-07-08T08:42:07Z

@LoayGhreeb I reply to your comment #11420 (comment) here to be able to track the progress w.r.t. to the code commits. (The commits and the comments are sorted by time in a pull request. Thus, one sees, whether a comment was made before a commit or after one).

Using the menu bar (Edit -> Undo) to undo changes works without any exceptions.

This is good 😅

Another issue, which may or may not be related, is that when updating a field using the context menu (Clear, Change Case, Normalize) and using Ctrl+Z to undo this change, nothing happens and no exception occurs.

I tested it with the latst commit here. Works.

However, when using "Protect Selection" from the context menu, this change is undoable but with the exception.

Works here without exception.

Maybe, you can re-check? 😅

github-actions · 2024-07-08T08:53:18Z

The build for this PR is no longer available. Please visit https://builds.jabref.org/main/ for the latest build.

LoayGhreeb · 2024-07-08T09:30:02Z

Maybe, you can re-check? 😅

I tried it, and it works without exceptions. However, can we do some checks before doing the undo/redo to see if the change is related to the focused field and selected entry or not?
Because now, if I add/remove an entry, pressing Ctrl + Z/Y in the text field will remove/restore the entry again.

koppor · 2024-07-08T09:37:34Z

Maybe, you can re-check? 😅
I tried it, and it works without exceptions. However, can we do some checks before doing the undo/redo to see if the change is related to the focused field and selected entry or not?

Why? I was thinking that there should be one line of redo/undo changes and not some local textfield ones and some "global" ones not realted to the textfield ones?

I think, the behavior of the last release was that an undo of a text field (JavaFX behavior) was recorded as new change event in JabRef's global undo/redo. And then pressing undo from the menu lead to strange effects.

Now, there is only one global undo/redo chain. For sure, if one changes a text field, does somewhere else somethings, than goes back to the text field, types a letter, presses ctrl+Z twice, then the "somethings" are undone.

Because now, if I add/remove an entry, pressing Ctrl + Z/Y in the text field will remove/restore the entry again.

This is the effect of a single global undo/redo stack.

Alternatively, we could "jump" in the undo chain and search for the last action regarding the text field and undo/redo that. -- I would stay back from using JavaFX textfield's local undo/redo (put aside for now that there is an NPE and we are in the need of fix JavaFX again).

Siedlerchr · 2024-07-08T09:47:42Z

I'm okay with the current approach.

calixtus · 2024-07-08T14:22:27Z

src/main/java/org/jabref/gui/undo/CountingUndoManager.java

-    private final EventBus eventBus = new EventBus();
-


calixtus · 2024-07-08T14:26:44Z

This is the effect of a single global undo/redo stack.

Alternatively, we could "jump" in the undo chain and search for the last action regarding the text field and undo/redo that. -- I would stay back from using JavaFX textfield's local undo/redo (put aside for now that there is an NPE and we are in the need of fix JavaFX again).

This can be resolved maybe by making undo actions in an event stream mergeable. So related edits (like entering a character) can be merged to larger undoables und be undone and redone as a compund (like now)

koppor · 2024-07-08T16:04:36Z

This is the effect of a single global undo/redo stack.

Alternatively, we could "jump" in the undo chain and search for the last action regarding the text field and undo/redo that. -- I would stay back from using JavaFX textfield's local undo/redo (put aside for now that there is an NPE and we are in the need of fix JavaFX again).

This can be resolved maybe by making undo actions in an event stream mergeable. So related edits (like entering a character) can be merged to larger undoables und be undone and redone as a compund (like now)

In addition, I meant:

Type a in field F1.
Type b in field F2.
Type c in Field F1.
Press Ctrl+Z twice in Field F1.

Loay expects F1 being empty.

Current behavior: F1 contains a and F2 contains nothing.

calixtus · 2024-07-08T16:41:13Z

i see.
i would expect the current behaviour. if you are jumping between fields, then every change has its own compund.

koppor · 2024-07-11T14:56:15Z

OK, Loay is asking for "Objektbezogenes Undo". The text there does not fully explain how it works with hierarchical objects. It also does not link real-world examples (which makes it a "pattern candidate")

koppor · 2024-07-11T15:01:03Z

if you are jumping between fields, then every change has its own compund.

We need to craft this. As you outlined. - But maybe we should only append if it is the same operation: add / delete. Replace should be in a new thing.

Meaning:

(a, ab) + (ab, abc) ->(a, abc) // both changes were additions, merge second into first
(abcd, abcde) + (abcde, abcd) -> (abcd, abcde), (abcde, abcd) // different actions, keep both, no merge

koppor added 3 commits June 24, 2024 14:06

WIP: Fix Ctrl+Z

3f0192d

Add comment

8d02b8e

Remove non-working UndoRedoEventManager

ca406ad

koppor mentioned this pull request Jun 26, 2024

Ctrl+Z (undo) not working any more #11420

Closed

koppor added 4 commits July 7, 2024 09:48

Merge remote-tracking branch 'upstream/main' into fix-ctrl-z-textfield

dcd0e69

Refactor UndoRedo to more proper OO

9f99c4a

Use JabRef's handling for Ctrl+Z / Ctrl+Y

01252fe

Remove double call to library changed marking

8906f8c

github-actions bot reviewed Jul 7, 2024

View reviewed changes

src/main/java/org/jabref/gui/frame/MainMenu.java Outdated Show resolved Hide resolved

Fix checkstyle

a58a7da

koppor added 2 commits July 7, 2024 16:50

Fix comment

0b8fc9c

Fix caught exception

8a96080

koppor changed the title ~~WIP: Fix ctrl z textfield~~ Fix ctrl z textfield Jul 7, 2024

koppor marked this pull request as ready for review July 7, 2024 15:06

koppor added 3 commits July 8, 2024 09:08

Obey keyboard shortcuts for undo and redo

28709be

Add CHANGELOG.md entry (because the general issue appeared in JabRef …

235d418

…before)

Inline UndoRedoAction

765e643

koppor added 3 commits July 8, 2024 09:16

Fix name

d0d73b0

Remove EventBus from CountingUndoManager

a02e9eb

Remove obsolete call

53103ff

Fix checkstyle

3230f37

koppor added this to the 5.14 milestone Jul 8, 2024

koppor added the status: devcall label Jul 8, 2024

calixtus reviewed Jul 8, 2024

View reviewed changes

calixtus approved these changes Jul 8, 2024

View reviewed changes

Siedlerchr approved these changes Jul 8, 2024

View reviewed changes

calixtus added this pull request to the merge queue Jul 8, 2024

Merged via the queue into main with commit 3ee825d Jul 8, 2024
22 checks passed

calixtus deleted the fix-ctrl-z-textfield branch July 8, 2024 14:48

koppor removed the status: devcall label Jul 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ctrl z textfield #11421

Fix ctrl z textfield #11421

koppor commented Jun 24, 2024

Siedlerchr commented Jun 24, 2024

Siedlerchr commented Jul 7, 2024 •

edited

Loading

koppor commented Jul 7, 2024 •

edited

Loading

koppor commented Jul 7, 2024

koppor commented Jul 7, 2024

calixtus commented Jul 7, 2024

koppor commented Jul 8, 2024

calixtus commented Jul 8, 2024

koppor commented Jul 8, 2024

koppor commented Jul 8, 2024

koppor commented Jul 8, 2024

koppor commented Jul 8, 2024

github-actions bot commented Jul 8, 2024 •

edited

Loading

LoayGhreeb commented Jul 8, 2024

koppor commented Jul 8, 2024

Siedlerchr commented Jul 8, 2024

calixtus Jul 8, 2024

calixtus commented Jul 8, 2024

koppor commented Jul 8, 2024

calixtus commented Jul 8, 2024

koppor commented Jul 11, 2024

koppor commented Jul 11, 2024

Fix ctrl z textfield #11421

Fix ctrl z textfield #11421

Conversation

koppor commented Jun 24, 2024

Mandatory checks

Siedlerchr commented Jun 24, 2024

Siedlerchr commented Jul 7, 2024 • edited Loading

koppor commented Jul 7, 2024 • edited Loading

koppor commented Jul 7, 2024

koppor commented Jul 7, 2024

calixtus commented Jul 7, 2024

koppor commented Jul 8, 2024

calixtus commented Jul 8, 2024

koppor commented Jul 8, 2024

koppor commented Jul 8, 2024

koppor commented Jul 8, 2024

koppor commented Jul 8, 2024

github-actions bot commented Jul 8, 2024 • edited Loading

LoayGhreeb commented Jul 8, 2024

koppor commented Jul 8, 2024

Siedlerchr commented Jul 8, 2024

calixtus Jul 8, 2024

Choose a reason for hiding this comment

calixtus commented Jul 8, 2024

koppor commented Jul 8, 2024

calixtus commented Jul 8, 2024

koppor commented Jul 11, 2024

koppor commented Jul 11, 2024

Siedlerchr commented Jul 7, 2024 •

edited

Loading

koppor commented Jul 7, 2024 •

edited

Loading

github-actions bot commented Jul 8, 2024 •

edited

Loading