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

Additional check if values have changed in bindToEntry #6528

Merged
merged 5 commits into from
Jul 17, 2020
Merged

Conversation

calixtus
Copy link
Member

@calixtus calixtus commented May 26, 2020

Fixes #5904

  • Change in CHANGELOG.md described (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 documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

@calixtus
Copy link
Member Author

Not yet mergeable, it's still in testing. I'm not sure if this really solved the problem. I'm waiting for @gianlucabaldassarre to answer in the issue thread.

@Siedlerchr
Copy link
Member

Seesms not to work.
Type randomly in the comment field and create new lines etc and type again
hit ctrl + s cursor jumps again to the beginning

@gianlucabaldassarre
Copy link

gianlucabaldassarre commented May 28, 2020 via email

@calixtus
Copy link
Member Author

@Siedlerchr just put me on a track. The issue occurs now, if you save after creating a new line... In saving, the fields are trimmed of whitespaces...

@calixtus
Copy link
Member Author

This should do the trick. Let's wait for the deployment.

@Siedlerchr Siedlerchr marked this pull request as ready for review May 28, 2020 11:57
@tobiasdiez
Copy link
Member

Mhhh, this looks dangerous. Not sure what consequences could arise when the field value and textfield value are different (even if it just whitespace difference). What about moving the trim from saving to the set field value method in BibEntry? Ok, maybe this also doesn't work since then users cannot type anything with spaces at the end because they are immediately deleted... damn.

Another idea: preserve the caret position by hand as in https://stackoverflow.com/a/45334996/873661 (if they really differ only by trimmed whitespace, the new caret position should be easy to calculate)

@Siedlerchr
Copy link
Member

Siedlerchr commented May 28, 2020

Tested it from eclipse, does not change anything, caret still jumps to the beginning

@gianlucabaldassarre
Copy link

gianlucabaldassarre commented May 28, 2020

System:
JabRef 5.1-PullRequest6528.455--2020-05-26--0965648
Windows 10 10.0 amd64
Java 14.0.1

...If it can help with the problem (sorry if this is naive!):
By directly typing into the multi-line tab "{} biblatex source" (with 1 field) the problem of jumping is not there. Maybe you could use the same code used there for the fields showing the problem?

Moreover, I just discovered that typing into the multi-line fields as "Abstract" and "Comment" (those with the problem) you have you have a lag for which if you type fast you cannot see what you are typing: a problem you do not have when directly typing within "{} biblatex source" where typing is a much smoother (normal) experience.

@calixtus
Copy link
Member Author

Next round of testing: https://builds.jabref.org/pull/6528/merge/ 😁
Thank you all for your patience. This bug seems to be somewhat tricky to catch.

@calixtus
Copy link
Member Author

Expected behaviour should be now after 8bcc65b:
While editing the contents of the editor field should always remain the same, including whitespaces etc. After changing the entry and saving, the contents are trimmed.

@gianlucabaldassarre Thanks for the note. The source editor is using a different kind of entry editor, called richtextfx (or to be exact CodeArea). This editor has higher demands on memory and processor time. So we try to use this only when necessary.

@Siedlerchr
Copy link
Member

Hm. I tested your latest changes on this branch, for me it's still happening

@calixtus
Copy link
Member Author

I really don't get it. It worked for me, maybe you something I don't see yet.
Can you please describe step by step what you are doing?

@Siedlerchr
Copy link
Member

Siedlerchr commented May 28, 2020

I recorded a gif

I started JabRef from eclipse with addtional command line parameter --debug

jabref_comment

@tobiasdiez
Copy link
Member

@Siedlerchr can you set a breakpoint in the binding code, and see if it's called (and if yes with which values of before/after)

@Siedlerchr
Copy link
Member

Siedlerchr commented May 28, 2020

Okay, debugging reveals:
The binding code triggers for every entered char.
It seems there is some issue with line breaks
I just typed a and a newline and another a in the next line

newValue: a\na\n
oldValue: a\r\na

@tobiasdiez
Copy link
Member

Ahhh...the textarea always uses \n for line breaks (it's hard-coded: https://github.com/openjdk/jfx/blob/master/modules/javafx.controls/src/main/java/javafx/scene/control/TextArea.java#L129) but JabRef uses OS.NEWLINE upon save.

@calixtus
Copy link
Member Author

Oh no... That raises more problems.
So the basic solution should actually be right, now it's about how the TextArea is compared to the newValue. Should be enough to replace in the string every OS.NEWLINE with \n...

@tobiasdiez
Copy link
Member

tobiasdiez commented May 28, 2020

Thinking about it, it might also be the reason for some of the "Library has been modified by another application." issues. #4877 Maybe we should rethink the newline handling on save...

@calixtus
Copy link
Member Author

Should we always save in the OS specific format and rework it on loading? I think this demands a more serious discussion...

@calixtus
Copy link
Member Author

calixtus commented May 28, 2020

Moreover, I just discovered that typing into the multi-line fields as "Abstract" and "Comment" (those with the problem) you have you have a lag for which if you type fast you cannot see what you are typing: a problem you do not have when directly typing within "{} biblatex source" where typing is a much smoother (normal) experience.

The reason for the lag could be the preview window. Could you please try to deactivate the Preview Window in Preferences->Preview, restart JabRef and try again?

@gianlucabaldassarre
Copy link

gianlucabaldassarre commented May 30, 2020 via email

@calixtus calixtus marked this pull request as draft June 5, 2020 14:00
* upstream/master: (243 commits)
  fix checkstyle
  mEDRA DOI fetcher implementation. (#6641)
  Bump bcprov-jdk15on from 1.65.01 to 1.66 (#6676)
  Bump appleboy/ssh-action from v0.0.6 to v0.1.2 (#6674)
  Add localisation strings and renamed formatter
  Bump gittools/actions from v0.9.2 to v0.9.3 (#6675)
  Skip non-working sourcespy site
  Let dependabot update github actions
  Add three formatters to fix new lines in abstract and digits in editors
  Update src/test/java/org/jabref/logic/importer/fetcher/CollectionOfComputerScienceBibliographiesParserTest.java
  Fix to imports
  Make fetcher test more specific by checking each field explicitly
  Use builder instead of "setField" statements
  (docs) fix styling
  Update jpackage notes
  Fix automerge workflow
  fix markdown
  Bump org.beryx.jlink from 2.20.0 to 2.21.0
  Bump unirest-java from 3.8.00 to 3.8.06
  Fix to dependency on Global
  ...
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jul 12, 2020
@Siedlerchr Siedlerchr marked this pull request as ready for review July 14, 2020 09:22
@Siedlerchr Siedlerchr merged commit d5c42a5 into master Jul 17, 2020
@Siedlerchr Siedlerchr deleted the fix_6453 branch July 17, 2020 15:23
@tobiasdiez
Copy link
Member

@Siedlerchr I think the build currently fails because of merging this PR (architecture violation)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
entry-editor 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.

Cursors jumps to the beginning
4 participants