Don't register any database changes to the indexer while dropping a file by btut · Pull Request #8334 · JabRef/jabref

btut · 2021-12-13T14:53:05Z

There was a problem that involved the Indexer and Drag&Drop operations while having auto-file-move/rename enabled.

When dropping a File onto an entry, JabRef links that file to the entry and moves and renames the files according to the set preferences. It was possible for this to happen while the PDFIndexer was running. This could lead to:

The pdf indexer not finding the file because it was invoked before the path changed
The move/rename operation to fail because the indexer blocked the PDF

This was solved by not allowing for any files to be added to the indexer during drag&drop operations.

Fixes #8182.

Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked 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 to the documentation repository.

tobiasdiez · 2021-12-13T18:30:41Z

+                            }
+                            case MOVE -> {
+                                LOGGER.debug("Mode MOVE"); // alt on win
+                                libraryTab.getIndexingTaskManager().setBlockingNewTasks(true);


Is there a reason why this blocking shouldn't be added directly to the importHandler (or linker)?

Not all users of importHandler have access to the IndexingTaskManager unfortunately.

Mhh, do you have an example where this occurs? The whole linker/import handler stuff arose from the fact that there were many very similarly-looking code blocks scattered around the code base, that all handled imports in a similar but same way. Was a huge headache, so I would like not to introduce special handling in certain situations again.

Oh, that sounds like a good reason to do it directly in the import handler.
The PreviewPanel was my concern, but I passed it along and made it work.

tobiasdiez · 2021-12-13T18:33:05Z

+                                LOGGER.debug("Mode MOVE"); // alt on win
+                                libraryTab.getIndexingTaskManager().setBlockingNewTasks(true);
+                                importHandler.getLinker().moveFilesToFileDirAndAddToEntry(entry, files);
+                                libraryTab.getIndexingTaskManager().setBlockingNewTasks(false);


Instead of this setBlockingNewTasks(true/false) pattern, I would suggest to use the try resource solution: https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html, i.e.

try (libraryTab.getIndexingTaskManager().blockingNewTasks()) { importHandler.getLinker().moveFilesToFileDirAndAddToEntry(entry, files); }

In this way, you make sure that the setBlockingNewTasks(false) is always executed, even in case there is a (runtime) exception.

So if I understand that right I would need to make the blocking() method of IndexingTaskManager return a 'closable', right? I could define that as a lambda that resets the flag.
Or is there a better way you are hinting at?

That sounds good!

Tried it and it was easy enough - so I think I got what you meant. Thanks for the hint!

tobiasdiez

Looks good to me, but I didn't deep look so would prefer if someone else also looks over it.

Thanks for your continued work on making the indexing better and better!

calixtus

Two comments.

calixtus · 2021-12-14T18:18:04Z

                    case COPY -> {
                        LOGGER.debug("Mode COPY");
-                        fileLinker.copyFilesToFileDirAndAddToEntry(entry, files);
+                        fileLinker.copyFilesToFileDirAndAddToEntry(entry, files, libraryTab.getIndexingTaskManager());


Can you somehow inverse the dependency on libraryTab here (maybe injecting it in the constructor) ?

calixtus · 2021-12-14T18:20:06Z

                        case MOVE -> {
                            LOGGER.debug("Mode MOVE"); // alt on win
-                            importHandler.getLinker().moveFilesToFileDirAndAddToEntry(entry, files);
+                            importHandler.getLinker().moveFilesToFileDirAndAddToEntry(entry, files, libraryTab.getIndexingTaskManager());


IoC here too?

Having the IndexingTaskManager in ImportHandler would require a lot of refactoring and passing along the IndexingTaskManager in many files. I would really like to avoid that.

Ok, I agree, it's probably better to fix the bug now.

But sooner or later we have to deal with that. 😅

btut · 2021-12-15T09:54:31Z

Great! Before merging I would like to wait for @koppor to confirm the issue is fixed for him as well (as he is on windows).

Siedlerchr · 2021-12-17T19:40:09Z

I'm going to merge this now, as it's good to have it in the release

* upstream/main: Don't register any database changes to the indexer while dropping a file (#8334) Fix ACM fetcher (#8338) Squashed 'buildres/csl/csl-styles/' changes from 3bb4b5f..60bf7d5 Exception shouldn't happen when pasting an entry with a publication date-range of form yyyy-yyyy (#8247) Refactor Sidepane logic (#8202) New translations JabRef_en.properties (Japanese) (#8331) Bump bcprov-jdk15on from 1.69 to 1.70 (#8333) Update Controlsfx to 11.1.1 (#8330) Update citeproc (#8329) Bump classgraph from 4.8.137 to 4.8.138 (#8327) Bump junit-platform-launcher from 1.8.1 to 1.8.2 (#8326) Remove outdated gradle deps check (#8324) Bump junit-jupiter from 5.8.1 to 5.8.2 (#8325) Bump libreoffice from 7.2.2 to 7.2.3 (#8328) Remove outdated ignores

btut · 2021-12-17T20:51:07Z

Certainly won't hurt. If @koppor still has an isse we can open a new Issue.

* upstream/main: (46 commits) New Crowdin updates (#8349) Bump pdfbox from 2.0.24 to 2.0.25 (#8345) Bump fontbox from 2.0.24 to 2.0.25 (#8348) Bump xmlunit-core from 2.8.3 to 2.8.4 (#8347) Bump tika-core from 2.1.0 to 2.2.0 (#8346) Added missing executable bindings to various commands (#8342) Update Gradle Wrapper from 7.3.1 to 7.3.2. (#8343) Fix issues with writing metadata to pdfs (#8332) add tinylog test (#8339) Tinylog (#8226) Don't register any database changes to the indexer while dropping a file (#8334) Fix ACM fetcher (#8338) Squashed 'buildres/csl/csl-styles/' changes from 3bb4b5f..60bf7d5 Exception shouldn't happen when pasting an entry with a publication date-range of form yyyy-yyyy (#8247) Refactor Sidepane logic (#8202) New translations JabRef_en.properties (Japanese) (#8331) Bump bcprov-jdk15on from 1.69 to 1.70 (#8333) Update Controlsfx to 11.1.1 (#8330) Update citeproc (#8329) Bump classgraph from 4.8.137 to 4.8.138 (#8327) ... # Conflicts: # build.gradle

Don't register any database changes to the indexer while dropping a file

dfc912d

btut requested a review from koppor December 13, 2021 14:53

Changelog

1c312ae

tobiasdiez reviewed Dec 13, 2021

View reviewed changes

btut mentioned this pull request Dec 14, 2021

PDF indexing should be paused when adding a file via drag and drop #8182

Closed

2 tasks

btut added 3 commits December 14, 2021 10:03

Merge branch 'main' into fixFileaccessDragAndDropIndexing

9f85d37

Use try-with-resources construct

6596726

Moved blocking to ExternalFilesEntryLinker

b905428

tobiasdiez approved these changes Dec 14, 2021

View reviewed changes

calixtus requested changes Dec 14, 2021

View reviewed changes

calixtus approved these changes Dec 15, 2021

View reviewed changes

Siedlerchr approved these changes Dec 17, 2021

View reviewed changes

Siedlerchr merged commit 144a37d into main Dec 17, 2021

Siedlerchr deleted the fixFileaccessDragAndDropIndexing branch December 17, 2021 19:40

InAnYan mentioned this pull request Jul 6, 2024

AI chatting functionality #11430

Merged

10 tasks

Uh oh!

Conversation

btut commented Dec 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

calixtus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

btut commented Dec 15, 2021

Uh oh!

Siedlerchr commented Dec 17, 2021

Uh oh!

btut commented Dec 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants