Add error dialog "Problem finding files" by IfIWantedTo · Pull Request #6842 · JabRef/jabref

IfIWantedTo · 2020-09-02T11:16:21Z

I added an Error Dialog in AutoSetFileLinks. #5890
I decided to open a pull request in order to get feedback on what I've done.

This is my first contribution to OpenSource ever, so beware of stupid mistakes.

I am currently trying to go through the motions of contributing. However, I have run into a
few issues:

 `ERROR org.jabref.logic.l10n.Localization - Messages are not initialized before accessing key: Your current Java version (%0) is not supported. Please install version %1 or higher. - `

Change in CHANGELOG.md described (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.

AutoSetFileLinksUtil: -> imported JabRefDialogService and Localization ->showErrorDialogAndWait(...) I am currently having trouble writing a test

Siedlerchr · 2020-09-03T10:08:37Z

@IfIWantedTo Welcome to JabRef!
I will try to give you some context.
Regarding the localization error, this seems your IDE configuration is not correct. Please check with the guide here
https://devdocs.jabref.org/getting-into-the-code/guidelines-for-setting-up-a-local-workspace#configure-your-ide

Back to the code, it's a bit more complicated then simply adding a dialog there.
The method has a loop so therefore a dialog would be a not a good idea for all files

I would propose to create a list with the exceptions in that class
Then in the AutoLinkFilesAction in the succeed method of the linksFilesTask you can check for that list if it has any exceptions and report a notifications (the notify method shows a toast message that there were errors and say sth like go to the log for more details)

Fixed mistake in first commit. .lists all exceptions .notifies user of error via DialogService

IfIWantedTo · 2020-09-03T11:13:06Z

@Siedlerchr Thank u for your help!

I have completely missed the loop before, thank you! I have replaced the code before with an iteration of
pushing to a list of exceptions as you suggested. I will still have to figure out where exactly to put the calling of notify() but I'm sure I will figure that out once I'm actually able to run the code.

Regarding the IDE: I figured there was a problem with the configuration and I went over the guide multiple times.
When it didn't seem to help I disregarded it. Now that you have confirmed my former suspicions I will resume my efforts.

Again, thank you for your comment! Without you, this would have taken much longer :)

