Skip to content

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 org.jabref@5.4.76/org.jabref.logic.importer.ImportFormatReader.importUnknownFormat(Unknown Source)
    at org.jabref@5.4.76/org.jabref.gui.importer.ImportAction.doImport(Unknown Source)
    at org.jabref@5.4.76/org.jabref.gui.importer.ImportAction.lambda$automatedImport$1(Unknown Source)
    at org.jabref@5.4.76/org.jabref.gui.util.BackgroundTask$1.call(Unknown Source)
    at org.jabref@5.4.76/org.jabref.gui.util.DefaultTaskExecutor$1.call(Unknown Source)
    at org.jabref.merged.module@5.4.76/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
return new UnknownFormatImport(ImportFormatReader.BIBTEX_FORMAT, parserResult);
} else {
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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