-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Adds a dynamic group mirroring the file system (#10930) #14252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…sting group sibling (JabRef#10930)
…group or edit its type (JabRef#10930)
…irectory was suppressed (JabRef#10930)
…x-issue-10930 # Conflicts: # jabgui/src/main/java/org/jabref/gui/groups/GroupDialogView.java # jabgui/src/main/java/org/jabref/gui/groups/GroupDialogViewModel.java # jabgui/src/main/java/org/jabref/gui/groups/GroupNodeViewModel.java # jabgui/src/main/resources/org/jabref/gui/groups/GroupDialog.fxml
Hey @ErwanGou!Thank you for contributing to JabRef! Your help is truly appreciated ❤️. We have automatic checks in place, based on which you will soon get automated feedback if any of them are failing. We also use TragBot with custom rules that scans your changes and provides some preliminary comments, before a maintainer takes a look. TragBot is still learning, and may not always be accurate. In the "Files changed" tab, you can go through its comments and just click on "Resolve conversation" if you are sure that it is incorrect, or comment on the conversation if you are doubtful. Please re-check our contribution guide in case of any other doubts related to our contribution workflow. |
|
Error log: |
InAnYan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I got this exception, but I'm not sure it's because of the PR:
java.lang.NullPointerException: Cannot invoke "javafx.collections.ListChangeListener.onChanged(javafx.collections.ListChangeListener$Change)" because "<local4>[<local6>]" is null
at javafx.base@25.0.1/com.sun.javafx.collections.ListListenerHelper$Generic.fireValueChangedEvent(ListListenerHelper.java:327)
at javafx.base@25.0.1/com.sun.javafx.collections.ListListenerHelper.fireValueChangedEvent(ListListenerHelper.java:71)
at javafx.base@25.0.1/javafx.collections.ObservableListBase.fireChange(ObservableListBase.java:246)
at javafx.base@25.0.1/javafx.collections.ListChangeBuilder.commit(ListChangeBuilder.java:482)
at javafx.base@25.0.1/javafx.collections.ListChangeBuilder.endChange(ListChangeBuilder.java:541)
at javafx.base@25.0.1/javafx.collections.ObservableListBase.endChange(ObservableListBase.java:210)
at javafx.base@25.0.1/javafx.collections.ModifiableObservableListBase.add(ModifiableObservableListBase.java:200)
at org.jabref.jablib/org.jabref.model.TreeNode.addChild(TreeNode.java:465)
at org.jabref.jablib/org.jabref.model.TreeNode.addChild(TreeNode.java:447)
at org.jabref.jablib/org.jabref.model.groups.GroupTreeNode.addSubgroup(GroupTreeNode.java:226)
at org.jabref/org.jabref.gui.groups.GroupNodeViewModel.addSubgroup(GroupNodeViewModel.java:329)
at org.jabref/org.jabref.gui.groups.GroupTreeViewModel.lambda$addNewSubgroup$1(GroupTreeViewModel.java:237)
at java.base/java.util.Optional.ifPresent(Optional.java:178)
at org.jabref/org.jabref.gui.groups.GroupTreeViewModel.lambda$addNewSubgroup$0(GroupTreeViewModel.java:236)
at java.base/java.util.Optional.ifPresent(Optional.java:178)
at org.jabref/org.jabref.gui.groups.GroupTreeViewModel.addNewSubgroup(GroupTreeViewModel.java:229)
at org.jabref/org.jabref.gui.groups.GroupTreeView$ContextAction.execute(GroupTreeView.java:761)
at org.jabref/org.jabref.gui.actions.JabRefAction.lambda$new$1(JabRefAction.java:25)
at org.controlsfx.controls@11.2.2/org.controlsfx.control.action.Action.handle(Action.java:423)
at org.controlsfx.controls@11.2.2/org.controlsfx.control.action.Action.handle(Action.java:64)
at javafx.base@25.0.1/com.sun.javafx.event.CompositeEventHandler.dispatchBubblingEvent(CompositeEventHandler.java:86)
at javafx.base@25.0.1/com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:232)
at javafx.base@25.0.1/com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:189)
at javafx.base@25.0.1/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:58)
at javafx.base@25.0.1/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
at javafx.base@25.0.1/com.sun.javafx.event.EventUtil.fireEventImpl(EventUtil.java:74)
at javafx.base@25.0.1/com.sun.javafx.event.EventUtil.fireEvent(EventUtil.java:49)
at javafx.base@25.0.1/javafx.event.Event.fireEvent(Event.java:199)
at javafx.controls@25.0.1/javafx.scene.control.MenuItem.fire(MenuItem.java:459)
at javafx.controls@25.0.1/com.sun.javafx.scene.control.ContextMenuContent$MenuItemContainer.doSelect(ContextMenuContent.java:1426)
at javafx.controls@25.0.1/com.sun.javafx.scene.control.ContextMenuContent$MenuItemContainer.lambda$createChildren$6(ContextMenuContent.java:1379)
at javafx.base@25.0.1/com.sun.javafx.event.CompositeEventHandler$NormalEventHandlerRecord.handleBubblingEvent(CompositeEventHandler.java:247)
at javafx.base@25.0.1/com.sun.javafx.event.CompositeEventHandler.dispatchBubblingEvent(CompositeEventHandler.java:80)
at javafx.base@25.0.1/com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:232)
at javafx.base@25.0.1/com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:189)
at javafx.base@25.0.1/com.sun.javafx.event.CompositeEventDispatcher.dispatchBubblingEvent(CompositeEventDispatcher.java:59)
at javafx.base@25.0.1/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:58)
at javafx.base@25.0.1/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
at javafx.base@25.0.1/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
at javafx.base@25.0.1/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
at javafx.base@25.0.1/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
at javafx.base@25.0.1/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
at javafx.base@25.0.1/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
at javafx.base@25.0.1/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
at javafx.base@25.0.1/com.sun.javafx.event.EventUtil.fireEventImpl(EventUtil.java:74)
at javafx.base@25.0.1/com.sun.javafx.event.EventUtil.fireEvent(EventUtil.java:54)
at javafx.base@25.0.1/javafx.event.Event.fireEvent(Event.java:199)
at javafx.graphics@25.0.1/javafx.scene.Scene$MouseHandler.process(Scene.java:4061)
at javafx.graphics@25.0.1/javafx.scene.Scene.processMouseEvent(Scene.java:1947)
at javafx.graphics@25.0.1/javafx.scene.Scene$ScenePeerListener.mouseEvent(Scene.java:2784)
at javafx.graphics@25.0.1/com.sun.javafx.tk.quantum.GlassViewEventHandler$MouseEventNotification.get(GlassViewEventHandler.java:353)
at javafx.graphics@25.0.1/com.sun.javafx.tk.quantum.GlassViewEventHandler$MouseEventNotification.get(GlassViewEventHandler.java:255)
at javafx.graphics@25.0.1/com.sun.javafx.tk.quantum.QuantumToolkit.runWithoutRenderLock(QuantumToolkit.java:424)
at javafx.graphics@25.0.1/com.sun.javafx.tk.quantum.GlassViewEventHandler.handleMouseEvent(GlassViewEventHandler.java:387)
at javafx.graphics@25.0.1/com.sun.glass.ui.View.handleMouseEvent(View.java:573)
at javafx.graphics@25.0.1/com.sun.glass.ui.View.notifyMouse(View.java:975)
at javafx.graphics@25.0.1/com.sun.glass.ui.gtk.GtkApplication._runLoop(Native Method)
at javafx.graphics@25.0.1/com.sun.glass.ui.gtk.GtkApplication.lambda$runLoop$0(GtkApplication.java:240)
at java.base/java.lang.Thread.run(Thread.java:1447)
Then, I can't add any entry to the directory group or its subgroups.
But I confirm that when I create it, it properly determines the files. And when I add a file in the directory group via file managers, JabRef really captures it (but... I got the import dialog like 4 times, IDK why)
|
It seems that when I add a new entry (with or without file) it immediately deletes it |
|
Which is OK, isn't it? Requirement was
There should be no popup at all. JabRef should import it using "best guess". We have |
Can this be modified using #14396? I currently get
Maybe, this involves "hiearchy" of PDF sources in case there is no comparator. Implement a list and we can re-order during code review. |
...
I think, its an issue at Maybe, the log level at this point is too high -- the PdfVerbatimBibtexImporter could be simply be called on all PDFs and the exception should just be ignored than being output. |
|
@ErwanGou Please do not reference the issue in your commits. Since you push to the branch When mentioning the issue in the commit, all of these appear in the timeline of the issue, which is kind of strange, because all commits together solve the issue, not each single commit:
|
I think, the importers need to be checked if they add a file lock. Can you test if this issue persists if you do a "dummy import" of the PDF files? - Meaning: not calling any importer, but creating a simple BibEntry manually? BibEntry entry = new BibEntry()
.withFiles(List.of(new LinkedFile("", "example.pdf", StandardFileType.PDF.getName()))); |
I can make it so that only the root of the mirrored directory structure can be removed but there would still be a problem with remove subgroups and sort A-Z/ Z-A. The existing implementation is as follow : That means we can remove the subgroups of a group that can be removed and it never checks if the given subgroups are really deletable, so even if only the root is deletable the user will still be allowed to use this feature.
I tried that and it still locks the file directory with a dummy import. I am pretty sure it comes from that line : When a directory is registered the system automatically locks it. I don't know if there is a solution because I've never seen an application able to pass through this issue.
Do you mean 5.15 is supposed to be able to open libraries that contain DirectoryGroups ? Unknown groups throw an error so is it even possible ? |
I don't understand how NOT remove relates to "remove subgroups" issue. Do you say you want to implement subgroup removal? |






