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

Duplication when using entry editor #3352

Closed
sambo57u opened this Issue Oct 25, 2017 · 6 comments

Comments

Projects
None yet
5 participants
@sambo57u

sambo57u commented Oct 25, 2017

This is 4.1dev Oct.25 on Linux x86_64 with java 1.8.0_152.

  1. Open a bib file
  2. Double clck on an entry so that the source editor window opens up
  3. Click into the editor source window
  4. Then click onto one of the other bib entries shown in the table above the editor
  5. The other entry is duplicated

I also had a problem with changes done via the source editor not being saved. It may be related to
above. Will investigate further.

@halirutan

This comment has been minimized.

Show comment
Hide comment
@halirutan

halirutan Oct 26, 2017

Collaborator

This is neat and it was an overlook by the reviewers (including myself) who approved the PR #3333 from @tobiasdiez. What happens is that on moving to a new entry we call bindToEntry which in turn creates a new source editor. This will call storeSource with the new entry here. The entry is already the new one but the code in the code area is the old one and it overwrites it.

It should be an easy fix and maybe the right time to review if the communication flow does really what we intend. What I would suggest is:

  • if someone types into this field, the storeEntry method should be called after a tiny pause to make changes instantly visible if the code is correct
  • switching tabs or moving to a different entry will ensure as well that modifications are reflected
  • while typing, changes are only reflected when the parser says it's valid BibTeX code
  • when switching tabs or moving to a different and the code is incorrect, we probably should send a warning instead of silently not saving the code

Currently, this does not work correctly. Making changes that invalidate the entry will be silently ignored when you switch tabs or entries.

Collaborator

halirutan commented Oct 26, 2017

This is neat and it was an overlook by the reviewers (including myself) who approved the PR #3333 from @tobiasdiez. What happens is that on moving to a new entry we call bindToEntry which in turn creates a new source editor. This will call storeSource with the new entry here. The entry is already the new one but the code in the code area is the old one and it overwrites it.

It should be an easy fix and maybe the right time to review if the communication flow does really what we intend. What I would suggest is:

  • if someone types into this field, the storeEntry method should be called after a tiny pause to make changes instantly visible if the code is correct
  • switching tabs or moving to a different entry will ensure as well that modifications are reflected
  • while typing, changes are only reflected when the parser says it's valid BibTeX code
  • when switching tabs or moving to a different and the code is incorrect, we probably should send a warning instead of silently not saving the code

Currently, this does not work correctly. Making changes that invalidate the entry will be silently ignored when you switch tabs or entries.

@lenhard

This comment has been minimized.

Show comment
Hide comment
@lenhard

lenhard Oct 26, 2017

Member

The source tab with its dependencies on the other tabs is always a problem... It would be good if this got fixed before we release 4.1.

Most of what you write seems reasonable, except for reflecting changes while a user is typing in the source tab. If we immediately store every character change (as was the case for some time during development between 3.8.2 and 4.0), then we really annoy all users who have autosave or a sync to a database enabled. I think it would be best if we store the contents every time the text area looses focus or the entry changes.

Member

lenhard commented Oct 26, 2017

The source tab with its dependencies on the other tabs is always a problem... It would be good if this got fixed before we release 4.1.

Most of what you write seems reasonable, except for reflecting changes while a user is typing in the source tab. If we immediately store every character change (as was the case for some time during development between 3.8.2 and 4.0), then we really annoy all users who have autosave or a sync to a database enabled. I think it would be best if we store the contents every time the text area looses focus or the entry changes.

@lenhard lenhard added this to the v4.1 milestone Oct 26, 2017

@Siedlerchr

This comment has been minimized.

Show comment
Hide comment
@Siedlerchr

Siedlerchr Oct 26, 2017

Contributor

@lenhard didn't you implement the CoarseChangeFilter which only fires, if one leaves the field? Can't we re-use that here?

Contributor

Siedlerchr commented Oct 26, 2017

@lenhard didn't you implement the CoarseChangeFilter which only fires, if one leaves the field? Can't we re-use that here?

@halirutan

