Provides download option in context menu and fixes #3614 #3824

tobiasdiez · 2018-03-09T15:08:32Z

Inspired by the discussion in #3818, I added a "Download file" option in the context menu of a linked file (if it is an online link). Moreover, the bug #3614 is fixed.

This PR was brought to you with the support of coffee from the JabRef cup!

Change in CHANGELOG.md described
Tests created for changes
Manually tested changed features in running JabRef
Screenshots added in PR description (for bigger UI changes)
Ensured that the git commit message is a good one
Check documentation status (Issue created for outdated help page at help.jabref.org?)

Siedlerchr · 2018-03-09T16:38:39Z

src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java

+        } else {
+            directory = fileDirectories.get(0);
+        }
+        final Path suggestDir = directory == null ? Paths.get(System.getProperty("user.home")) : directory;


There is a method which always returns a file dir: getfirstExistingFileDir

Good observation. Thanks for the hint.

Siedlerchr · 2018-03-09T16:39:24Z

src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java

+        * https://support.apple.com/en-us/HT202808
+        */
+        if (OS.WINDOWS) {
+            plannedName = plannedName.replaceAll("\\?|\\*|\\<|\\>|\\||\\\"|\\:|\\.$|\\[|\\]", "");


extract to a matcher?

It turns out that this code is not needed since the FileUtil class already handles cleanup of file names

Siedlerchr · 2018-03-09T16:43:06Z

src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java


-    public LinkedFilesEditorViewModel(String fieldName, AutoCompleteSuggestionProvider<?> suggestionProvider, DialogService dialogService, BibDatabaseContext databaseContext, TaskExecutor taskExecutor, FieldCheckers fieldCheckers) {
+    public LinkedFilesEditorViewModel(String fieldName, AutoCompleteSuggestionProvider<?> suggestionProvider, DialogService dialogService, BibDatabaseContext databaseContext, TaskExecutor taskExecutor, FieldCheckers fieldCheckers, JabRefPreferences preferences) {


I know that's its not taht good style to have methods with long params, but why not directly use the FileDirectoryPreferences and ImportFormatPrefernces as args?

Mhhh, I tried to collect all the preferences accesses and even for the LinkedFileViewModel it were 5 different settings that are needed. Thus I prefer to keep the general preferences class as dependency.

Siedlerchr

LGTM now

Siedlerchr · 2018-03-14T16:42:08Z

If you fix checkstyle we can merge

clarity-bot · 2018-03-15T19:31:20Z

I generated a structure-diff below from pulling 19945df on JabRef:downloadLinks into 988fb54 on JabRef:master.

koppor

Some comments on "Globals", otherwise: LGTM. Did not try out locally though 😇

koppor · 2018-03-16T08:29:31Z

src/main/java/org/jabref/gui/fieldeditors/FieldEditors.java

@@ -51,7 +51,7 @@ public static FieldEditorFX getForField(String fieldName, TaskExecutor taskExecu
        } else if (fieldExtras.contains(FieldProperty.OWNER)) {
            return new OwnerEditor(fieldName, preferences, suggestionProvider, fieldCheckers);
        } else if (fieldExtras.contains(FieldProperty.FILE_EDITOR)) {
-            return new LinkedFilesEditor(fieldName, dialogService, databaseContext, taskExecutor, suggestionProvider, fieldCheckers);
+            return new LinkedFilesEditor(fieldName, dialogService, databaseContext, taskExecutor, suggestionProvider, fieldCheckers, preferences);


Do really the whole preferences need to be passed? Didn't we want to implement information hiding here? 😇

koppor · 2018-03-16T08:30:01Z

src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java


+    @Deprecated


Please add a JavaDoc pointing to the method one should use from now on.

Good observation. Fixed.

koppor · 2018-03-16T08:31:00Z

src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java

    }

-    protected LinkedFileViewModel(LinkedFile linkedFile, BibEntry entry, BibDatabaseContext databaseContext,
-                                  TaskExecutor taskExecutor, DialogService dialogService) {
+    public LinkedFileViewModel(LinkedFile linkedFile, BibEntry entry, BibDatabaseContext databaseContext,


I think, only the IMPORT_FILENAMEPATTERN and preferences.getFileDirectoryPreferences() are needed. Why not pass them both (as quick fix regarding the information hiding). OK, a new preference object would even be better, but that costs more time.

Also the auto-link preferences as well as the import preferences are needed. You are definitely right, a custom preference object unifying them all would be nice. To be honest, I don't have the motivation right now to create one (especially since the focus of this PR is a different one).

Siedlerchr · 2018-03-16T09:29:03Z

src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java

-                    Globals.prefs.getLayoutFormatterPreferences(Globals.journalAbbreviationLoader),
-                    Globals.prefs.getFileDirectoryPreferences(), linkedFile);
+                    cleanupPreferences.getFileNamePattern(),
+                    layoutFormatterPreferences,


Actually layoutFormatterPrefernces can be removed from all cleanup ops (did this the maintable beta), but out of scope for this pr

* upstream/master: Provides download option in context menu and fixes #3614 (#3824) Minor refactoring # Conflicts: # src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java # src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java # src/test/java/org/jabref/architecture/TestArchitectureTests.java # src/test/java/org/jabref/gui/fieldeditors/LinkedFileViewModelTest.java

…esDlg * upstream/maintable-beta: Get rid of journal abbrev loader in linkedFiles (#3862) fix build, remove layout prefs, pass journal abbrev loader and prefs where necessary Watch main css file for change and reapply automatically (#3847) Provides download option in context menu and fixes #3614 (#3824) Minor refactoring # Conflicts: # src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java

* javafx replacement for file dialog * Add some core structure for selectFilesDialog * Add Properties for binding * add getters * Renaming, use properties in controller open dialog with attach file action * port some more code * Asssign combobox selected value property * fix some formatting * make getController public to get viewModel * add enum with config values instead of booleans * Fix depdendency injection * Rework * Merge remote-tracking branch 'upstream/master' into maintable-beta * upstream/master: Provides download option in context menu and fixes #3614 (#3824) Minor refactoring # Conflicts: # src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java # src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java # src/test/java/org/jabref/architecture/TestArchitectureTests.java # src/test/java/org/jabref/gui/fieldeditors/LinkedFileViewModelTest.java * Create Wrapper to pass LinkedFile around TODO: what do to on edit? How to save the new file? * fix some indentations update File after Edit * remove open unknown external file type dialog convert ImportInspectionDialog to use new file edit dialog * fix indentation * convert attachFileDialog to javafx show file open dialog on attach file show dialog for editing file afterwards * reformat * Mark old filelist as deprecated Pass Preferences Service instead of Preferences * fix checkstyle remove unused parameter entry * fix checkstyle again * add changelog Move fxml to resources rework layout to grid * adjust indentation * convert to new FXML dialog model funcionality still some NPEs * fix empty lines * fix checkstyle * Fix viewModel NPE in copyFiles Action and linkedfilesEditDialog * Add close button in copy linked files * fix checkstyle * Rename and reformat * remove changelog move fxml to java package remove wrapper class pass externalFileType to viewModel renammings * remove close method Add ok button to dialog pane * change some odd looking assigments * renaming * forgotten rename

Provides download option in context menu and fixes #3614

7714558

tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 9, 2018

Siedlerchr reviewed Mar 9, 2018

View reviewed changes

Code cleanup

6c19f6d

Siedlerchr approved these changes Mar 14, 2018

View reviewed changes

Fix checkstyle

44ac7c0

Fix tests

19945df

koppor requested changes Mar 16, 2018

View reviewed changes

Siedlerchr reviewed Mar 16, 2018

View reviewed changes

koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 16, 2018

tobiasdiez added 2 commits March 17, 2018 11:09

Add java doc

d58dd2b

Fix tests

80a6fb0

tobiasdiez merged commit 2cdf085 into master Mar 17, 2018

tobiasdiez deleted the downloadLinks branch March 17, 2018 10:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provides download option in context menu and fixes #3614 #3824

Provides download option in context menu and fixes #3614 #3824

tobiasdiez commented Mar 9, 2018 •

edited

Loading

Siedlerchr Mar 9, 2018

tobiasdiez Mar 10, 2018

Siedlerchr Mar 9, 2018

tobiasdiez Mar 10, 2018

Siedlerchr Mar 9, 2018

tobiasdiez Mar 10, 2018

Siedlerchr left a comment

Siedlerchr commented Mar 14, 2018

clarity-bot bot commented Mar 15, 2018 •

edited

Loading

koppor left a comment

koppor Mar 16, 2018

koppor Mar 16, 2018

tobiasdiez Mar 17, 2018

koppor Mar 16, 2018

tobiasdiez Mar 17, 2018

Siedlerchr Mar 16, 2018


		public LinkedFilesEditorViewModel(String fieldName, AutoCompleteSuggestionProvider<?> suggestionProvider, DialogService dialogService, BibDatabaseContext databaseContext, TaskExecutor taskExecutor, FieldCheckers fieldCheckers) {
		public LinkedFilesEditorViewModel(String fieldName, AutoCompleteSuggestionProvider<?> suggestionProvider, DialogService dialogService, BibDatabaseContext databaseContext, TaskExecutor taskExecutor, FieldCheckers fieldCheckers, JabRefPreferences preferences) {

Provides download option in context menu and fixes #3614 #3824

Provides download option in context menu and fixes #3614 #3824

Conversation

tobiasdiez commented Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr left a comment

Choose a reason for hiding this comment

Siedlerchr commented Mar 14, 2018

clarity-bot bot commented Mar 15, 2018 • edited Loading

koppor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasdiez commented Mar 9, 2018 •

edited

Loading

clarity-bot bot commented Mar 15, 2018 •

edited

Loading