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 error dialog "Problem finding files" #6842

Merged
merged 15 commits into from
Dec 24, 2020

Conversation

IfIWantedTo
Copy link
Contributor

@IfIWantedTo IfIWantedTo commented Sep 2, 2020

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.

IfIWantedTo and others added 2 commits September 2, 2020 12:53
AutoSetFileLinksUtil:
-> imported JabRefDialogService and Localization
->showErrorDialogAndWait(...)

I am currently having trouble writing a test
@Siedlerchr
Copy link
Member

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

@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 :)

}

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

@Siedlerchr Siedlerchr Sep 3, 2020

Choose a reason for hiding this comment

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

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

Task<List<BibEntry>> linkFilesTask = new Task<>() {
@Override
protected List<BibEntry> call() {
return util.linkAssociatedFiles(entries, nc);
}
@Override
protected void succeeded() {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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)
@calixtus calixtus marked this pull request as ready for review December 15, 2020 23:16
@calixtus
Copy link
Member

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.

@calixtus calixtus added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 15, 2020
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 @IfIWantedTo for your contribution and @calixtus for finishing this PR.

@tobiasdiez tobiasdiez added the status: changes required Pull requests that are not yet complete label Dec 16, 2020
@calixtus calixtus removed the status: changes required Pull requests that are not yet complete label Dec 18, 2020
@Siedlerchr Siedlerchr merged commit 574ca59 into JabRef:master Dec 24, 2020
Siedlerchr added a commit that referenced this pull request Dec 25, 2020
…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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants