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

Fix IndexOutOfBounds exceptions #3390

Closed
wants to merge 3 commits into from
Closed

Fix IndexOutOfBounds exceptions #3390

wants to merge 3 commits into from

Conversation

tobiasdiez
Copy link
Member

This should fix (most) IndexOutOfBounds exceptions in the entry editor. The problem is that the source code editor cannot handle updates when it is hidden. (In particular, there is no problem with the bindings etc....it is just a bug in the library we use; for normal text fields there is absolutely no problem in having an active binding while the control is hidden). In order to circumvent this problem, the source tab now removes the code editor as soon as it looses focus.

  • 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 Nov 2, 2017
@lenhard
Copy link
Member

lenhard commented Nov 2, 2017

I just tested this locally in a few settings and I have not experienced exceptions or freezes. The problem described in 2. here still persists #3366 (comment) When you open the source tab and click somewhere, the cursor seems to stay at the initial position. But when you enter the text, the text is inserted at the position you clicked. So it seems that the cursor has indeed moved, but is just not displayed at the right position. That is ugly, but not as bad as the freezes. So if this PR really solves the freezes, then I am in favor of merging.

It would be great if sthe users who reported problems with the source tab in the prior PR could give this branch some testing. @jonasstein @sambo57u could you please give the source tab a try with the version available here: http://builds.jabref.org/storeSourceFix/

@jonasstein
Copy link

jonasstein commented Nov 2, 2017

@lenhard this version does not freeze, if I drag an article in a group.

The source tab does sometimes not move the curser, where I click the mouse to. I can not reproduce when this happens. I click on the 2nd line in the bib code and it fails. Then I click around in the tabs on the bottom try again and it works. Then I click around in the tabs on the bottom try again and it eventually fails again.

Removing {\hspace{0.167em}} from the authors in https://journals.aps.org/prl/abstract/10.1103/PhysRevLett.119.177201 works now too.

JabRef 4.1-dev--snapshot--2017-11-02--storeSourceFix--598abe18d
Linux 4.12.12-gentoo+ amd64 
Java 1.8.0_152

@lenhard
Copy link
Member

lenhard commented Nov 2, 2017

@jonasstein Thanks for reviewing! This version does not freeze when you drag an article in a group?!?! That sounds good, but the code changes here have no relation whatsoever to the groups panel. I am unsure what to make of this observation.

@jonasstein
Copy link

The 4.0 release freezes. Did you expect that this version freezes too?

@lenhard
Copy link
Member

lenhard commented Nov 2, 2017

From your comments in other issues, I thought that the master version also freezes. I am glad to hear that this was a misunderstanding.

@lenhard
Copy link
Member

lenhard commented Nov 2, 2017

After playing around some more, I have discovered a problem: Pasting into the source pane does not work. Also marking text through keeping the mouse pressed or hitting ctrl+a does not work. It actually marks all text, but that is not visible.

The real problem is this:

  1. When I mark all text in the source tab
  2. and paste a valid entry into it,
  3. the pasted entry is displayed, but the database is not marked as changed
  4. and as soon as I switch to something else, all changes in the source tab are lost (even if I saved the database in between).

Somehow it seems that the source tab does not register that text has changed through the paste. @tobiasdiez Any chance of looking into this as part of this PR?

@tobiasdiez
Copy link
Member Author

@lenhard Both of these issues seem to also be present in the current master. Since I'm not sure when I'll find the time to fix them (and the indexoutofbounds exceptions are really a pain in the *** for the user), I would prefer if this PR could be merged with its current scope.

@lenhard
Copy link
Member

lenhard commented Nov 3, 2017

Ok, I understand. Looks like there are still some errors left, e.g. #3366 (comment)

We could merge this, since it is at least an improvement. The source tab is still not bug-free, though.

@JabRef/developers Could a second person please give this a review?

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.

Codewise it's okay, and I am in favor of merging. A huge step in supressing the exceptions

@tobiasdiez
Copy link
Member Author

Actual fix comes in #3398

@tobiasdiez tobiasdiez closed this Nov 4, 2017
@tobiasdiez tobiasdiez deleted the storeSourceFix branch November 4, 2017 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants