Rework AutoSetFileLinks by Siedlerchr · Pull Request #3368 · JabRef/jabref

Siedlerchr · 2017-10-28T20:00:39Z

Change in CHANGELOG.md described
Tests created for changes
Screenshots added (for bigger UI changes)
Manually tested changed features in running JabRef
Check documentation status (Issue created for outdated help page at help.jabref.org?)
If you changed the localization: Did you run gradle localizationUpdate?

…manually triggering it

tobiasdiez

The code looks good to me, besides some smaller points. However, I have to admit that I don't really understand what this auto-set method does in detail. Thus I would recommend to do a bit of refactoring, extract all the logic code to a new class in logic and add tests for this new class. I know this is additional work, but in view of the recent issues related to the autolink feature, tests would be a huge help.

tobiasdiez · 2017-10-29T01:36:43Z

+                                    return file.isPresent() && Files.isSameFile(file.get(), foundFile);
+                                } catch (IOException e) {
+                                    // TODO Auto-generated catch block
+                                    e.printStackTrace();


Use proper logging here.

tobiasdiez · 2017-10-29T01:37:42Z

+
+                        foundAny = true;
+                        Optional<ExternalFileType> type = FileHelper.getFileExtension(foundFile)
+                                .map(extension -> ExternalFileTypes.getInstance().getExternalFileTypeByExt(extension))


Use flatmap instead?

tobiasdiez · 2017-10-29T01:38:43Z

+                        try {
+                            return file.isPresent() && Files.isSameFile(file.get(), foundFile);
+                        } catch (IOException e) {
+                            // TODO Auto-generated catch block


proper logging

tobiasdiez · 2017-10-29T01:42:49Z

        List<LinkedFileViewModel> result = new ArrayList<>();
-        for (Path newFile : newFiles) {
-            boolean alreadyLinked = files.get().stream()
+        for (Path foundFile : newFiles) {


This code now seems to have a huge overlap with the above changes. Can you please try to extract the common code?

TODO: remove duplicate findings

Only call setFiles when pressing F7 Fix javafx thread error

use streams in externalfiletype

remove javadoc params

* upstream/master: Refactorings (#3370) # Conflicts: # src/main/java/org/jabref/gui/externalfiles/AutoSetLinks.java # src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java

Siedlerchr · 2017-10-29T18:22:04Z

@tobiasdiez I now moved the common code to an own method in a class in the gui.
It can't be moved to logic because it depends on ExternalFileType
I tried adding a test for it, but even with extensive mocking, ExternalFileTypes are again the problem - Because it's a singleton class...Mockito is not able to mock it.

* upstream/master: revert wrongly commited changes # Conflicts: # src/main/java/org/jabref/gui/externalfiles/AutoSetLinks.java # src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java

tobiasdiez · 2017-10-30T01:24:15Z

+                    String strType = type.isPresent() ? type.get().getName() : "";
+
+                    LinkedFile linkedFile = new LinkedFile("", foundFile.toString(), strType);
+                    linkedFiles.put(entryFilePair.getKey(), linkedFile);


If I see it correctly, we are only returning one file per entry although the file finder may find more than 1, right?

Personally, I would restructure the code a bit more: instead of calling findassociatedNotLinkedFiles(List<BibEntry> entries,... in the method findassociatedNotLinkedFiles(BibEntry entry,..., I would iterate over all bib entries and call the method for on entry, i.e.

findassociatedNotLinkedFiles(List<BibEntry> entries) { for(entry in entries) { append(findassociatedNotLinkedFiles(entry)); } }

Or is there is good reason to not do this?

I changed the code so that multiple files will be added, except if there already exists the same file. That's why I return a map now
In the lines above I iterate over all pairs of entries and files

Sorry, I don't really understand what you mean. Since you return a map, there is by definition only one returned file per entry (since the key in the map is unique). What you need is a multimap (we use a library that provides these...no idea which...guava?)

Ah yes, did not have this in mind. Will think about another solution

I think the code gets considerably more easy if you just do the check for one entry (and not a collection) and return a list of to-be-added links. Then the caller can iterate over it.

tobiasdiez · 2017-10-30T01:26:12Z

Thanks for the further refactoring. The code looks better now!

Concerning the tests, you should be able to extract ExternalFileTypes.getInstance() to a method parameter and then simply pass a mock(ExternalFileTypes.class) in the test.

Add test

Siedlerchr · 2017-10-30T14:32:13Z

Thanks for the hints. I was no able to create a test and to faciliate the code

tobiasdiez

Thanks for the quick follow-ups! The code looks good to me now.

* upstream/master: Rework AutoSetFileLinks (#3368) revert wrongly commited changes Refactorings (#3370) Fix freezing when running cleanup file operations like rename (#3315) This additional style setting for IDEA (#3355) Fix checkstyle Add proper equals to content selector Source tab entry duplication (#3360) Use CITE_COMMENT not only for external latex editors but also for cop… (#3351) Updating with new translations (#3348) Upgrade error-prone (#3350) Jabref_pt_BR partially updated (#3349) Delete unused code Extract difference finder from gui to logic Used late initialization for context menus (#3340) Remove unnecessary EntrySorter Move existing code to gui and rename change classes to view model Small code improvements

* upstream/master: (79 commits) Update richtext and flowless (#3386) Source tab: hide notification pane when code is correct again (#3387) Titles are optional at crossref (#3378) Update entry for every change in the source panel (#3366) Rework AutoSetFileLinks (#3368) revert wrongly commited changes Refactorings (#3370) Fix freezing when running cleanup file operations like rename (#3315) This additional style setting for IDEA (#3355) Fix checkstyle Add proper equals to content selector Source tab entry duplication (#3360) Use CITE_COMMENT not only for external latex editors but also for cop… (#3351) Updating with new translations (#3348) Upgrade error-prone (#3350) Jabref_pt_BR partially updated (#3349) Delete unused code Extract difference finder from gui to logic Used late initialization for context menus (#3340) Fix NPE when calling with bib file as cmd argument (#3343) ...

* upstream/master: (22 commits) Improve SyncLang.py (#3184) Config intellj code style to adhere to empty lines checkstyle rule (#3365) Don't list same look and feels more than once (#3393) progessdialog and result table are now shown correclty on copy files Export pdf/linked files (#3147) Added checking integrity dialog (#3388) Update richtext and flowless (#3386) Source tab: hide notification pane when code is correct again (#3387) Titles are optional at crossref (#3378) Update entry for every change in the source panel (#3366) Rework AutoSetFileLinks (#3368) revert wrongly commited changes Refactorings (#3370) Fix freezing when running cleanup file operations like rename (#3315) This additional style setting for IDEA (#3355) Fix checkstyle Add proper equals to content selector Delete unused code Extract difference finder from gui to logic Remove unnecessary EntrySorter ... # Conflicts: # src/main/java/org/jabref/gui/entryeditor/SourceTab.java

* upstream/master: (26 commits) Fix static localized text (#3400) Fix problems in source editor (#3398) Fix MathSciNet fetcher (#3402) Fix NPE when saving new file Improve SyncLang.py (#3184) Config intellj code style to adhere to empty lines checkstyle rule (#3365) Don't list same look and feels more than once (#3393) progessdialog and result table are now shown correclty on copy files Export pdf/linked files (#3147) Added checking integrity dialog (#3388) Update richtext and flowless (#3386) Source tab: hide notification pane when code is correct again (#3387) Titles are optional at crossref (#3378) Update entry for every change in the source panel (#3366) Rework AutoSetFileLinks (#3368) revert wrongly commited changes Refactorings (#3370) Fix freezing when running cleanup file operations like rename (#3315) This additional style setting for IDEA (#3355) Fix checkstyle ... # Conflicts: # CHANGELOG.md

Siedlerchr added 5 commits October 28, 2017 22:00

Rework AutoSetFileLinks

61fb09f

Only add first found file

d03f155

Improve magic AutoFileLinking in entry editor by using same logic as …

51fe583

…manually triggering it

Don't use paths.get

0a305fe

add changelog entry

124bc37

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 28, 2017

tobiasdiez requested changes Oct 29, 2017

View reviewed changes

Siedlerchr added 6 commits October 29, 2017 16:46

refactor

8ab45cd

TODO: remove duplicate findings

Use HashMap with BibEntry and LinkedFiles

8aed0d0

Only call setFiles when pressing F7 Fix javafx thread error

improve optional code

fd29758

use streams in externalfiletype

logging

12f21bf

remove javadoc params

pass preferences to method

fff82ea

Merge remote-tracking branch 'upstream/master' into autofilelink

1e16b17

* upstream/master: Refactorings (#3370) # Conflicts: # src/main/java/org/jabref/gui/externalfiles/AutoSetLinks.java # src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java

Merge remote-tracking branch 'upstream/master' into autofilelink

1948112

* upstream/master: revert wrongly commited changes # Conflicts: # src/main/java/org/jabref/gui/externalfiles/AutoSetLinks.java # src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java

tobiasdiez reviewed Oct 30, 2017

View reviewed changes

Return list of Linkedfiles and let caller handle bib entries

229ce64

Add test

Replace mocking of Jabrefs prefs with autolink prefs

05fa33f

tobiasdiez approved these changes Oct 31, 2017

View reviewed changes

Siedlerchr merged commit 551cb91 into master Oct 31, 2017

Siedlerchr deleted the autofilelink branch October 31, 2017 10:09

Siedlerchr mentioned this pull request Dec 10, 2017

Fixes #3472: Files starting with BibTeX key of a similar entry are no longer found by mistake #3509

Merged

6 tasks

koppor mentioned this pull request May 30, 2018

Look up fulltext documents does not link existing PDFs #3874

Closed

Uh oh!

Conversation

Siedlerchr commented Oct 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobiasdiez 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!

Siedlerchr commented Oct 29, 2017

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 commented Oct 30, 2017

Uh oh!

Siedlerchr commented Oct 30, 2017

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Siedlerchr commented Oct 28, 2017 •

edited

Loading