Closes #10930
Changed or Added files
The class DirectoryGroup.java contains the new type of group. There are some tests in DirectoryGroupTest.java but most of the feature cannot be tested because it needs the WatchService.
The changes in GroupNodeViewModel.java allow directory groups to be edited or removed. The root node of the mirrored structure can be dragged.
The changes in GroupDialog.fxml, GroupDialogView.java and GroupDialogViewModel.java add the button to mirror the user's local structure within JabRef. The GroupDialogViewModelTest.java was updated to fit the new constructor and to test the new feature.
The new version of JabRef_en.properties contains the English text displayed to the user when trying to mirror its local structure.
The files DirectoryUpdateListener.java, DirectoryUpdateMonitor.java, DummyDirectoryUpdateMonitor.java and DefaultDirectoryUpdateMonitor.java implement a watcher for directories. The file JabRefGUI.java is now able to initialize this watcher.
The file GroupTreeViewModel.java can now create a directory structure. It needed new attributes in the constructor so GroupTreeViewModelTest.java has been updated.
The changes in GroupTreeView.java correct a bug : the user could use "Sort subgroups Z-A" on groups that are not sortable.
In the file CHANGELOG.md there is the description of what was changed in the code.
There are many other changes to create a DirectoryGroup from a string representation.
Next steps
There is an issue with the watcher on Windows : the user can't rename, move or delete a local directory that is not empty because it is detected as opened in an application.
I chose not to allow the user to add entries in a directory group because it is supposed to work only automatically, but due to this choice they are not sortable. About this point I think the existing implementation of
sortAlphabeticallyRecursive()andsortReverseAlphabeticallyRecursive()in GroupTreeViewModel.java is a bit weird :group.canAddEntriesIn()should be true because it seems the sorted method never uses that.Steps to test
Start JabRef and open a library, empty or not :
You can add a new group by clicking on the 'Add group' button. A dialog should show up. Select 'Directory structure' under the 'Collect by' title :
Here you can select the root folder of the structure you want to mirror by clicking on the folder icon button :
Once the root is set it should automatically fill the 'Root path'. It also fills the 'Name' of the group if and only if it was empty and set the hierarchical context to 'Union', which means a group contains the entries of its subgroups -it is not mandatory to use that but it is recommended because it is how local folders work. You can also write or paste the root path instead of selecting the folder but remember that the 'OK' button won't be active until a valid path and a valid name are provided. You can add a description, an icon, a color or even change the hierarchical context if you want :
Then click on the 'OK' button, your local structure should appear :

The entries created with the local PDFs should appear a little while later :
Now feel free to test the feature. You can either add, delete, rename or move PDFs or folders in your local directory structure, you should see the consequences on your library groups. You can also edit those groups with a right-click. Remember that if you remove a group or change its type it won't adapt to your local changes anymore. If you create an entry that has a file within the mirrored structure it should automatically be added to the respective group. The feature also works if you have multiple opened libraries.
Mandatory checks