Siedlerchr · 2020-09-03T11:21:06Z

            }

            if (ce != null) {
+                if (!fileExceptions.isEmpty()) { // is this where it goes? find out!


I would propose to move this into the succeeded methoid here :

jabref/src/main/java/org/jabref/gui/externalfiles/AutoLinkFilesAction.java

Lines 60 to 68 in 5ab494e

Task<List<BibEntry>> linkFilesTask = new Task<>() {

@Override

protected List<BibEntry> call() {

return util.linkAssociatedFiles(entries, nc);

}

@Override

protected void succeeded() {

Oh ok, thank you. Can I ask you a few questions about the succeeded() method?

As far as I understood, it notifies the user when it has set the file links, i.e. when the returned List of linkAssociatedFiles(...) is not empty. In the other case Finished automatically setting external links. No files found. I haven't quite understood yet, how this else case is different from what I am trying to accomplish.

Sorry for the delay in answering. The succeeded method is called whenever the task is finished normally and doesn't get interrupted.
It either says files found or no files found, but it doesn't tell the user if there was an error (exception) which prevented finding the files.
So one addtional else case (your changes) is to notify the users that there have been X exceptions with the hint
e.g. There were x files with an exception. See log for details

- add returnFileExceptions() in AutoSetFileLinksUtil - add if ( 'there are exceptions' ) -> notify user in AutoSetFileLinkAction (placement still unclear, might be changed later)

…into IfIWantedTo-fix-issue-5890

…ssue-5890

calixtus · 2020-12-15T23:18:06Z

I tried to finish this PR. Just did not had a file at hand to test the actual auto link, but every other case. So i think this should work, since I did not change the actual logic of the method. So ready for review.

tobiasdiez

Thanks @IfIWantedTo for your contribution and @calixtus for finishing this PR.

…tedTo-fix-issue-5890

…dtask * upstream/master: add language mapping for chinese remove chinese content fix hamcrest link Add Traditional Chinese (#7240) Show development information Allow manual trigger of the deployment workflow Release v5.2 Adapt changelog for 5.2 release Update external-libraries.md checkstyle L10n master (#7235) fix fetcher architecture test Add missing author Add error dialog "Problem finding files" (#6842) Disable ACM, Google Scholar, JSTOR (#7229)

IfIWantedTo and others added 2 commits September 2, 2020 12:53

JabRef#5890 added Error Dialog

d4bd594

AutoSetFileLinksUtil: -> imported JabRefDialogService and Localization ->showErrorDialogAndWait(...) I am currently having trouble writing a test

Merge branch 'master' into fix-issue-5890

5d77abb

Fix errorDialog AutoSetFileLinks

f8a3175

Fixed mistake in first commit. .lists all exceptions .notifies user of error via DialogService

Siedlerchr reviewed Sep 3, 2020

View reviewed changes

Fix notifying in AutoSetFileLinksAction

d8d70d2

- add returnFileExceptions() in AutoSetFileLinksUtil - add if ( 'there are exceptions' ) -> notify user in AutoSetFileLinkAction (placement still unclear, might be changed later)

Siedlerchr reviewed Sep 29, 2020

View reviewed changes

Comment thread src/main/java/org/jabref/gui/externalfiles/AutoLinkFilesAction.java Outdated

calixtus added 3 commits September 29, 2020 08:47

Fixed checkstyle error in AutoSetFileLinksUtilTest

90ab478

Fixed import order in JabRefFrame

ee5e7cf

Update JabRefFrame.java

355f6ff

koppor assigned calixtus Nov 23, 2020

calixtus added 4 commits December 12, 2020 18:55

Merge branch 'fix-issue-5890' of https://github.com/IfIWantedTo/jabref …

e765f26

…into IfIWantedTo-fix-issue-5890

Merge remote-tracking branch 'upstream/master' into IfIWantedTo-fix-i…

dbedcde

…ssue-5890

Merge remote-tracking branch 'upstream/master' into IfIWantedTo-fix-i…

445ca39

…ssue-5890

Refactored to fail fast

fac9c37

calixtus marked this pull request as ready for review December 15, 2020 23:16

calixtus added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 15, 2020

Update JabRef_en.properties

c351314

tobiasdiez requested changes Dec 16, 2020

View reviewed changes

tobiasdiez added the status: changes-required Pull requests that are not yet complete label Dec 16, 2020

calixtus added 3 commits December 18, 2020 14:43

Included remarks

36d8140

Merge remote-tracking branch 'IfIWantedTo/fix-issue-5890' into IfIWan…

fc07f28

…tedTo-fix-issue-5890

Merge error

12fefda

calixtus removed the status: changes-required Pull requests that are not yet complete label Dec 18, 2020

Siedlerchr approved these changes Dec 19, 2020

View reviewed changes

Siedlerchr merged commit 574ca59 into JabRef:master Dec 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add error dialog "Problem finding files"#6842

Add error dialog "Problem finding files"#6842
Siedlerchr merged 15 commits into
JabRef:masterfrom
IfIWantedTo:fix-issue-5890

IfIWantedTo commented Sep 2, 2020 •

edited

Loading

Uh oh!

Siedlerchr commented Sep 3, 2020

Uh oh!

IfIWantedTo commented Sep 3, 2020

Uh oh!

Siedlerchr Sep 3, 2020 •

edited

Loading

Uh oh!

IfIWantedTo Sep 4, 2020

Uh oh!

Siedlerchr Sep 25, 2020

Uh oh!

Uh oh!

calixtus commented Dec 15, 2020

Uh oh!

tobiasdiez left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	Task<List<BibEntry>> linkFilesTask = new Task<>() {

	@Override
	protected List<BibEntry> call() {
	return util.linkAssociatedFiles(entries, nc);
	}

	@Override
	protected void succeeded() {

Uh oh!

Conversation

IfIWantedTo commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Siedlerchr commented Sep 3, 2020

Uh oh!

IfIWantedTo commented Sep 3, 2020

Uh oh!

Siedlerchr Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IfIWantedTo Sep 4, 2020

Choose a reason for hiding this comment

Uh oh!

Siedlerchr Sep 25, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

calixtus commented Dec 15, 2020

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

IfIWantedTo commented Sep 2, 2020 •

edited

Loading

Siedlerchr Sep 3, 2020 •

edited

Loading