Skip to content

Adds a dynamic group mirroring the file system (#10930)#14252

Closed
ErwanGou wants to merge 78 commits into
JabRef:mainfrom
Julien384760:fix-issue-10930
Closed

Adds a dynamic group mirroring the file system (#10930)#14252
ErwanGou wants to merge 78 commits into
JabRef:mainfrom
Julien384760:fix-issue-10930

Conversation

@ErwanGou

@ErwanGou ErwanGou commented Nov 7, 2025

Copy link
Copy Markdown

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() and sortReverseAlphabeticallyRecursive() in GroupTreeViewModel.java is a bit weird :

  • It is recursive but never checks if the subgroup is sortable so we can actually sort Directory Groups if we sort the AllEntries node.
  • Furthermore I didn't get why it is mandatory that 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 :

image

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 :

image

Here you can select the root folder of the structure you want to mirror by clicking on the folder icon button :

image

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 :

image

Then click on the 'OK' button, your local structure should appear :
image

The entries created with the local PDFs should appear a little while later :

image

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

ErwanGou and others added 25 commits November 3, 2025 15:16
…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
@github-actions

github-actions Bot commented Nov 7, 2025

Copy link
Copy Markdown
Contributor

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.

@koppor koppor added the status: waiting-for-feedback The submitter or other users need to provide more information about the issue label Nov 25, 2025
@koppor

koppor commented Nov 25, 2025

Copy link
Copy Markdown
Member
  • It should not be possible to remove subgroups
    [...]
    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

I don't understand how NOT remove relates to "remove subgroups" issue.

Do you say you want to implement subgroup removal?

@ErwanGou

Copy link
Copy Markdown
Author

I don't understand how NOT remove relates to "remove subgroups" issue.

Do you say you want to implement subgroup removal?

Sorry, my explanation wasn't clear.

In fact the subgroup removal is already implemented and it works properly because of the cleanupListenersAccordingToJabRefChanges() method in DefaultDirectoryUpdateMonitor. It stops monitoring local directories which are no longer mirrored in JabRef. I did it because I wanted the user to be able to edit a DirectoryGroup, mostly to change the group's color, icon or description, and I realised that they could also change the group's type --which is allowed even if the group is not removable even though it is basically a deletion--.

However, I agree that it would be better if the mirrored group structure in JabRef were immutable, and it lacks few things :

  • Remove group must be disabled for a DirectoryGroup, except for the root of the structure --all or nothing--. This point is easy and has already been done but not commited.
  • Regarding Remove subgroups however --and it's more of an implementation choice than a problem --, it never checks if all the subgroups are really removable, so this feature would still be allowed on the root of the structure. Since the parent group can be deleted it has never been an issue to remove the subgroups and keep the parent, but here it would leave the root of the structure empty. To fix that I can either rewrite the recursive method to delete removable subgroups only, or modify this part of the code :
case GROUP_SUBGROUP_REMOVE ->
          group.isEditable() && group.hasSubgroups() && group.canRemove()
               || group.isRoot();

I could add the test && !(group.getGroupNode().getGroup() instanceof DirectoryGroup) --but it breaks the consistency-- or change group.canRemove() to group.canRemoveSubgroups and add this method --sounds to be the best option to me--. Since it impacts an already existing feature I would like your opinion about that.

  • Finally there is the issue with the group editor. Maybe we should find a way to allow minor changes only for certain types of group.

@koppor koppor added the status: changes-required Pull requests that are not yet complete label Nov 28, 2025

@koppor koppor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems not to work with a library already having files attached. Maybe try with https://github.com/JabRef/jabref-demonstration-libraries/blob/main/chocolate/Chocolate.bib

Absolute paths should not be required. The directory should be resolved .

See https://devdocs.jabref.org/code-howtos/#get-absolute-filename-or-path-for-file-in-file-directory and also the library properties - the button on the right side.

grafik

private void notifyAboutDirectoryCreation(Path newPath) {
Path parentPath = newPath.toAbsolutePath().getParent();
for (DirectoryUpdateListener listener : listeners.get(parentPath)) {
if (listener instanceof DirectoryGroup parentGroup) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code like this should not be there, because the UpdateMonitor should be agnostic of the listeners -- the listener should handle this.

Also importFilesInDatabase should go to a listener.

Comment on lines +149 to +151
dialogService.notify(Localization.lang("Failed to import %0 PDF.", nbPDFs));
} else {
dialogService.notify(Localization.lang("Failed to import %0 PDFs.", nbPDFs));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong localizations. See "Pluralizations" at https://devdocs.jabref.org/code-howtos/localization.html#general-hints

I think, you were not aware of https://phrase.com/blog/posts/pluralization/


private final Path absoluteDirectoryPath;
private final DirectoryUpdateMonitor directoryUpdateMonitor;
private BibDatabaseContext database;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use context as variable name or the full name.

database is wrong, because a database is "nested" in a databaseContext.

entry.setFiles(cleanedFiles);
}

private void cleanupFilesOfAllEntries() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is wrong here - it matches entries of the complete library - and not just of some directory.

private void cleanupFilesOfAnEntry(BibEntry entry) {
List<LinkedFile> cleanedFiles = new ArrayList<>();
for (LinkedFile file : entry.getFiles()) {
if (Files.exists(Path.of(file.getLink()).toAbsolutePath())) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might work for DirectoryGroups as imlemented for now - but it won't work if relative paths are used.

return group;
}

// without DirectoryUpdateMonitor

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

@github-actions github-actions Bot added status: changes-required Pull requests that are not yet complete and removed status: changes-required Pull requests that are not yet complete labels Nov 30, 2025
@github-actions

github-actions Bot commented Dec 1, 2025

Copy link
Copy Markdown
Contributor

Your pull request conflicts with the target branch.

Please merge with your code.
For a step-by-step guide to resolve merge conflicts, see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-using-the-command-line.

@koppor

koppor commented Dec 1, 2025

Copy link
Copy Markdown
Member

Remove group must be disabled for a DirectoryGroup, except for the root of the structure --all or nothing--. This point is easy and has already been done but not commited.

Maybe, its time for two Group objects: DirectoryGroup (containing the start directory) and SubDirectoryGroup

it never checks if all the subgroups are really removable

With two different group objects, this should be easily solved.

@koppor koppor removed the status: waiting-for-feedback The submitter or other users need to provide more information about the issue label Dec 1, 2025
@ErwanGou

ErwanGou commented Dec 2, 2025

Copy link
Copy Markdown
Author

Thank you for all your reviews, sadly I'm very busy right now --it is the end of the semester-- so I'll be less active for the next few weeks. I'll be happy to continue working on this project but probably in January. I see that I still have many things to change so I'll convert this PR to draft.

@ErwanGou ErwanGou marked this pull request as draft December 2, 2025 09:43
@koppor

koppor commented Dec 4, 2025

Copy link
Copy Markdown
Member

I found this in my drafts - maybe

  • Remove group must be disabled for a DirectoryGroup, except for the root of the structure --all or nothing--.

Yes!

This point is easy and has already been done but not commited.

Nice! Pay attention that I merged main and thus you need to pull before you can push.


Regarding Remove subgroups however --and it's more of an implementation choice than a problem --, it never checks if all the subgroups are really removable,

I don't get this. I just think of directory groups now, because we are focussing on this - and not other groups.

  • Case 1: Current group is not a directory group
  • Case 2: Current group is a directory group
  • Case 3: Current group is a subdirectory group

With these, it can be distinguished

@koppor

koppor commented Dec 4, 2025

Copy link
Copy Markdown
Member

Thank you for all your reviews, sadly I'm very busy right now --it is the end of the semester-- so I'll be less active for the next few weeks. I'll be happy to continue working on this project but probably in January.

Would be nice to read!

I see that I still have many things to change so I'll convert this PR to draft.

Thank you.

Note: The issue is a good third issue; Meaning it is harder than the "good first" and "good second" issues; and it is OK to take more time.

@koppor

koppor commented Dec 18, 2025

Copy link
Copy Markdown
Member

I think, we all lost context here. Seeing all the merge conflicts and the comments, its time to start from scratch. Therefore, I close this PR. I hope, everyone learned something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dynamic group mirroring the file system.

5 participants