-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Relink moved files [Find Unlinked Files part] #13842
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?
Changes from all commits
b929b47
1375669
33c1eb9
374491c
a34fa0f
9fb6ddb
faadb71
edfc5d7
da72c9c
a67e1a4
10fb428
9d3fa0e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -7,9 +7,13 @@ | |||||||||
| import java.nio.file.Files; | ||||||||||
| import java.nio.file.Path; | ||||||||||
| import java.nio.file.StandardOpenOption; | ||||||||||
| import java.util.ArrayList; | ||||||||||
| import java.util.List; | ||||||||||
| import java.util.Optional; | ||||||||||
| import java.util.Set; | ||||||||||
| import java.util.function.Predicate; | ||||||||||
| import java.util.stream.Collectors; | ||||||||||
| import java.util.stream.Stream; | ||||||||||
|
|
||||||||||
| import javax.swing.undo.UndoManager; | ||||||||||
|
|
||||||||||
|
|
@@ -41,7 +45,10 @@ | |||||||||
| import org.jabref.logic.util.BackgroundTask; | ||||||||||
| import org.jabref.logic.util.StandardFileType; | ||||||||||
| import org.jabref.logic.util.TaskExecutor; | ||||||||||
| import org.jabref.logic.util.io.FileUtil; | ||||||||||
| import org.jabref.model.database.BibDatabaseContext; | ||||||||||
| import org.jabref.model.entry.BibEntry; | ||||||||||
| import org.jabref.model.entry.LinkedFile; | ||||||||||
| import org.jabref.model.util.FileUpdateMonitor; | ||||||||||
|
|
||||||||||
| import de.saxsys.mvvmfx.utils.validation.FunctionBasedValidator; | ||||||||||
|
|
@@ -118,14 +125,82 @@ public UnlinkedFilesDialogViewModel(DialogService dialogService, | |||||||||
| treeRootProperty.setValue(Optional.empty()); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| public void findAndFixBrokenLinks(Path searchDirectory) { | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method needs a JavaDoc comment Maybe also the name needs to be adjusted --> Is it possible NOT to use a global variable Finally, this should go to |
||||||||||
| List<ImportFilesResultItemViewModel> results = new ArrayList<>(); | ||||||||||
Siedlerchr marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
|
|
||||||||||
| // pre-build a map of all existing linked files for O(1) lookup | ||||||||||
| Set<Path> existingLinkedFiles = bibDatabase.getDatabase().getEntries().stream() | ||||||||||
| .flatMap(entry -> entry.getFiles().stream()) | ||||||||||
| .map(linkedFile -> linkedFile.findIn(bibDatabase, preferences.getFilePreferences())) | ||||||||||
| .filter(Optional::isPresent) | ||||||||||
| .map(Optional::get) | ||||||||||
| .collect(Collectors.toSet()); | ||||||||||
|
|
||||||||||
| for (BibEntry entry : bibDatabase.getDatabase().getEntries()) { | ||||||||||
| try { | ||||||||||
| List<LinkedFile> currentFiles = new ArrayList<>(entry.getFiles()); | ||||||||||
| List<LinkedFile> filesToRemove = new ArrayList<>(); | ||||||||||
| List<LinkedFile> filesToAdd = new ArrayList<>(); | ||||||||||
|
|
||||||||||
| for (LinkedFile brokenLink : currentFiles) { | ||||||||||
| if (brokenLink.findIn(bibDatabase, preferences.getFilePreferences()).isPresent()) { | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this just duplicated? You add all those to entriesWithBrokenLInks that are not present so why checking them again here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not all.. only entries having at least one broken link... though it could be improved thnks for letting know |
||||||||||
| continue; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| String exactFileName = Path.of(brokenLink.getLink()).getFileName().toString(); | ||||||||||
| List<Path> matches = new ArrayList<>(); | ||||||||||
|
|
||||||||||
| try (Stream<Path> walk = Files.walk(searchDirectory)) { | ||||||||||
| walk.filter(path -> !Files.isDirectory(path)) | ||||||||||
| .filter(path -> path.getFileName().toString().equals(exactFileName)) | ||||||||||
| .forEach(matches::add); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // check if matches found is only one | ||||||||||
| if (matches.size() != 1) { | ||||||||||
| continue; | ||||||||||
| } | ||||||||||
| Path foundFile = matches.getFirst(); | ||||||||||
|
|
||||||||||
| // check if the potential file is not linked to other entries | ||||||||||
| if (existingLinkedFiles.contains(foundFile)) { | ||||||||||
| continue; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| Path newFilePath = preferences.getFilePreferences().shouldStoreFilesRelativeToBibFile() ? | ||||||||||
| FileUtil.relativize(foundFile, bibDatabase.getFileDirectories(preferences.getFilePreferences())) : | ||||||||||
| foundFile; | ||||||||||
|
|
||||||||||
| filesToRemove.add(brokenLink); | ||||||||||
| filesToAdd.add(new LinkedFile(brokenLink.getDescription(), newFilePath, brokenLink.getFileType())); | ||||||||||
| existingLinkedFiles.add(foundFile); | ||||||||||
|
|
||||||||||
| results.add(new ImportFilesResultItemViewModel(foundFile, true, | ||||||||||
| Localization.lang("File relinked to entry %0.", exactFileName))); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| if (!filesToRemove.isEmpty() || !filesToAdd.isEmpty()) { | ||||||||||
| currentFiles.removeAll(filesToRemove); | ||||||||||
| currentFiles.addAll(filesToAdd); | ||||||||||
| entry.setFiles(currentFiles); | ||||||||||
| } | ||||||||||
| } catch (IOException e) { | ||||||||||
| LOGGER.error("Error finding associated files for entry", e); | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if the data structures are consistent in all error cases. Shouldn't the try/catch be more close to the points where the exception can occur and an appropriate handling take place? Are |
||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| resultList.clear(); | ||||||||||
| resultList.addAll(results); | ||||||||||
|
Comment on lines
+192
to
+193
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs a Java comment - why this is done. |
||||||||||
| } | ||||||||||
|
|
||||||||||
| public void startSearch() { | ||||||||||
| Path directory = this.getSearchDirectory(); | ||||||||||
| findAndFixBrokenLinks(directory); | ||||||||||
| Filter<Path> selectedFileFilter = selectedExtension.getValue().dirFilter(); | ||||||||||
| DateRange selectedDateFilter = selectedDate.getValue(); | ||||||||||
| ExternalFileSorter selectedSortFilter = selectedSort.getValue(); | ||||||||||
| progressValueProperty.unbind(); | ||||||||||
| progressTextProperty.unbind(); | ||||||||||
|
|
||||||||||
| findUnlinkedFilesTask = new UnlinkedFilesCrawler(directory, selectedFileFilter, selectedDateFilter, selectedSortFilter, bibDatabase, preferences.getFilePreferences()) | ||||||||||
| .onRunning(() -> { | ||||||||||
| progressValueProperty.set(ProgressIndicator.INDETERMINATE_PROGRESS); | ||||||||||
|
|
@@ -296,4 +371,9 @@ public BooleanProperty taskActiveProperty() { | |||||||||
| public SimpleListProperty<TreeItem<FileNodeViewModel>> checkedFileListProperty() { | ||||||||||
| return checkedFileListProperty; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // For testing purposes | ||||||||||
| public int resultListSize() { | ||||||||||
|
Comment on lines
+375
to
+376
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
| return resultList.size(); | ||||||||||
| } | ||||||||||
| } | ||||||||||
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.
This feels not OK - promising a list in the method contract, but internally a set with a random order.
Please convert the result type to
Setand also adapt the callers. Then, you will see if there is a list assumption anywhere -- and if there should be an ordering done. If an ordering needs to be done, either do it here (Maybe, useSortedSet). Or do it at the place where sorting is needed.