Add a preference to add files in entry editor by shahamish150294 · Pull Request #4408 · JabRef/jabref

shahamish150294 · 2018-10-26T03:46:29Z

This PR for #4356 mainly involves

Allowing users to choose how they want to link/copy/rename/move files in Entry Editor through Preferences
Apply same behavior of drag and drop for the whole entry editor instead of a different behavior in file view of the entry editor.

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
I will log an issue to help.jabref.org if these changes are approved. Please feel free to suggest any changes. Thank you!

Siedlerchr · 2018-10-26T06:04:50Z

 - We added a new keyboard shortcut so that the entry editor could be closed by <kbd>Ctrl<kbd> + <kbd>E<kbd>. [#4222] (https://github.com/JabRef/jabref/issues/4222)
 - We added an option in the preference dialog box, that allows user to pick the dark or light theme option. [#4130] (https://github.com/JabRef/jabref/issues/4130)
-
+- Allow the user to choose behavior after dragging and dropping files in Entry Editor using Preferences. [#4356]


Please follow the convention of the other log entries

Please let me know if this d57beda looks good

Also, I have logged an issue to help.jabref.org to update the documentation for this new preference option - https://github.com/JabRef/help.jabref.org/issues/194

Siedlerchr

Thanks for your contribution 👍 Codewise looks good. Please adapt the changelog entry and then we can merge it.

shahamish150294 · 2018-10-26T12:43:04Z

Codacy seems to have stuck in Pending/Analyzing state. Is there something that I need to do from my side so that its analysis is completed?

Siedlerchr · 2018-10-26T14:31:41Z

Hit it was probably stuck because you had a merge conflict in the changelog.md file.
I resolved this.
PS: For future contributions create a new branch and do not work on master (Reduces potential conflicts)

Siedlerchr

LGTM! Thanks for your contribution!

shahamish150294 · 2018-10-26T22:07:40Z

Thanks!

tobiasdiez

Thanks for your PR.

The code changes look good to me, however, your solution has the drawback that one can no longer use the modifier keys (Shift/Alt/Ctrl) to control the behavior of the drop. The preference value should only be treated as the default action (when no modifier is pressed). It would be nice if you could revise this part again.

tobiasdiez · 2018-10-27T11:47:43Z

-
-        if (dragboard.hasContent(DragAndDropDataFormats.LINKED_FILE)) {
-
-            LinkedFile linkedFile = (LinkedFile) dragboard.getContent(DragAndDropDataFormats.LINKED_FILE);


It is ok to remove the code related to drag & drop of external files, because this is handled by the entry editor as you have noted. However, it should still be possible to resort the linked files using drag & drop.

tobiasdiez · 2018-10-27T11:51:27Z

        builder.add(firstNameModeBoth,  1, 22);
+
+        final ToggleGroup group = new ToggleGroup();
+        Label linkFileOptions = new Label(Localization.lang("Options to add file"));


Maybe reformulate to Default drag & drop action?

Fixed as you suggested: https://github.com/JabRef/jabref/pull/4408/files#diff-e0f0e1ade7c4cee8c43b0bb2254c21ebR125

tobiasdiez · 2018-10-27T11:52:09Z

+        final ToggleGroup group = new ToggleGroup();
+        Label linkFileOptions = new Label(Localization.lang("Options to add file"));
+        linkFileOptions.getStyleClass().add("sectionHeader");
+        copyFile = new RadioButton(Localization.lang("Copy file to current location"));


Suggestion: Copy file to default file folder (or location instead of folder)

https://github.com/JabRef/jabref/pull/4408/files#diff-e0f0e1ade7c4cee8c43b0bb2254c21ebR127

tobiasdiez · 2018-10-27T11:54:10Z

+    private boolean linkFile;
+    private boolean renameCopyFile;
+
+    public FileDragDropPreferences(boolean copyFile, boolean linkFile, boolean renameCopyFile) {


At this point, it appears to be cleaner to introduce an enum for the three different behaviors, especially since they are mutually exclusive.

Switched to enum:
66443c0#diff-65271fca5ef3c430d80bc31ef69fdf56R3

shahamish150294 · 2018-10-27T20:50:38Z

@tobiasdiez Thanks for reviewing. For:

The preference value should only be treated as the default action (when no modifier is pressed)

I don't think there is a straightforward solution using the DragEvent used in the code. This is because I wont be able to detect the modifiers pressed or confirm whether no modifier is pressed. I will keep looking into this, let me know if you have any guidance to resolve it.

tobiasdiez · 2018-10-27T22:45:03Z

The code you edited had this functionality before. You can get the modifier using event.getTransferMode() (see also the code comments).

shahamish150294 · 2018-10-27T23:03:30Z

What will happen in a case where the TransferMode is MOVE and would have no Shift key pressed at that time? The comment states - 'shift on win or no modifier' How would I differentiate between TransferMode.MOVE (without Shift key pressed) and dragDropped behavior from Preference option (as we want this to be read only when there no modifiers)?
For the other two modes (LINK and COPY), I was thinking to do an OR with preference option but that would not be applicable for TransferMode.MOVE.

tobiasdiez · 2018-10-28T09:00:10Z

Its probably not possible to differentiate between move = no modifier and move = shift. Thus, I would suggest the following behavior:
move -> do what is configured in preferences (default: move, rename and link file)
link -> link file except if preferences = link, in this case move, rename and link
copy -> do what is configured in preferences (default: move, rename and link file)

In this way, the user can configure the default action on windows and linux, but is also possible to only link the file if he wants to.

shahamish150294 · 2018-10-29T13:22:42Z

Just need some clarification on the behavior for 'link'. I got that if the transfer mode is link, then we should allow the user to link file. But if the preference is set to 'link' with 'link' as the transfer mode then why should we move, rename and link file? Shouldn't we follow move, rename and link when preference is set to 'move, rename and link' and not when it is set 'link'?

tobiasdiez · 2018-10-29T16:52:23Z

I wasn't sure about this either. My rationale was to allow for the following behavior:

Users selects "Move & Rename" as default action:

No modifier -> Move & Rename
Ctrl (= TransferMode.Link) -> Link

Users selects "Link" as default action:

No modifier -> Link
Ctrl (= TransferMode.Link) -> Move & Rename

Because otherwise the users has no option to perform "Move & Rename" if he selected "Link" as the default action. But I'm very open for suggestions if this behavior is counter intuitive.

shahamish150294 · 2018-10-29T23:11:11Z

I think your option is good. But I just feel that it should be documented that what keys would be used for each option in preferences tab in brackets. One more suggestion that we can consider is as follows:

pref = move
: shift or no modifier => move
: ctrl => copy
: alt => link
pref = copy
: shift or no modifier => copy
: alt => link (alt is the "default" key for linking so user has one less thing to remember)
: ctrl => move
pref = link
: shift or no modifier => link
: alt => move
: ctrl => copy (ctrl is the "default" key for copying so user has one less thing to remember)

Let me know your thoughts on this.

Siedlerchr · 2018-10-30T07:57:33Z

@shahamish150294 Please be aware that the keys differ on other operating systems, e.g. Linux or Mac. and not all operating systems differ between all modifiers. See my code comment regarding Ubuntu somewhere

https://ux.stackexchange.com/questions/83748/what-are-the-most-common-modifier-keys-for-dragging-objects-with-a-mouse

tobiasdiez · 2018-10-30T10:41:35Z

Your suggestion looks good to me.

Whatever behavior we choose in the end, it should be documented in the help and even better directly during the drag & drop action, e.g. something like the following and replacing "sposta" with the real action to be performed (but this is something for a new PR).

…op preference with enums

…eference default

shahamish150294 · 2018-10-31T03:53:16Z

I have added the logic we discussed above and fixed the LinkedFilesEditor to allow moving of the files within the File Editor. I also updated the issue on help.jabref to follow the behavior as we have discussed here for documentation.

Siedlerchr

LGTM , thanks for your contribution!

Siedlerchr · 2018-11-01T16:09:31Z

If @tobiasdiez gives his okay, we can merge this!

tobiasdiez

Sorry for the late review. Was a bit busy the last days. Thanks a lot for the follow-up! The code looks very good now and I'll hence press the magic button ;-)

* upstream/master: Bump junit-pioneer from 0.2.2 to 0.3.0 (#4451) Add Depandabot badge Emphasize that users should try out the newest version before reporting a bug Add a preference to add files in entry editor (#4408)

* upstream/master: New Crowdin translations (#4454) Fix error in l10n file: Toogle -> Toggle (#4453) Remove depandabot Bump junit-pioneer from 0.2.2 to 0.3.0 (#4451) Add Depandabot badge Emphasize that users should try out the newest version before reporting a bug Add a preference to add files in entry editor (#4408) Add submodule pull to circle ci and fix theme loading css (#4443) fix groups drag and drop (#4439)

* upstream/master: New Crowdin translations (#4454) Fix error in l10n file: Toogle -> Toggle (#4453) Remove depandabot Bump junit-pioneer from 0.2.2 to 0.3.0 (#4451) Add Depandabot badge Emphasize that users should try out the newest version before reporting a bug Add a preference to add files in entry editor (#4408) Add submodule pull to circle ci and fix theme loading css (#4443) fix groups drag and drop (#4439) Convert merge entries dialog to JavaFX (#4410) Fix ArrayIndexOutOfBoundsException on second pdf import (#4426) Fix radiobuttons in preference menu (#4409) Add citation styles as git submodule (#4431) Fix highlight color of selected text and progress bar (#4420) Fix two new issues Fix Violations of Always Use Braces (#4400) Delete PreferencesService.java.orig Add JabRef icon to installer and change watermark to JabRef (#4421) Checkstyle: force braces around code blocks

Siedlerchr reviewed Oct 26, 2018

View reviewed changes

Siedlerchr approved these changes Oct 26, 2018

View reviewed changes

shahamish150294 mentioned this pull request Oct 26, 2018

Document - New preference option to add files in Entry Editor JabRef/user-documentation#194

Closed

shahamish150294 closed this Oct 26, 2018

shahamish150294 reopened this Oct 26, 2018

Siedlerchr approved these changes Oct 26, 2018

View reviewed changes

tobiasdiez requested changes Oct 27, 2018

View reviewed changes

Siedlerchr mentioned this pull request Oct 27, 2018

Dropping a PDF on an entry should be consistent #121

Closed

shahamish150294 force-pushed the master branch from 50657f1 to b4d4b2d Compare October 31, 2018 03:09

shahamish150294 added 7 commits October 30, 2018 22:12

Add a preference to add files in entry editor (JabRef#4356)

be0c251

Fix import order

4a01a80

Edit changelog

955bb78

Use enum to represent FileDragDropPreference for Entry Editor

ff29ae6

Changes for localization message and replaced boolean for drag and dr…

dbfafc0

…op preference with enums

Change drag-drop option name in preferences

50bf10a

Added behaviors to accommodate different drag-drop ways along with pr…

ca4ac4f

…eference default

shahamish150294 force-pushed the master branch from b4d4b2d to ca4ac4f Compare October 31, 2018 03:16

Added whitespaces after if condition

8c6a90f

Remove unused imports

89592d6

Siedlerchr approved these changes Nov 1, 2018

View reviewed changes

tobiasdiez approved these changes Nov 1, 2018

View reviewed changes

tobiasdiez merged commit 4d2ae94 into JabRef:master Nov 1, 2018


		if (dragboard.hasContent(DragAndDropDataFormats.LINKED_FILE)) {

		LinkedFile linkedFile = (LinkedFile) dragboard.getContent(DragAndDropDataFormats.LINKED_FILE);

Uh oh!

Conversation

shahamish150294 commented Oct 26, 2018 • edited by Siedlerchr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Siedlerchr left a comment

Choose a reason for hiding this comment

Uh oh!

shahamish150294 commented Oct 26, 2018

Uh oh!

Siedlerchr commented Oct 26, 2018

Uh oh!

Siedlerchr left a comment

Choose a reason for hiding this comment

Uh oh!

shahamish150294 commented Oct 26, 2018

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!

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!

shahamish150294 commented Oct 27, 2018

Uh oh!

tobiasdiez commented Oct 27, 2018

Uh oh!

shahamish150294 commented Oct 27, 2018

Uh oh!

tobiasdiez commented Oct 28, 2018

Uh oh!

shahamish150294 commented Oct 29, 2018

Uh oh!

tobiasdiez commented Oct 29, 2018

Uh oh!

shahamish150294 commented Oct 29, 2018

Uh oh!

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

Uh oh!

Uh oh!

tobiasdiez commented Oct 30, 2018

Uh oh!

shahamish150294 commented Oct 31, 2018

Uh oh!

Siedlerchr left a comment

Choose a reason for hiding this comment

Uh oh!

Siedlerchr commented Nov 1, 2018

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shahamish150294 commented Oct 26, 2018 •

edited by Siedlerchr

Loading

Siedlerchr commented Oct 30, 2018 •

edited

Loading