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

[ErwanGou]

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 :

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

• [x] I own the copyright of the code submitted and I license it under the MIT license
• [x] I manually tested my changes in running JabRef (always required)
• [x] I added JUnit tests for changes (if applicable)
• [x] I added screenshots in the PR description (if change is visible to the user)
• [x] I described the change in CHANGELOG.md in a way that is understandable for the average user (if change is visible to the user)
• [x] I checked the user 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 updating file(s) in https://github.com/JabRef/user-documentation/tree/main/en.

[github-actions[bot]]

Hey @ErwanGou!

Thank you for contributing to JabRef! Your help is truly appreciated :heart:.

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.

[InAnYan]

Is this PR work in progress?

[ErwanGou]

Is this PR work in progress?

Not really. The recent commits are minor changes to try fixing the issues raised by the jabref bot, but I don't understand why it is still not working. I also regularly update my branch. Apart from that the feature lacks some improvements as detailled in the 'Next steps' section, but I need your help for those.

[InAnYan]

But why there are TODO comments?

[ErwanGou]

But why there are TODO comments?

The issue in its entirety is not resolved by the actual code that I propose so I detailled what is missing in the 'Next steps' section of the PR and to remember it I added some TODO comments that I can delete if you prefer. For now I'm a bit stuck on these missing features so I wanted some feedback about the existing code before continuing.

[InAnYan]

Ah, okay, you made this PR not draft, so we could give you feedback. Now I understand

[ErwanGou]

I recently added the last features and modified the PR message. Apart from what I explained in the Next steps section --which can't be fixed-- it should work properly. I won't commit new code anymore to this branch, you can review it freely now.

[InAnYan]

@ErwanGou can you fix merge conflicts in CHANGELOG.md? If you believe that no more commits is needed, just keep track on conflicts and our review

[ErwanGou]

@InAnYan I couldn't reproduce this error --maybe because I use Windows, I tried all the ways to add a PDF to an empty group and it never failed-- but thanks to the error message I could find where it came from and it should be fixed : the class GroupSerializer.java build a string representation of a group but it was not implemented for DirectoryGroup. However I just discovered that this kind of string is used to create a group from scraps in GroupsParser.java so a new error will probably show up. I will need to modify tons of constructors to make it work properly because I need a DirectoryUpdateMonitor and an entire BibDatabaseContext --not only the Metadata-- to initialize the new group type. I will do it shortly but I won't commit that until someone asks me to. I really think that there is no other choice but I am a bit reluctant to modify so many constructors.

[ErwanGou]

@InAnYan I think I fixed the issue with GroupsParser.java. As I said I had to modify many constructors so in total it impacted 87 files. It should work properly now though I can't test it because of an error during :jabref:run. This error was fixed recently in the main repository but I can't pull due to conflicts with two of the 87 files I modified : I need to commit first and then resolve the conflicts. I won't commit those changes until I get your approbation.

[InAnYan]

I will not be available during next 2-3 hours, so, if you have time fixing the conflicts now, you can do this before my aprobation.

Thanks for looking into the issue!

[ErwanGou]

In fact I managed to pull the most recent version of Jabref without committing my changes so I can now run :jabref:run. I tested the app and at first glance it works correctly so the files I modified shouldn't have brought new errors.

[ErwanGou]

@InAnYan I recently fixed a slight bug with the user interface. Since the PDF import is a background task, when a DirectoryGroup is created it first doesn't contain any entry, so after its creation it is automatically selected and no entry is shown. Now, after the background task has finished, the new group is selected again to display the entries. I also added a message for the user, I don't know if it is relevant. I still haven't commited the 87 files that fix your bug, I'm afraid that it will make your task more complicated, as there would be a lot of changes. Please, tell me when it's ok for you.

[InAnYan]

Error log:

