Skip to content
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

Add a preference to add files in entry editor #4408

Merged
merged 9 commits into from
Nov 1, 2018

Conversation

shahamish150294
Copy link
Contributor

@shahamish150294 shahamish150294 commented Oct 26, 2018

This PR for #4356 mainly involves

Fixes #4356

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

image

CHANGELOG.md Outdated
@@ -35,7 +35,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
- We added a minimal height for the entry editor so that it can no longer be hidden by accident. [#4279](https://github.com/JabRef/jabref/issues/4279)
- 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]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow the convention of the other log entries

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@shahamish150294
Copy link
Contributor Author

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
Copy link
Member

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)

Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for your contribution!

@shahamish150294
Copy link
Contributor Author

Thanks!

Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

LinkedFile linkedFile = (LinkedFile) dragboard.getContent(DragAndDropDataFormats.LINKED_FILE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -114,6 +122,21 @@ public EntryEditorPrefsTab(JabRefPreferences prefs) {
builder.add(firstNameModeAbbr, 1, 20);
builder.add(firstNameModeFull, 1, 21);
builder.add(firstNameModeBoth, 1, 22);

final ToggleGroup group = new ToggleGroup();
Label linkFileOptions = new Label(Localization.lang("Options to add file"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe reformulate to Default drag & drop action?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private boolean linkFile;
private boolean renameCopyFile;

public FileDragDropPreferences(boolean copyFile, boolean linkFile, boolean renameCopyFile) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shahamish150294
Copy link
Contributor Author

@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
Copy link
Member

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

@shahamish150294
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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

  1. Users selects "Move & Rename" as default action:
  • No modifier -> Move & Rename
  • Ctrl (= TransferMode.Link) -> Link
  1. 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
Copy link
Contributor Author

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
Copy link
Member

Siedlerchr commented Oct 30, 2018

@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
Copy link
Member

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).
image

@shahamish150294
Copy link
Contributor Author

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.

Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM , thanks for your contribution!

@Siedlerchr
Copy link
Member

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

Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;-)

@tobiasdiez tobiasdiez merged commit 4d2ae94 into JabRef:master Nov 1, 2018
Siedlerchr added a commit that referenced this pull request Nov 2, 2018
* 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)
Siedlerchr added a commit that referenced this pull request Nov 3, 2018
* 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)
Siedlerchr added a commit that referenced this pull request Nov 3, 2018
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants