Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 5 commits into from
Dec 17, 2021
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dfc912d
Don't register any database changes to the indexer while dropping a file
btut Dec 13, 2021
1c312ae
Changelog
btut Dec 13, 2021
9f85d37
Merge branch 'main' into fixFileaccessDragAndDropIndexing
btut Dec 14, 2021
6596726
Use try-with-resources construct
btut Dec 14, 2021
b905428
Moved blocking to ExternalFilesEntryLinker
btut Dec 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
- We fixed an issue where an exception would occur when clicking on a DOI link in the preview pane [#7706](https://github.com/JabRef/jabref/issues/7706)
- We fixed an issue where XMP and embedded BibTeX export would not work [#8278](https://github.com/JabRef/jabref/issues/8278)
- We fixed an issue where the XMP and embedded BibTeX import of a file containing multiple schemas failed [#8278](https://github.com/JabRef/jabref/issues/8278)
- We fixed an issue where pdf-paths and the pdf-indexer could get out of sync [#8182](https://github.com/JabRef/jabref/issues/8182)

### Removed

Expand Down
42 changes: 29 additions & 13 deletions src/main/java/org/jabref/gui/maintable/MainTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,13 @@
import org.jabref.gui.util.ViewModelTableRowFactory;
import org.jabref.logic.importer.ImportCleanup;
import org.jabref.logic.l10n.Localization;
import org.jabref.logic.pdf.search.indexing.PdfIndexer;
import org.jabref.logic.util.OS;
import org.jabref.model.database.BibDatabaseContext;
import org.jabref.model.database.BibDatabaseMode;
import org.jabref.model.database.event.EntriesAddedEvent;
import org.jabref.model.entry.BibEntry;
import org.jabref.preferences.FilePreferences;
import org.jabref.preferences.PreferencesService;

import com.google.common.eventbus.Subscribe;
Expand All @@ -69,6 +71,8 @@ public class MainTable extends TableView<BibEntryTableViewModel> {
private long lastKeyPressTime;
private String columnSearchTerm;

private final FilePreferences filePreferences;

public MainTable(MainTableDataModel model,
LibraryTab libraryTab,
BibDatabaseContext database,
Expand Down Expand Up @@ -178,6 +182,8 @@ public MainTable(MainTableDataModel model,
this.jumpToSearchKey(getSortOrder().get(0), key);
});

filePreferences = preferencesService.getFilePreferences();

database.getDatabase().registerListener(this);
}

Expand Down Expand Up @@ -375,20 +381,30 @@ private void handleOnDragDropped(TableRow<BibEntryTableViewModel> row, BibEntryT
switch (ControlHelper.getDroppingMouseLocation(row, event)) {
case TOP, BOTTOM -> importHandler.importFilesInBackground(files).executeWith(Globals.TASK_EXECUTOR);
case CENTER -> {
BibEntry entry = target.getEntry();
switch (event.getTransferMode()) {
case LINK -> {
LOGGER.debug("Mode LINK"); // shift on win or no modifier
importHandler.getLinker().addFilesToEntry(entry, files);
}
case MOVE -> {
LOGGER.debug("Mode MOVE"); // alt on win
importHandler.getLinker().moveFilesToFileDirAndAddToEntry(entry, files);
}
case COPY -> {
LOGGER.debug("Mode Copy"); // ctrl on win
importHandler.getLinker().copyFilesToFileDirAndAddToEntry(entry, files);
try {
BibEntry entry = target.getEntry();
switch (event.getTransferMode()) {
case LINK -> {
LOGGER.debug("Mode LINK"); // shift on win or no modifier
importHandler.getLinker().addFilesToEntry(entry, files);
}
case MOVE -> {
LOGGER.debug("Mode MOVE"); // alt on win
libraryTab.getIndexingTaskManager().setBlockingNewTasks(true);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

importHandler.getLinker().moveFilesToFileDirAndAddToEntry(entry, files);
libraryTab.getIndexingTaskManager().setBlockingNewTasks(false);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

That sounds good!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

libraryTab.getIndexingTaskManager().addToIndex(PdfIndexer.of(database, filePreferences), entry, database);
}
case COPY -> {
LOGGER.debug("Mode Copy"); // ctrl on win
libraryTab.getIndexingTaskManager().setBlockingNewTasks(true);
importHandler.getLinker().copyFilesToFileDirAndAddToEntry(entry, files);
libraryTab.getIndexingTaskManager().setBlockingNewTasks(false);
libraryTab.getIndexingTaskManager().addToIndex(PdfIndexer.of(database, filePreferences), entry, database);
}
}
} catch (IOException e) {
LOGGER.error("Failed to obtain PDFIndexer.", e);
}
}
}
Expand Down
23 changes: 16 additions & 7 deletions src/main/java/org/jabref/logic/pdf/search/indexing/IndexingTaskManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ public class IndexingTaskManager extends BackgroundTask<Void> {

private final Object lock = new Object();
private boolean isRunning = false;
private boolean isBlockingNewTasks = false;

public IndexingTaskManager(TaskExecutor taskExecutor) {
this.taskExecutor = taskExecutor;
Expand Down Expand Up @@ -58,17 +59,25 @@ private void updateProgress() {
}

private void enqueueTask(Runnable indexingTask) {
taskQueue.add(indexingTask);
// What if already running?
synchronized (lock) {
if (!isRunning) {
isRunning = true;
this.executeWith(taskExecutor);
showToUser(false);
if (!isBlockingNewTasks) {
taskQueue.add(indexingTask);
// What if already running?
synchronized (lock) {
if (!isRunning) {
isRunning = true;
this.executeWith(taskExecutor);
showToUser(false);
}
}
}
}

public void setBlockingNewTasks(boolean blockingNewTasks) {
synchronized (lock) {
isBlockingNewTasks = blockingNewTasks;
}
}

public void createIndex(PdfIndexer indexer) {
enqueueTask(() -> indexer.createIndex());
}
Expand Down