Could not parse entry
java.io.IOException: Error in line 20: Expected { or ( but received B
	at org.jabref.jablib/org.jabref.logic.importer.fileformat.BibtexParser.consume(BibtexParser.java:1197)
	at org.jabref.jablib/org.jabref.logic.importer.fileformat.BibtexParser.parseEntry(BibtexParser.java:712)
	at org.jabref.jablib/org.jabref.logic.importer.fileformat.BibtexParser.parseAndAddEntry(BibtexParser.java:339)
	at org.jabref.jablib/org.jabref.logic.importer.fileformat.BibtexParser.parseFileContent(BibtexParser.java:265)
	at org.jabref.jablib/org.jabref.logic.importer.fileformat.BibtexParser.parse(BibtexParser.java:183)
	at org.jabref.jablib/org.jabref.logic.importer.fileformat.BibtexParser.parseEntries(BibtexParser.java:148)
	at org.jabref.jablib/org.jabref.logic.importer.Parser.parseEntries(Parser.java:18)
	at org.jabref.jablib/org.jabref.logic.importer.fileformat.pdf.PdfVerbatimBibtexImporter.importDatabase(PdfVerbatimBibtexImporter.java:34)
	at org.jabref.jablib/org.jabref.logic.importer.fileformat.pdf.PdfMergeMetadataImporter.extractCandidatesFromPdf(PdfMergeMetadataImporter.java:103)
	at org.jabref.jablib/org.jabref.logic.importer.fileformat.pdf.PdfMergeMetadataImporter.importDatabase(PdfMergeMetadataImporter.java:82)
	at org.jabref.jablib/org.jabref.logic.importer.fileformat.pdf.PdfImporter.importDatabase(PdfImporter.java:52)
	at org.jabref.jablib/org.jabref.logic.importer.fileformat.pdf.PdfMergeMetadataImporter.importDatabase(PdfMergeMetadataImporter.java:175)
	at org.jabref.jablib/org.jabref.logic.externalfiles.ExternalFilesContentImporter.importPDFContent(ExternalFilesContentImporter.java:24)
	at org.jabref/org.jabref.gui.externalfiles.ImportHandler$1.call(ImportHandler.java:147)
	at org.jabref/org.jabref.gui.externalfiles.ImportHandler$1.call(ImportHandler.java:109)
	at org.jabref/org.jabref.gui.util.UiTaskExecutor$1.call(UiTaskExecutor.java:188)
	at [email protected]/javafx.concurrent.Task$TaskCallable.call(Task.java:1407)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:328)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:545)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:328)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1095)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:619)
	at java.base/java.lang.Thread.run(Thread.java:1447)

[ErwanGou]

@InAnYan I couldn't reproduce your error but it should be fixed with commit f858e08. I tried to fix it without modifying so many files but now I really think there is no other option.

[InAnYan]

It seems that when I add a new entry (with or without file) it immediately deletes it

[ErwanGou]

@InAnYan

• In the file GroupNodeViewModel.java we can choose whether it is possible or not to add entries manually in a group of a given type. I didn't allow it for DirectoryGroup because I thought it was supposed to be only automatic but I can easily change that if you think it's more relevant.
• I still need to figure out why the error you sent had shown up.
• Regarding the import dialog -if you are talking about the dialog that asks if you want to merge entries or not-, well it is not on my purpose : I use an ImportHandler to import the PDFs and it displays that each time a single file is imported. That means if you create a DirectoryGroup from a local directory structure that contains only 1 PDF the dialog will be displayed, but if there are multiple PDFs it won't. While monitoring the file system, the WatchService raise an event for each new file, so even if you paste or move multiple PDFs at the same time the dialog will show up as many times as there are PDFs. I don't know why it was implemented like that but it seems to be the best way to import files.

[koppor]

Then, I can't add any entry to the directory group or its subgroups.

Which is OK, isn't it?

Requirement was

Currently: Only inbound mirroring. Use WatchService to watch for changes.

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)

There should be no popup at all. JabRef should import it using "best guess". We have org.jabref.logic.bibtex.comparator.FieldValuePlausibilityComparator. There could be more. Update See https://github.com/JabRef/jabref/pull/14396 for the infrastructure.

[koppor]

• I use an ImportHandler to import the PDFs and it displays that each time a single file is impored.

Can this be modified using https://github.com/JabRef/jabref/pull/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.

[koppor]

• [ ] It should not be possible to remove subgroups
  
  
• [ ] It should be possible to sort A-Z and Z-A
  

[koppor]

Error log:

Could not parse entry
java.io.IOException: Error in line 20: Expected { or ( but received B
...
	at org.jabref.jablib/org.jabref.logic.importer.Parser.parseEntries(Parser.java:18)
	at org.jabref.jablib/org.jabref.logic.importer.fileformat.pdf.PdfVerbatimBibtexImporter.importDatabase(PdfVerbatimBibtexImporter.java:34)
	at org.jabref.jablib/

... org.jabref.logic.importer.fileformat.pdf.PdfMergeMetadataImporter.importDatabase(PdfMergeMetadataImporter.java:175)

at org.jabref.jablib/org.jabref.logic.externalfiles.ExternalFilesContentImporter.importPDFContent(ExternalFilesContentImporter.java:24)

I think, its an issue at PdfVerbatimBibtexImporter -- we should investigate your PDFs in a seperate issue (or private issue tracker)

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.

[koppor]

@ErwanGou Please do not reference the issue in your commits. Since you push to the branch Julien384760:fix-issue-10930 and you have this PR, it already correlates to the PR.

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:

[koppor]

There is an issue with the watcher on Windows : the user can't rename, move or delete a local directory which contains a folder that is not empty because it is detected as opened in an application.

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())));

[koppor]

I think, : should be escaped when writing and unescaped when reading

I did NOT get the exception when closing and opening the library in JabRef. But I got it when closing JabRef and re-opening JabRef again. -- OK, at another branch... -- But we need to be backwards compatible. That means, version 5.15 of JabRef should be able to open the library without any exception.

[ErwanGou]

• It should not be possible to remove subgroups
• It should be possible to sort A-Z and Z-A

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 :

case GROUP_SUBGROUP_REMOVE ->
          group.isEditable() && group.hasSubgroups() && group.canRemove()
               || group.isRoot();
case GROUP_SUBGROUP_SORT,
     GROUP_SUBGROUP_SORT_REVERSE ->
          group.isEditable() && group.hasSubgroups() && group.canAddEntriesIn()
               || group.isRoot();

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. Regarding the sort A-Z/ Z-A there is the same problem : it is recursive but never checks if it is allowed to sort the children --that's why we can currently sort DirectoryGroups using sort A-Z on the All entries node--. Moreover the sorting method never adds entries to the given group so I don't get why it is mandatory to have group.canAddEntriesIn() there. If we remove this test we could sort DirectoryGroups without any problem.

---

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?

I tried that and it still locks the file directory with a dummy import. I am pretty sure it comes from that line :

directory.register(watcher, StandardWatchEventKinds.ENTRY_CREATE, StandardWatchEventKinds.ENTRY_DELETE);

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.

---

But we need to be backwards compatible. That means, version 5.15 of JabRef should be able to open the library without any exception.

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 ? It made me think about one last thing : what is the expected behaviour if the user mirrors its local directories, save the library, closes JabRef, modify its local directories and opens the old library ? Currently it displays the old version of the structure with directories that possibly don't exist anymore --or even worse, a root that was deleted--, which could lead to bugs. Do we recreate the mirrored structure from the root --if it still exists--, even if it could also duplicate the entries depending on how it is done ? Or do we store DirectoryGroups as ExplicitGroups to avoid conflicts and keep the stored library untouched even if that means to stop adapting to local changes ?

[koppor]

• 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]

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.

[github-actions[bot]]

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]

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.

[ErwanGou]

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.