Added information in the tool tip for undo and redo and disabled when…

[oscargus]

A tool tip text is shown describing the undo/redo action. Also, the undo/redo buttons are enabled/disabled when there are things to undo/redo.

Current problem is that it doesn't work on startup (both undo and redo is enabled as before), but as soon as one edit is done it works just fine.

• Change in CHANGELOG.md described
• Screenshots added (for bigger UI changes)

… no undo/redo is available

[simonharrer]

Shouldn't this be queriable from the undo/redo action? So each action that can be made undo/redo should have an identifier describing its action.

[oscargus]

You mean basically move the updateText logic to a method for the UndoAction and RedoAction classes and have a single call in JabRefFrame like updateUndoAndRedoTexts which calls these to methods?

Yes, that might be a better idea. However, I'm not really sure if one also needs to change the corresponding classes in the preamble editor and string dialog. (Nor, actually, how it works there now...)

[oscargus]

It can indeed be queried. However, the actual undo/redo classes are in BasePanel but the actions associated to menu and toolbar are defined in JabRefFrame, providing the undo/redo class instance. So, the undo/redo class have no idea about its tooltip.

When I tried it out now, it appeared much more flaky, completely ignoring "change field" edits. No idea why this happens as it worked well earlier. This happens also in master so trying to figure out what might have changed... Most (all?) other types of edits can be undone, but "change field"...

[oscargus]

@JabRef/developers : I'm not opening a new issue, but can you please check to see if you can undo field changes in the current master? It seems like it stopped working sometime between creating this PR and now. From debugging I can tell that addEdit() is indeed called as it should, but somehow the edits are not stored. This appears to only happen for "change field" edits as the other ones I've tried are OK.

I've looked through the recent commits multiple times to try to figure out what have changed, but at no luck...

[oscargus]

@JabRef/developers: Just after posting, I think I found the issue... No emergency...

[oscargus]

The problem was actually discovered because of this and just having worked with #1575 (which I will fix later) and fixed in #1576. Totally unrelated to any recent commit.

[tobiasdiez]

I don't like that the UndoManager has to know about the frame. Thus I would propose to use "inversion of control" here. That is, the CountingUndoManager throws an event (using the new EventBus) whenever the last undoable action changes. In the frame you can then subscribe to this event and change the display accordingly. In the event you can store the undoable event (of type UndoableEdit) and then simply call event.getUndoPresentationName() to get the description.

Using events in a similar manner, you can also move the panel.updateEntryEditorIfShowing(); callings from the undo manager to the frame.

[oscargus]

OK, EventBus is introduced and I think the structure is clearly better. It would now be nice if there are more "advanced" getPresentationName() for the Undoable actions with content from the actual action. Not in this PR though.

[oscargus]

There is a minor flaw which may not be so easy to solve (and is not that serious): if one makes an edit in the entry editor and directly selects a different tab, the undo/redo state is from the previous tab. Also, when trying to undo in the new tab, the state is not updated as an Exception is thrown and the code never gets to the place where the update is done.

[oscargus]

Rebased. Anything else here?

[simonharrer]

My head is spinning by thinking about UndoRedoEvents, as undo and redo should cancel each other out, leaving only a event? Can you please document this? And the AddUndoEvent als well? What should be described in these events? When are they fired?

[tobiasdiez]

In addition, I also don't get why UndoRedo derives from AddUndo. I would prefer if only one event UndoableActionChangedEvent would be needed, i.e. one event for adding a new undo action and the same event is fired when an action is "redoed".

[oscargus]

I'll add comments. Quickly: AddUndoEvent when something is added to the undo manager (update actions), UndoRedoEvent when something is undone/redone (update actions and possibly entry editor).

[oscargus]

Basically to avoid copying code as they have exactly the same content but two different types are needed. An alternative might be to add the type in the event but that seemed worse.

[matthiasgeiger]

Just duplicate the code and do not introduce inheritance if it is semantically inadequate.

Maybe the meaning of "AddUndoEvent" gets clearer if it is named "AddUndoableActionEvent"?

[simonharrer]

👍 for code duplication in that case.
👍 for "AddUndoableActionEvent", but still, is this just not something that is happening automatically because of other events? So do we really need this event as such? To add an undoable item to the undo manager seems a little bit too low from a granularity point of view. We should add the event that a save was triggered, and this would then be also the "AddUndoableActionEvent" as it was a save event. Does this make sense?
👍 for "UndoOrRedoEvent"

[oscargus]

Another reason for inheriting was that there could be a shared function for updating the text. I have copied that now though. The reason that such an event is needed is that it is triggering the tool tip changes. That is all it does. Will push tomorrow. :-)

[tobiasdiez]

You can create a common base event like Undo(Manager?)ChangedEvent and let both events derive from this new class.

[oscargus]

I improved the information a bit: