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

Make sure that unregistered event sources do not stop JabRef from shu… #2487

Merged
merged 4 commits into from
Jan 26, 2017

Conversation

lenhard
Copy link
Member

@lenhard lenhard commented Jan 24, 2017

…tting down

Fixes #2486. Unregistering event sources that have not been registered before should not stop JabRef from shutting down orderly. This PR logs such a problem instead of halting the shutdown of the program

  • Change in CHANGELOG.md described
  • Manually tested changed features in running JabRef

@Siedlerchr
Copy link
Member

Would it be possible to first check if the event is registered and only doing a shutdown then?
This should be implemented at best in unregister itself

@lenhard
Copy link
Member Author

lenhard commented Jan 25, 2017

@Siedlerchr: Unfortunately, the implementation of the event bus (from Google) supports no such checking functionality. Hence, we would have to remember subscribers in a custom list, which in my point of view would be an unnecessary overhead. However, I have now moved the exception handling code to all the different event sources in JabRef (I hope I found them all). That way, we catch the Exception as early as possible. I cannot think of a circumstance where trying to remove a listener that never listened should crash the complete application.

It is not always logged, though, since not all of the classes that are event sources use a logger. But again, I do not think that this is very tragic, or indeed important.

@Siedlerchr
Copy link
Member

There is already discussion about checking it ... google/guava#784
For fixing check style you should add the comments for switching it off at hat position;
http://stackoverflow.com/questions/4023185/how-to-disable-a-particular-checkstyle-rule-for-a-particular-line-of-code?noredirect=1&lq=1

@lenhard
Copy link
Member Author

lenhard commented Jan 25, 2017

@Siedlerchr Sorry, but I am not getting your comment regarding checkstyle.

The builds seem to fail because of the usual random fetcher errors.

@Siedlerchr
Copy link
Member

Siedlerchr commented Jan 25, 2017

@lenhard I meant the codacy, which will complain about the empty catch blocks.
But I think I confused it with checkstyle module

@lenhard
Copy link
Member Author

lenhard commented Jan 25, 2017

@Siedlerchr Ok, I understand. However, I am not too fond of codacy and refuse to add comments, just because the tool produces false positives. I am well aware that it is near impossible for a tool to tell if an empty catch block makes sense or not, but it is false positive nonetheless. The existing comment in there is meaningful to humans, which is way more important.

I just pushed a few minor formatting fixes.

# Conflicts:
#	CHANGELOG.md
@lenhard
Copy link
Member Author

lenhard commented Jan 26, 2017

I'll take the "thumps-up" as an "ok-for-merge", which I'll do right away.

@lenhard lenhard merged commit ed89bda into master Jan 26, 2017
@lenhard lenhard deleted the fix-shutdown branch January 26, 2017 12:35
Siedlerchr added a commit that referenced this pull request Jan 26, 2017
* upstream/master:
  Fix medline tests...again (#2492)
  Make sure that unregistered event sources do not stop JabRef from shu… (#2487)
  Fix #2481: ClassCastException because of wrong cast (#2490)
  Catch NumberFormatException if context can't be parsed in groups (#2488)
  Improve CHANGELOG formatting
  Update guava from 20.0 to 21.0 and mockito-core from 2.5.5 to 2.6.2
  Fix aux duplicates (#2480)
  add update from DOI to the entryeditor sidebar (#2476)
  Remove obsolete import
  Add CHANGELOG entry (and one more link)
  Resolves #2309 JabRef freezes when importing unlinked PDF files into Database
  Update CHANGELOG.md
  Fixed bug when assigning refs to groups.
  emove html code from ACM fetcher before calling parser to prevent junk in bib file (#2473)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants