-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Fix Permissions of LaTeXintegration #5134
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, two small sugestions from my side
searchInProgress.set(false); | ||
successfulSearch.set(false); | ||
|
||
final boolean permissionProblem = exception instanceof IOException && exception.getCause() instanceof FileSystemException && exception.getCause().getMessage().endsWith("Operation not permitted"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to move this code to dialogService.showErrorDialogAndWait(exception)
so that access issues in other parts of JabRef are treated similarly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't do that because the dialogService
is a global thing, whereas how we handle the exception here is quite special.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, so the same exception but e.g. thrown while opening a bib file should still be displayed in the default error dialog with the stack trace instead of by the specialized permission problem dialog?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least add a complete error message with stack trace to the log. I hate error messages without stack traces.
src/main/java/org/jabref/gui/texparser/ParseTexDialogViewModel.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Although I think that some lines are redundant. It is explained over the code.
src/main/java/org/jabref/gui/texparser/ParseTexDialogViewModel.java
Outdated
Show resolved
Hide resolved
* upstream/master: Bugfix/5045 : Modified the existing logic to comply crossref resolution with biblatex specification (JabRef#5086) Fix issue with missing year value in year column (JabRef#5197) Bump com.gradle.build-scan from 2.4 to 2.4.1 (JabRef#5199) Add citation commands to TexParser: autocite, blockcquote, and textcquote (JabRef#5195) Conversion of preferencesDialog/advancedTab, networkTab and groupsTab to mvvm (JabRef#5141) Fix Permissions of LaTeXintegration (JabRef#5134) Border for group color indicator and some space for tooltip (JabRef#5190) Fix issue 5152, tooltip and icon added to group cell (JabRef#5191) Fix tooltips in CitationsDisplay (JabRef#5188)
This fixes one problem of JabRef trying to parse a directory, which it has no permission for.
Instead a generic exception dialog with a StackTrace, a more useful error message is displayed to the users.