A bug about the attachment and the deletion

[idadoudou]

JabRef 5.3--2021-01-02--0b4be8a
Linux 4.18.0-240.1.1.el8_3.x86_64 amd64
Java 15.0.1

Steps to reproduce:

1. Attach a file to a record.
2. Select another record and press the "delete" key to delete it.
3. The delete warning window shows information about the attached file not the record itself.

[calixtus]

Seems a binding is not updated properly.

[BenArou]

I'm a junior software engineering student interested in this issue. I would like to take a look into it and see what I can do. May I take this issue? If so, I would like a little guidance as I'm new to this project. Thanks in advance!

[calixtus]

Hello @BenArou nice to hear, that you took interest in JabRef.
To get started, I would suggest you take a look at our developer documentation ( https://devdocs.jabref.org/ ) and start by solving some "good first issues" in our issue tracker ( https://github.com/JabRef/jabref/issues?q=is%3Aissue+is%3Aopen+label%3A%22good+first+issue%22 ) to become familiar with the code base of JabRef.
As this Issue is marked as a good first issue and I don't see, that anyone else is working on this, I would give it a go. You should take a look at the constructor of the MainTable class, it's Method setupKeyBindings and maybe also the EntryEditor class. Try to reproduce the issue, debug what happens, if you press the delete key and find out which BibEntry is connected and when it is connected. Good lock and have fun!

[tp-1000]

It looks like there is an issue that stems from ContextMenu MenuItem's accelerators overlapping with the MainMenu accelerators. Not sure theres a clean solution. Where this pattern appears in the application (such as RightClickMenu generation) there are also similar issues.

Note: The accelerator issue when deleting a linked file is related but not the complete problem. It also appears there are some focus issues which is what I think the original bug is describing. When the attached file, looses focus the application should no longer be able to delete the file by pressing DELETE.

That being said.
Here is a more clear explanation of the the overlapping accelerator issue mentioned above and which may be more appropriate as a new issue.

Expected behavior

| press DELETE with an entry selected, and a SINGLE prompt will show up asking if you want to delete x. Noramlly works this way and there are no issues. Unless...

The Issue

The issue is triggered when a contextMenu (in this case RightClickMenu) is generated with a right click. The menu creation process adds key shortcuts (via the accelerator) which are identical to the main menu accelerator shortcuts. Now when I press DELETE TWO events are fired, one from each menu. If I have two entries and press it it will ask me twice and potentially delete both of them.

Potential solutions, but someone may have a better idea.

• A filter in the menuItem event handler that takes context into account. If triggered from ContextMenu MenuItem's accelerator, then ignore the event. (I tried a simple version to confirm it can be fixed with a filter and would be happy to share)
• Remove the Accelerator for the context menus (doesn't help with deleting the attached file)
• Change the accelerator key combinations (confusing user experience)

Link contains an illustration of the issue.
https://stackoverflow.com/questions/34941706/share-menu-item-in-system-menu-and-contextmenu-with-accelerator

[BenArou]

Thank you for the pointers, I'll see what I can do and keep you updated on whether I need some help!

[HoussemNasri]

Hi @BenArou, I need to know if you're still working on this before I take a look into it.

[HoussemNasri]

Did a little digging into this issue and it seems this is not the only one related to keybinding. for example when you first open the app and type Ctrl+R, the Find and Replace dialog will pop up but if you select an entry and use the same shortcut Ctrl+R on an attached file to rename it, after closing the entry editor or selecting another entry, suddenly the shortcut will stop responding or even worse it will open the rename dialog for the previously attached file even if we have closed the library, the reason for this weird behavior is how the accelerator API is implemented in JavaFX, all accelerators are being processed in the Scene so if you have two accelerators belonging to two different ContextMenu's that are using the same KeyCombination the one added to the scene last will be favored because it will override the one before it, so because the LinkedFilesEditor was created and added after the menu bar the scene will keep notifying the LinkedFilesEditor about the shortcut events even if it's not visible, this is only one of many and it all boils down to how accelerators work.

Solution

I had a look at how other open-source projects using JavaFX handle this, Scene Builder, for example, have only one Menu with accelerators, which is the one in the MenuBar, all other menus don't use accelerators, so all key events will be received by the menu bar and from there it will decide what action to perform based on the selected object or the FocusOwner, this requires that all-important actions be in the menu bar like DELETE, COPY, PASTE, RENAME... and REPLACE_ALL should have its own shortcut.

https://github.com/gluonhq/scenebuilder/blob/487965ead80921fedaf0d46722c3edc1b4cb1a5c/kit/src/main/java/com/oracle/javafx/scenebuilder/kit/editor/util/ContextMenuController.java#L343

https://github.com/gluonhq/scenebuilder/blob/487965ead80921fedaf0d46722c3edc1b4cb1a5c/app/src/main/java/com/oracle/javafx/scenebuilder/app/menubar/MenuBarController.java#L717

[AvaniDhaliwal]

Hello, is this issue still valid? If so, we are a group of university students who would like to attempt to solve this. Thanks!

[idadoudou]

Please use ***@***.*** due that it is difficult to login qq email.

[AvaniDhaliwal]

I'm sorry, I'm not sure what you mean. Could you please rephrase.

[koppor]

@AvaniDhaliwal I think @idadoudou should reconfigure its email adress stored on GitHub. Currently, they receives notifications from this issue.

[koppor]

@AvaniDhaliwal Please thoroughly read what is written at #7392 (comment).

[HoussemNasri]

@AvaniDhaliwal I haven't looked into this issue in a long time. It's definitely a difficult one, and it's not a good first issue. To solve this for good, one needs to make many changes across the codebase and a change to the entire keybinding architecture; thus, a good understanding of the codebase is required. However, as @koppor stated, you are welcome to read the comment above to better understand the problem and look for workarounds.