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

Improvement on check for missing commas when importing or opening a .bib file #8840

Merged
merged 8 commits into from
Jul 9, 2022

Conversation

LIM0000
Copy link
Contributor

@LIM0000 LIM0000 commented May 23, 2022

Previous discussion about check for missing commas when importing or opening a .bib file #8011

  • When importing the bib file, the following exception stack trace pops up:
    org.jabref.logic.importer.ImportException: Could not find a suitable import format.
    at [email protected]/org.jabref.logic.importer.ImportFormatReader.importUnknownFormat(Unknown Source)
    at [email protected]/org.jabref.gui.importer.ImportAction.doImport(Unknown Source)
    at [email protected]/org.jabref.gui.importer.ImportAction.lambda$automatedImport$1(Unknown Source)
    at [email protected]/org.jabref.gui.util.BackgroundTask$1.call(Unknown Source)
    at [email protected]/org.jabref.gui.util.DefaultTaskExecutor$1.call(Unknown Source)
    at [email protected]/javafx.concurrent.Task$TaskCallable.call(Unknown Source)
    at java.base/java.util.concurrent.FutureTask.run(Unknown Source)
    at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Unknown Source)
    at java.base/java.util.concurrent.FutureTask.run(Unknown Source)
    at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source)
    at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)
    at java.base/java.lang.Thread.run(Unknown Source)
  • When opening the library normally, there is no error message. It is just shown to be empty. No message, no error, nothing.

Proposal solution: Display warning dialog with meaningful message instead of error dialog to user

Fixes: #8011

  1. Display meaningful dialog by getting error message from parserResult.
throw new ImportException(parserResult.getErrorMessage());
  1. Check if is instanceof ImportExpection, display warning dialog instead of error dialog
} else if (importError instanceof ImportException) {
                    DefaultTaskExecutor.runAndWaitInJavaFXThread(() -> dialogService.showWarningDialogAndWait(Localization.lang("Import error"), importError.getLocalizedMessage()));

111

Further discussion:

Add this implementation to opening of file.

PR checklist:

  • 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 developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@LIM0000 LIM0000 changed the title Add validation that check for missing commas when importing or opening a .bib file Improvement on check for missing commas when importing or opening a .bib file May 23, 2022
throw new ImportException(Localization.lang("Could not find a suitable import format."));
throw new ImportException(parserResult.getErrorMessage());
Copy link
Member

Choose a reason for hiding this comment

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

I like this change

Comment on lines 80 to 81
} else if (importError instanceof ImportException) {
DefaultTaskExecutor.runAndWaitInJavaFXThread(() -> dialogService.showWarningDialogAndWait(Localization.lang("Import error"), importError.getLocalizedMessage()));
Copy link
Member

Choose a reason for hiding this comment

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

This is "only" to show a warning instead of an error?

What happens if you remove that code? Is JabRef showing an error? Then I would vote to remove this check

@Siedlerchr
Copy link
Member

@LIM0000 What's the status here?

@LIM0000
Copy link
Contributor Author

LIM0000 commented Jun 21, 2022

Hi @Siedlerchr , this PR is still pending for #8011 (comment) and #8011 (comment).

@Siedlerchr
Copy link
Member

grafik

Please also change the dialog when leaving the source tab

@ThiloteE
Copy link
Member

Additionally, as I was arguing in #8011 (comment), having different error messages depending on what the user was doing would be nice.

Those are the error messages I came up with

  • When user input via entry editor in {}bibtex source tab:

    User input via entry-editor in `{}bibtex source` tab led to failure.
    Please check your library file for wrong syntax.
    
    Error occurred when parsing entry:
    Error in line 11 or above: Empty text token.
    Typical cause: Missing comma between two fields.
    
    JabRef skipped the entry.
    
  • When importing library file:

    Please check your library file for wrong syntax.
    
    Error occurred when parsing entry:
    Error in line 11 or above: Empty text token.
    Typical cause: Missing comma between two fields.
    
    JabRef skipped the entry.
    
  • When opening library file:

    Please check your library file for wrong syntax.
    
    Error occurred when parsing entry:
    Error in line 11 or above: Empty text token.
    Typical cause: Missing comma between two fields.
    
    JabRef skipped the entry.
    

The blank lines in between sentences is intentional to prevent "wall of text".

@ThiloteE ThiloteE added the status: changes required Pull requests that are not yet complete label Jun 22, 2022
@LIM0000 LIM0000 marked this pull request as ready for review July 4, 2022 03:19
@ThiloteE ThiloteE added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed status: changes required Pull requests that are not yet complete labels Jul 9, 2022
* upstream/main:
  Disable ResearchGateTest on CI (JabRef#8955)
  Bump checkstyle from 10.1 to 10.3.1 (JabRef#8950)
  Bump checkstyle from 10.1 to 10.3.1 (JabRef#8950)
  Bump mariadb-java-client from 2.7.5 to 2.7.6 (JabRef#8953)
  Add notification when adding previous entries to new group configuration (JabRef#8919)
  Remember Sidepane width after restart (JabRef#8936)
  move "Warn about duplicates on import" preferences option (JabRef#8937)
  Fix theme switching exception (JabRef#8939)
  fix merge conflict
  Squashed 'buildres/csl/csl-locales/' changes from 969d9567ac..55459cd79f
  Squashed 'buildres/csl/csl-styles/' changes from e740261..3d3573c
  Show pdf icon for mime type in maintable (JabRef#8935)
  Fix for Dark theme not readable (JabRef#8929)
  Find Unlinked files should ignore Thumbs.db, etc (JabRef#8800)
  Try to  make reproducible builds (JabRef#8925)
  Link to new board (and refine text)
@Siedlerchr Siedlerchr merged commit 8bd1845 into JabRef:main Jul 9, 2022
Siedlerchr added a commit that referenced this pull request Jul 10, 2022
* upstream/main:
  Keep UTF-8 encoding header if present (#8964)
  Cleanup index when opening a library (#8962)
  Refactor context menu entry types changing (#8957)
  Improvement on check for missing commas when importing or opening a .bib file (#8840)

# Conflicts:
#	src/test/java/org/jabref/logic/importer/fileformat/BibtexImporterTest.java
Siedlerchr added a commit that referenced this pull request Jul 14, 2022
…failure-dialog

* upstream/main: (76 commits)
  New Crowdin updates (#8972)
  New Crowdin updates (#8969)
  Fix .bat errorlevel handling with pwsh.exe check (#8965)
  revert jsoup version upgrade
  Fix charset detection with utf16 and others (#8947)
  Bump classgraph from 4.8.147 to 4.8.149 (#8968)
  Bump jsoup from 1.15.1 to 1.15.2 (#8967)
  Keep UTF-8 encoding header if present (#8964)
  Cleanup index when opening a library (#8962)
  Refactor context menu entry types changing (#8957)
  Improvement on check for missing commas when importing or opening a .bib file (#8840)
  Disable ResearchGateTest on CI (#8955)
  Bump checkstyle from 10.1 to 10.3.1 (#8950)
  Bump checkstyle from 10.1 to 10.3.1 (#8950)
  Bump mariadb-java-client from 2.7.5 to 2.7.6 (#8953)
  Add notification when adding previous entries to new group configuration (#8919)
  Remember Sidepane width after restart (#8936)
  move "Warn about duplicates on import" preferences option (#8937)
  Fix theme switching exception (#8939)
  fix merge conflict
  ...

# Conflicts:
#	src/main/resources/l10n/JabRef_en.properties
Siedlerchr added a commit to LIM0000/jabref that referenced this pull request Jul 15, 2022
* upstream/main: (28 commits)
  [Bot] Update CSL styles (JabRef#8978)
  New Crowdin updates (JabRef#8972)
  New Crowdin updates (JabRef#8969)
  Fix .bat errorlevel handling with pwsh.exe check (JabRef#8965)
  revert jsoup version upgrade
  Fix charset detection with utf16 and others (JabRef#8947)
  Bump classgraph from 4.8.147 to 4.8.149 (JabRef#8968)
  Bump jsoup from 1.15.1 to 1.15.2 (JabRef#8967)
  Keep UTF-8 encoding header if present (JabRef#8964)
  Cleanup index when opening a library (JabRef#8962)
  Refactor context menu entry types changing (JabRef#8957)
  Improvement on check for missing commas when importing or opening a .bib file (JabRef#8840)
  Disable ResearchGateTest on CI (JabRef#8955)
  Bump checkstyle from 10.1 to 10.3.1 (JabRef#8950)
  Bump checkstyle from 10.1 to 10.3.1 (JabRef#8950)
  Bump mariadb-java-client from 2.7.5 to 2.7.6 (JabRef#8953)
  Add notification when adding previous entries to new group configuration (JabRef#8919)
  Remember Sidepane width after restart (JabRef#8936)
  move "Warn about duplicates on import" preferences option (JabRef#8937)
  Fix theme switching exception (JabRef#8939)
  ...

# Conflicts:
#	src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java
Siedlerchr added a commit to Ognimalf/jabref that referenced this pull request Jul 18, 2022
* upstream/main: (25 commits)
  Update Gradle Wrapper from 7.4.2 to 7.5. (JabRef#8986)
  New translations JabRef_en.properties (Russian) (JabRef#8985)
  [Bot] Update CSL styles (JabRef#8978)
  New Crowdin updates (JabRef#8972)
  New Crowdin updates (JabRef#8969)
  Fix .bat errorlevel handling with pwsh.exe check (JabRef#8965)
  revert jsoup version upgrade
  Fix charset detection with utf16 and others (JabRef#8947)
  Bump classgraph from 4.8.147 to 4.8.149 (JabRef#8968)
  Bump jsoup from 1.15.1 to 1.15.2 (JabRef#8967)
  Keep UTF-8 encoding header if present (JabRef#8964)
  Cleanup index when opening a library (JabRef#8962)
  Refactor context menu entry types changing (JabRef#8957)
  Improvement on check for missing commas when importing or opening a .bib file (JabRef#8840)
  Disable ResearchGateTest on CI (JabRef#8955)
  Bump checkstyle from 10.1 to 10.3.1 (JabRef#8950)
  Bump checkstyle from 10.1 to 10.3.1 (JabRef#8950)
  Bump mariadb-java-client from 2.7.5 to 2.7.6 (JabRef#8953)
  Add notification when adding previous entries to new group configuration (JabRef#8919)
  Remember Sidepane width after restart (JabRef#8936)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
entry-editor import Logging 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.

Check for missing commas when importing or opening a .bib file
4 participants