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.

Sign up for GitHub

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

Jump to bottom

Crash after saving BibTeX source with parsing error #2106

Merged
merged 3 commits into from
Oct 3, 2016
Merged

Crash after saving BibTeX source with parsing error #2106

merged 3 commits into from
Oct 3, 2016

Conversation

grimes2
Copy link
Contributor

@grimes2 grimes2 commented Sep 30, 2016 •
edited
Loading

Fixes #2104.

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)
  • If you changed the localization: Did you run gradle localizationUpdate?

@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 1, 2016
Siedlerchr
Siedlerchr approved these changes Oct 2, 2016
View reviewed changes
Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

Looks good to me

@lenhard
Copy link
Member

lenhard commented Oct 3, 2016

I just tested your PR following the instructions stated in #2104. JabRef does not crash anymore. However, no error message appears and the additional faulty comma is just not stored silently. When I look into the error console, I see the error.

Imho, this is not quite the behavior we want. It is fine to avoid storing the broken BibTeX, but there should be an error message (that does not crash JabRef). Otherwise, a user might expect that the save worked and would be surprised to see that it did not. This is critical, since all changes made to the entry are lost, not just the comma. Therefore, we need the error message so that the user has a change to fix the mistakes.

lenhard
lenhard suggested changes Oct 3, 2016
View reviewed changes
Copy link
Member

@lenhard lenhard left a comment

Choose a reason for hiding this comment

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

There should be a notification to the user about the error

@grimes2
Copy link
Contributor Author

grimes2 commented Oct 3, 2016

I get an error message:
parsingerror
I don't know, why it does not work for you.
Edit keeps the BibTeX source (with parsing error) and gives the opportunity to correct the error.
Revert to original source does not store the changes.

@lenhard
Copy link
Member

lenhard commented Oct 3, 2016

You are correct, my bad! I was hitting CTRL+S and not pressing the button in the toolbar. Your PR can be merged, and apologies for not getting it.

Didn't we use to have a keyboard shortcut for saving?

@lenhard lenhard merged commit 3456255 into JabRef:master Oct 3, 2016
@grimes2
Copy link
Contributor Author

grimes2 commented Oct 3, 2016

Ohh, is this another bug? Ctrl+s should also trigger the error message.

@lenhard
Copy link
Member

lenhard commented Oct 3, 2016

@grimes2 For me it did not. Can you please double check? If you can reproduce it, would you be willing to submit a second pull request?

This could be a keybinding issue though, not directly related to the saving logic.

@grimes2
Copy link
Contributor Author

grimes2 commented Oct 3, 2016

File > Save database works
Ctrl+s fails
Strange!
I'll open a new PR.

@lenhard
Copy link
Member

lenhard commented Oct 3, 2016

Thanks a lot!

@grimes2 grimes2 deleted the parsingfreeze branch October 4, 2016 07:28
Siedlerchr added a commit that referenced this pull request Oct 9, 2016
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into fileDirPattern
5b66fc8 
* upstream/master: (102 commits)
  Removed unused test code (#2140)
  Fix main table bug when creating a duplicate (#2135)
  Remove explicit author and add SPDX-License-Identifier
  Remove GPL from README.md and CONTRIBUTING.md
  fix preview update (#2125)
  Remove some UnicodeToLatex uses (#2132)
  Fix mixup in french/farsi localization
  FetcherException should extend JabRefException
  Fix exception when opening preference dialog (#2127)
  Unify ParserException and ParseException (#2124)
  Small refactoring in Importer package (#2053)
  Implement Datepicker "none"-button (#2122)
  revert change from 816d30c
  Change title/tooltip of source panel in biblatex mode (#2120)
  Refactoring: completey typed metadata and add detailed travis output (#2112)
  RTFchars fix (#2097)
  Fix NPE in Medline fetcher on missing ISSN (#2113)
  Ctrl-s parsing error message (#2114)
  Fix bad web search error messages (#2034)
  parse error freeze (#2106)
  ...

# Conflicts:
#	src/main/java/net/sf/jabref/collab/FileUpdateMonitor.java
#	src/main/java/net/sf/jabref/gui/externalfiles/DownloadExternalFile.java
#	src/main/java/net/sf/jabref/gui/externalfiles/DroppedFileHandler.java
#	src/main/java/net/sf/jabref/gui/externalfiles/MoveFileAction.java
#	src/main/java/net/sf/jabref/logic/cleanup/RenamePdfCleanup.java
#	src/main/java/net/sf/jabref/logic/exporter/FileSaveSession.java
#	src/main/java/net/sf/jabref/logic/util/io/FileUtil.java
#	src/main/java/net/sf/jabref/preferences/JabRefPreferences.java
zesaro pushed a commit to zesaro/jabref that referenced this pull request Nov 22, 2016
@zesaro
parse error freeze (JabRef#2106)
c50cf52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lenhard lenhard lenhard requested changes

@Siedlerchr Siedlerchr Siedlerchr approved these changes

Assignees
No one assigned
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@grimes2 @lenhard @Siedlerchr @tobiasdiez