This comment has been minimized.

Show comment
Hide comment
@halirutan

halirutan Oct 27, 2017

Collaborator

@lenhard

If we immediately store every character change (as was the case for some time during development between 3.8.2 and 4.0), then we really annoy all users who have autosave or a sync to a database enabled.

I understand. I was just talking from experience with IDEA development where of course not each newly typed character triggers a sync with the lexer and parser. They have a very clever way of deferring this so that the user has a fluid typing experience. Nevertheless, the reparse is almost immediately available so that code correctness and highlighting can kick in.

There are additional flaws in the logic. One that I could identify is the following: Change something in the source editor and press Ctrl+S for saving the database. The changes from the source tab won't make it into the main table and won't be saved.

Collaborator

halirutan commented Oct 27, 2017

@lenhard

If we immediately store every character change (as was the case for some time during development between 3.8.2 and 4.0), then we really annoy all users who have autosave or a sync to a database enabled.

I understand. I was just talking from experience with IDEA development where of course not each newly typed character triggers a sync with the lexer and parser. They have a very clever way of deferring this so that the user has a fluid typing experience. Nevertheless, the reparse is almost immediately available so that code correctness and highlighting can kick in.

There are additional flaws in the logic. One that I could identify is the following: Change something in the source editor and press Ctrl+S for saving the database. The changes from the source tab won't make it into the main table and won't be saved.

@lenhard lenhard referenced this issue Oct 27, 2017

Merged

Source tab entry duplication #3360

2 of 6 tasks complete
@lenhard

This comment has been minimized.

Show comment
Hide comment
@lenhard

lenhard Oct 27, 2017

Member

I have implemented a quickfix for the problem described in the op's comment: #3360 The problem didn't come from the focus binding linked above, but from the binding that was triggered when an entry change happened. Essentially that binding is useless in the new structure, because the source tab gets the notification of an entry change via bindToEntry() anyway. So, I removed this binding. Also pushing around an entry is no longer needed, because there is now a currentEntry that can be used.

This gets rid of the critical issue of overriding the data of a different entry, but there are several other flaws still present in the source tab. Content in the source tab is not stored if you change it and save and quit or if you just move to a different entry. It is very easy to store the source for every character change, instead of binding to the focus property you just have to bind to the text property like this:

EasyBind.subscribe(codeArea.textProperty(), text -> {
      storeSource();
 });

The update might not be a problem, because as @Siedlerchr said if the listeners use the CoarseChangeFilter they will not immediately trigger an autosave. The problem is rather that the store operation will move the cursor to the end of the source tab. I could not find a quick solution, and really have to be doing something else now. I would be happy if someone else could look into this.

Member

lenhard commented Oct 27, 2017

I have implemented a quickfix for the problem described in the op's comment: #3360 The problem didn't come from the focus binding linked above, but from the binding that was triggered when an entry change happened. Essentially that binding is useless in the new structure, because the source tab gets the notification of an entry change via bindToEntry() anyway. So, I removed this binding. Also pushing around an entry is no longer needed, because there is now a currentEntry that can be used.

This gets rid of the critical issue of overriding the data of a different entry, but there are several other flaws still present in the source tab. Content in the source tab is not stored if you change it and save and quit or if you just move to a different entry. It is very easy to store the source for every character change, instead of binding to the focus property you just have to bind to the text property like this:

EasyBind.subscribe(codeArea.textProperty(), text -> {
      storeSource();
 });

The update might not be a problem, because as @Siedlerchr said if the listeners use the CoarseChangeFilter they will not immediately trigger an autosave. The problem is rather that the store operation will move the cursor to the end of the source tab. I could not find a quick solution, and really have to be doing something else now. I would be happy if someone else could look into this.

@tobiasdiez

This comment has been minimized.

Show comment
Hide comment
@tobiasdiez

tobiasdiez Oct 31, 2017

Member

All the remaining issues should be fixed now with #3366

Member

tobiasdiez commented Oct 31, 2017

All the remaining issues should be fixed now with #3366

@tobiasdiez tobiasdiez closed this Oct 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment