Fix for issue 6966: open all files of multiple entries #7709

XDZhelheim · 2021-05-07T07:31:43Z

Fixes #6966

Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

Added a new function to "Open file" option in the right-click menu.

Now we can open all linked files of the selected entries.

Test videos:

Single entry selected: the behavior remains unchanged

6966-1.mp4

Multiple entries selected

All of them have linked files: open them all

6966-2.mp4

Some have and some not: ~~pop out a dialog to ask the user whether to continue, if yes,~~ only open the former and skip the latter

6966-3.mp4

None of them have any linked file: option will be disabled

6966-4.mp4

To implement these functions, I modified OpenExternalFileAction and added a new method hasPresentFileForSelectedEntries to check if at least one of the selected entries has linked files in ActionHelper.

update 2021.3.3

update 2021.3.8

update 2021.3.9

update 2021.3.11

update 2021.3.12

update 2021.3.15

update 2021.3.17

update 20213.30

update2021.4.5

update 2021.4.28

Siedlerchr · 2021-05-07T08:32:28Z

Idea for usabilty: As a user I expect to open the file using "open all files" also when I select one entry. So if I only select one entry => same beahviour as now

tobiasdiez · 2021-05-07T12:10:40Z

In line with @Siedlerchr comment above, what do you think about adding the functionality to the existing "open file" action? So if a user has only a single entry selected, then the linked file of this entry is opened; but if multiple files are selected, then all linked files are opened.

XDZhelheim · 2021-05-07T12:20:13Z

I agree. When one entry selected, if "open all files" has the same behavior as "open file" as @Siedlerchr suggested, which means the function of the new added option totally covers the original one, then why not merge them as one option, just as you proposed.

So should I delete OpenAllExternalFileAction class and add its function to OpenExternalFileAction?

tobiasdiez · 2021-05-07T13:01:51Z

Yes, please!

…if there is any linked file present; modified CHANGELOG.md; added javadoc

XDZhelheim · 2021-05-07T13:32:15Z

Done.

I will update detailed description later.

XDZhelheim · 2021-05-07T14:02:53Z

May I ask why these tests are failing?

All these failures have the exactly same error:

/home/runner/work/jabref/jabref/src/test/java/org/jabref/logic/integrity/EditionCheckerTest.java:59: error: cannot find symbol
        assertEquals(Optional.empty(), checker.checkValue(""));
                                       ^
  symbol:   variable checker
  location: class EditionCheckerTest
/home/runner/work/jabref/jabref/src/test/java/org/jabref/logic/integrity/EditionCheckerTest.java:64: error: cannot find symbol
        assertEquals(Optional.empty(), checker.checkValue(null));
                                       ^
  symbol:   variable checker
  location: class EditionCheckerTest
/home/runner/work/jabref/jabref/src/test/java/org/jabref/logic/integrity/EditionCheckerTest.java:69: error: cannot find symbol
        var editionChecker = new EditionChecker(bibtex, false);
                                                ^
  symbol:   variable bibtex
  location: class EditionCheckerTest
/home/runner/work/jabref/jabref/src/test/java/org/jabref/logic/integrity/EditionCheckerTest.java:75: error: cannot find symbol
        var editionChecker = new EditionChecker(bibtex, false);
                                                ^
  symbol:   variable bibtex
  location: class EditionCheckerTest

But I didn't change anything in EditionChecker.

Same as #7705 (comment).

XDZhelheim · 2021-05-10T03:03:22Z

I noticed that the tests are fixed.

So, is there still anything more I need to change in my code?

Siedlerchr · 2021-05-10T16:20:40Z

You need to push the commit after merging as well, because the tests are still failing

Siedlerchr · 2021-05-10T16:45:13Z

src/main/java/org/jabref/gui/maintable/OpenExternalFileAction.java

+ for (BibEntry entry:selectedEntries) {
+ if (entry.getFiles().isEmpty()) {
+ if (!asked) {
+ boolean continu = dialogService.showConfirmationDialogAndWait(Localization.lang("Missing file"),


I, personally, don't think it's necessary to present a dialog here to the user. To me it seems obvious that only the files will open from entries that have associated files
So just ignore them.

XDZhelheim · 2021-05-11T01:56:07Z

Done.

Removed dialog and resolved a conflict in CHANGELOG.md

Siedlerchr · 2021-05-13T10:25:30Z

src/main/java/org/jabref/gui/actions/ActionHelper.java

+ files = entry.getFiles();
+
+ if ((entry.getFiles().size() > 0) && stateManager.getActiveDatabase().isPresent()) {
+ if (files.get(0).isOnlineLink()) {


I would exclude online file links. Otherwise you will suddenly have 300 Tabs open in your browser f you select 300 entries and they have at least one link

What's the difference to selecting 300 entries that have pdf attached?

Should we set a limit?
For example, if a user is trying to open 100 files, pop out a dialog to ask the user whether to continue or cancel.
But how to determine the limit.

Yep, that's a good idea. "You are about to open more than 10 files". Continue?
I would simply create a constant with 10. I think that's appropriate.

Continue / Cancel

Siedlerchr · 2021-05-13T10:26:29Z

src/main/java/org/jabref/gui/actions/ActionHelper.java

+ return true;
+ }
+
+ Optional<Path> filename = FileHelper.find(


No need to check the filename for presence here, just later on try to open the file. Either it works or not.

Siedlerchr · 2021-05-13T10:26:55Z

src/main/java/org/jabref/gui/maintable/OpenExternalFileAction.java

+ LinkedFileViewModel linkedFileViewModel;
+
+ for (BibEntry entry:selectedEntries) {
+ if (!entry.getFiles().isEmpty()) {


An entry can have multiple files linked. I would suggest you open all

Siedlerchr

some improvements necessary

XDZhelheim · 2021-05-13T10:30:45Z

OK. I will fix it.

…inding; question: open a large number of files

XDZhelheim · 2021-05-13T12:11:16Z

Two changes:

open all linked files of an entry
modify boolean binding

Under discussion: how to handle opening a large number of files?

XDZhelheim · 2021-05-13T13:41:09Z

Yep, that's a good idea. "You are about to open more than 10 files". Continue?
I would simply create a constant with 10. I think that's appropriate.

Continue / Cancel

Done.

P.S. Online links are included.

Test video:

6966-5.mp4

tobiasdiez

Thanks, changle look good to me. Two smaller suggestions

src/main/java/org/jabref/gui/actions/ActionHelper.java

src/main/java/org/jabref/gui/maintable/OpenExternalFileAction.java

* upstream/main: (71 commits) [Bot] Update CSL styles (#7735) Fix for issue 6966: open all files of multiple entries (#7709) Add simple unit tests (#7696) Add simple unit tests (#7543) Update check-outdated-dependencies.yml Added preset for new entry keybindings and reintroduced defaults (#7705) Select the entry which has smaller dictonary order when merge (#7708) Update CHANGELOG.md fix: make more fields, fomatters, ids and languages sorted by alphabetical order (#7717) Bump libreoffice from 7.1.2 to 7.1.3 (#7721) Bump unoloader from 7.1.2 to 7.1.3 (#7724) Bump org.beryx.jlink from 2.23.7 to 2.23.8 (#7723) Bump org.openjfx.javafxplugin from 0.0.9 to 0.0.10 (#7725) fix: make fields sorted by lexicographical order (#7711) Fix tests Refactoring existing unit tests (#7687) Refactoring and addition of unit tests (#7581) Refactor simple Unit Tests (#7571) Add simple unit tests (#7544) add and extend unit tests (#7685) ...

XDZhelheim and others added 13 commits March 3, 2021 10:27

Merge pull request #1 from JabRef/master

4851e84

update 2021.3.3

Merge pull request #2 from JabRef/master

ae45307

update 2021.3.8

Merge pull request #3 from JabRef/master

b95ce43

update 2021.3.9

Fix JabRef#7416

fec81da

Merge pull request #5 from JabRef/master

c1434f7

update 2021.3.11

sync with origin

8428b49

Merge pull request #6 from JabRef/master

3fd7a0d

update 2021.3.12

Merge pull request #7 from JabRef/master

5cc3085

update 2021.3.15

Merge pull request #8 from JabRef/master

d3af958

update 2021.3.17

Merge pull request #9 from JabRef/main

9ecdf53

update 20213.30

Merge pull request #10 from JabRef/main

ac7a2ce

update2021.4.5

Merge pull request #22 from JabRef/main

1c7511d

update 2021.4.28

core functions implemented

55490bd

moved function to OpenExternalFileAction; fixed a bug about checking …

6507d71

…if there is any linked file present; modified CHANGELOG.md; added javadoc

XDZhelheim marked this pull request as ready for review May 7, 2021 13:56

XDZhelheim changed the title ~~(WIP) Fix for issue 6966: open all files of multiple entries~~ Fix for issue 6966: open all files of multiple entries May 7, 2021

Siedlerchr requested changes May 10, 2021

View reviewed changes

Siedlerchr added the status: changes required Pull requests that are not yet complete label May 10, 2021

XDZhelheim and others added 2 commits May 11, 2021 09:35

remove dialog

665e73d

Merge branch 'main' into fix-for-issue-6966

7c67917

XDZhelheim requested a review from Siedlerchr May 12, 2021 14:14

Siedlerchr reviewed May 13, 2021

View reviewed changes

Siedlerchr requested changes May 13, 2021

View reviewed changes

(incomplete) open all linked files of an entry; rewrite the boolean b…

92841f9

…inding; question: open a large number of files

Dialog: ask user whether to continue when opening more than 10 files

44d4d64

fixed a bug: wrong position of the codes of dialog

5662cc1

XDZhelheim requested a review from Siedlerchr May 13, 2021 13:59

tobiasdiez requested changes May 13, 2021

View reviewed changes

src/main/java/org/jabref/gui/actions/ActionHelper.java Outdated Show resolved Hide resolved

src/main/java/org/jabref/gui/maintable/OpenExternalFileAction.java Outdated Show resolved Hide resolved

Modified binding; changed a variable name

ec158ab

tobiasdiez approved these changes May 13, 2021

View reviewed changes

Siedlerchr approved these changes May 13, 2021

View reviewed changes

Siedlerchr merged commit 84c8849 into JabRef:main May 13, 2021

Siedlerchr mentioned this pull request May 13, 2021

Fix 6966 multiple file open #7732

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for issue 6966: open all files of multiple entries #7709

Fix for issue 6966: open all files of multiple entries #7709

XDZhelheim commented May 7, 2021 •

edited

Loading

Siedlerchr commented May 7, 2021

tobiasdiez commented May 7, 2021 •

edited

Loading

XDZhelheim commented May 7, 2021 •

edited

Loading

tobiasdiez commented May 7, 2021

XDZhelheim commented May 7, 2021

XDZhelheim commented May 7, 2021 •

edited

Loading

XDZhelheim commented May 10, 2021

Siedlerchr commented May 10, 2021

Siedlerchr May 10, 2021

XDZhelheim commented May 11, 2021

Siedlerchr May 13, 2021 •

edited

Loading

tobiasdiez May 13, 2021

XDZhelheim May 13, 2021

Siedlerchr May 13, 2021

XDZhelheim May 13, 2021

Siedlerchr May 13, 2021

Siedlerchr May 13, 2021

Siedlerchr left a comment

XDZhelheim commented May 13, 2021

XDZhelheim commented May 13, 2021

XDZhelheim commented May 13, 2021

tobiasdiez left a comment

Fix for issue 6966: open all files of multiple entries #7709

Fix for issue 6966: open all files of multiple entries #7709

Conversation

XDZhelheim commented May 7, 2021 • edited Loading

Siedlerchr commented May 7, 2021

tobiasdiez commented May 7, 2021 • edited Loading

XDZhelheim commented May 7, 2021 • edited Loading

tobiasdiez commented May 7, 2021

XDZhelheim commented May 7, 2021

XDZhelheim commented May 7, 2021 • edited Loading

XDZhelheim commented May 10, 2021

Siedlerchr commented May 10, 2021

Siedlerchr May 10, 2021

Choose a reason for hiding this comment

XDZhelheim commented May 11, 2021

Siedlerchr May 13, 2021 • edited Loading

Choose a reason for hiding this comment

tobiasdiez May 13, 2021

Choose a reason for hiding this comment

XDZhelheim May 13, 2021

Choose a reason for hiding this comment

Siedlerchr May 13, 2021

Choose a reason for hiding this comment

XDZhelheim May 13, 2021

Choose a reason for hiding this comment

Siedlerchr May 13, 2021

Choose a reason for hiding this comment

Siedlerchr May 13, 2021

Choose a reason for hiding this comment

Siedlerchr left a comment

Choose a reason for hiding this comment

XDZhelheim commented May 13, 2021

XDZhelheim commented May 13, 2021

XDZhelheim commented May 13, 2021

tobiasdiez left a comment

Choose a reason for hiding this comment

XDZhelheim commented May 7, 2021 •

edited

Loading

tobiasdiez commented May 7, 2021 •

edited

Loading

XDZhelheim commented May 7, 2021 •

edited

Loading

XDZhelheim commented May 7, 2021 •

edited

Loading

Siedlerchr May 13, 2021 •

edited

Loading