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

Error setting field freeze #1863

Merged
merged 9 commits into from
Aug 27, 2016
Merged

Error setting field freeze #1863

merged 9 commits into from
Aug 27, 2016

Conversation

grimes2
Copy link
Contributor

@grimes2 grimes2 commented Aug 26, 2016

  • 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?)

@lenhard
Copy link
Member

lenhard commented Aug 26, 2016

Refs #1757

It is always nice to see a new contributor! :) You took a rather challenging task for your first contribution.

When you consider your PR ready, please indicate this to us by writing a message here. We will set the label "ready-for-review" at the right hand side and we will have a look at your code, comment on things to improve, and try to verify and test that your solution works as desired.

Probably @oscargus should have a look at this.

@grimes2
Copy link
Contributor Author

grimes2 commented Aug 26, 2016

My pull request is ready. Please set the label "ready-for-review".

@lenhard lenhard added bug Confirmed bugs or reports that are very likely to be bugs status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Aug 26, 2016
@oscargus
Copy link
Contributor

Thanks! It clearly solves the problem! However, it also removes the warning when changing field, but I am not sure if this is an improvement or an issue. ;-) A maybe more serious issue is that when selecting another entry, no warning is shown either and the edit is dropped. However, the dropping appears without the fix as well, so again, the question is what is the worst.

One thing is clear, that even though this fix solves the crash problem, which is clearly significant, we really need to look into this automatic field storage/warning mechanism... Also, the red color only flashes by in some situations, although the dialog shows up, so many events happening more or less at the same time. I guess I'm not missing many brackets as I haven't really notices this earlier...

@JabRef/developers : crash on rare occasions or missing warnings slightly more often? I'd say missing warnings.
@grimes2 : feel free to add a change log entry, if not, I'll do it later.

@grimes2
Copy link
Contributor Author

grimes2 commented Aug 26, 2016

I can add a else branch with the red background color only, for the EDT Events. A error message is impossible, because JOptionPane is not thread-safe.

@oscargus
Copy link
Contributor

Yes, either that or set the background outside of the if. Another good thing with the fix is that only one dialog is shown for the save toolbar button. 👍

@lenhard
Copy link
Member

lenhard commented Aug 26, 2016

@oscargus Missed warnings are certainly preferable to application crashes. Moreover, I can confirm the change losses when selecting a new entry. I will open a new issue for that.

I have tested this PR locally and can confirm that it fixes #1757. The coloring also seems to be consistent after the last commit.

All that is missing for merging is the Changelog entry. This is a little complicated since we had the intermediate release of 3.6. @grimes2: It would be best if you merge master into your branch before editing the Changelog. This should carry over the release data.

@grimes2
Copy link
Contributor Author

grimes2 commented Aug 26, 2016

I don't know, how to merge master into branch.

@Siedlerchr
Copy link
Member

If you have setup your local git repository (look here for details on how to https://help.github.com/articles/fork-a-repo/)
git fetch upstream
git merge upstream/master
Resolve any conflicts
git push (best done over the git gui)

https://github.com/JabRef/jabref/wiki/Guidelines-for-setting-up-a-local-workspace

@oscargus
Copy link
Contributor

Great, thanks! I merge this in.

@oscargus oscargus merged commit cbef5a6 into JabRef:master Aug 27, 2016
@oscargus
Copy link
Contributor

oscargus commented Aug 27, 2016

@grimes2 Feel free to open a PR with your name added in https://github.com/JabRef/jabref/blob/master/AUTHORS

We forgot about that. Not so used with new contributors... 👍

@stefan-kolb
Copy link
Member

This is autogenerated from the git history @oscargus

@oscargus
Copy link
Contributor

Ah, OK! Thanks @stefan-kolb .

This was referenced Aug 29, 2016
ayanai1 pushed a commit to ayanai1/jabref that referenced this pull request Sep 5, 2016
* Error setting field freeze

* Update EntryEditor.java

* Background color

* Typo

* Another Typo

* color before if

* CHANGELOG edited
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bugs or reports that are very likely to be bugs 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.

5 participants