Fixes #3472: Files starting with BibTeX key of a similar entry are no longer found by mistake #3509
Conversation
… longer found by mistake
| default List<Path> findAssociatedFiles(BibEntry entry, List<Path> directories, List<String> extensions) { | ||
| Map<BibEntry, List<Path>> associatedFiles = findAssociatedFiles(Collections.singletonList(entry), directories, extensions); | ||
| return associatedFiles.getOrDefault(entry, Collections.emptyList()); | ||
| default Collection<Path> findAssociatedFiles(BibEntry entry, List<Path> directories, List<String> extensions) { |
There was a problem hiding this comment.
Why do you use the collection interface here? List interface defines a somehow ordered collection
I would really prefer list here
https://stackoverflow.com/a/3317408/3450689
There was a problem hiding this comment.
The multimap only returns a collection because it actually is not implemented as a list (the sorting is random). But now we don't need multimaps anymore so we can go back to lists!
| if (!files.isEmpty()) { | ||
| Path file = files.get(0); | ||
| Path file = files.iterator().next(); |
There was a problem hiding this comment.
I see no advantage in using Collection here
| this(databaseContext.getFileDirectoriesAsPaths(fileDirectoryPreferences), autoLinkPreferences, externalFileTypes); | ||
| } | ||
|
|
||
| public AutoSetFileLinksUtil(List<Path> directories, AutoLinkPreferences autoLinkPreferences, ExternalFileTypes externalFileTypes) { |
There was a problem hiding this comment.
I am against creating a second constructor. It was my initial idea in #3368 and guess who supposed the current alternative? 😆
See your comment #3368 (comment)
There was a problem hiding this comment.
I don't understand what you mean. The linked comment is about the list of entries and not about how the AutoSetFileLinksUtil gets the things needed (like directories and preferences). I think, the current setup makes sense: on initialization, specify everything the auto set functionality needs to know. Why don't you like these constructors?
There was a problem hiding this comment.
Ah sorry, I could not see all code in the line on my mobile, so I was a b9it confused. Forget about my remark,
| Objects.requireNonNull(directories); | ||
| Objects.requireNonNull(entries); | ||
|
|
||
| Map<BibEntry, List<Path>> result = new HashMap<>(); | ||
| Multimap<BibEntry, Path> result = ArrayListMultimap.create(); |
There was a problem hiding this comment.
If you structure the code similar to the one in AutoSetLinks, you won't need a multimap:
for (BibEntry entry : entries) {
List<LinkedFile> linkedFiles = util.findassociatedNotLinkedFiles(entry, databaseContext, Globals.prefs.getFileDirectoryPreferences(), Globals.prefs.getAutoLinkPreferences(), ExternalFileTypes.getInstance());
if (ce != null) {
for (LinkedFile linkedFile : linkedFiles) {
There was a problem hiding this comment.
Thanks! This is really good feedback. I just realized that the finder never gets a list of entries as argument but always a single entry. Thus the design can be simplified drastically without mulitmaps or maps to collections.
| @@ -56,7 +57,7 @@ public void testFindFiles() { | |||
| RegExpBasedFileFinder fileFinder = new RegExpBasedFileFinder("**/[bibtexkey].*\\\\.[extension]", ','); | |||
|
|
|||
| //when | |||
| List<Path> result = fileFinder.findAssociatedFiles(localEntry, dirs, extensions); | |||
| Collection<Path> result = fileFinder.findAssociatedFiles(localEntry, dirs, extensions); | |||
There was a problem hiding this comment.
Here you can change them back to list again
|
How does this relate with the preferences?
Documentation at http://help.jabref.org/en/FileLinks#auto-linking-files Are these settings still respected? |
|
@koppor Yes, these settings are still valid. The fix applies only if you select the first option "Auto link files that start with the BibTeX key". |
| // The file name starts with the key, that's already a good start | ||
| // However, we do not want to match "JabRefa" for "JabRef" since this is probably a file belonging to another entry published in the same time / same name | ||
| char charAfterKey = filename.charAt(citeKey.length()); | ||
| return !StringUtil.contains(BibtexKeyPatternUtil.CHARS, charAfterKey); |
There was a problem hiding this comment.
Better direclty use the java method: String.contains(Character.toString(c))
There was a problem hiding this comment.
And also take a look at the codacy
|
@koppor Why did you removed the "ready-for-review" flag and marked this PR as WIP? I was and I am still waiting for a second review, otherwise this PR is ready from my point of view. The minor issues pointed-out by @Siedlerchr are now also fixed. |
|
Think, because I watched this PR, liked it and just wanted others to focus on other PRs. |
Fixes #3472.
The auto-link algorithm just checked if the file starts with the BibTeX key. This leads to a lot of false positives since "Einstein1902a.pdf" is found for the entry "Einstein1902" although this pdf probably belongs to a second paper by Einstein that was published in the same year.
This is fixed in this PR.
With the proposed implementation also the pdf file "Einstein1902aboutSomething.pdf" is no longer found (since there is really no possibility in telling the difference to "Einstein1902a - a second paper.pdf"). Thus the user is advised to add some non-letter character after the key.
gradle localizationUpdate?