jabref icon indicating copy to clipboard operation
jabref copied to clipboard

Add referenced entry's observers to update the entry in the main-table

Open SuXiChangZhen opened this issue 3 years ago • 10 comments

I change the getField method so that every time it will bind entry and referenced entry(if exists). Although the behavior now seems correct, I don't think it is an appropriate way to solve the problem. Hope for some suggestions. Mitigates #7730

  • [x] Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • [ ] Tests created for changes (if applicable)
  • [x] 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.

SuXiChangZhen avatar May 19 '21 05:05 SuXiChangZhen

Thank you for all your comments. In fact, this was my first attempt to solve the issue and it just worked (ugly indeed). The reason I delete the code about the init cache is that if the value is not null, it just return without considering update. So maybe we can consider update the binding in the corresponding places(crossref field is changed, etc.). Currently I still don't figure out a proper way to solve this. So appreciate for your further comments and suggestions! 😊

SuXiChangZhen avatar Jul 06 '21 02:07 SuXiChangZhen

@SuXiChangZhen our thinking is that with the caching re-enabled, this is still an improvement on the previous behavior. Perhaps the caching can be re-enabled, you use bibDatabaseContext.getDatabase().getReferencedEntry(entry) and we do a re-review -> merge?

Regarding a "proper way to solve this", unless you have an interest in EasyBind/JavaFX in particular, it is probably not going to be worth your time, it might be better to pick a different issue.

If you want to give it an attempt anyway, we'd still propose fixing the other parts we commented on in this PR, merge, and then create a follow-up PR. I can expand on the suggested approach in https://github.com/JabRef/jabref/pull/7754#discussion_r664093093 but I make no guarantees that it will work nor that it is the "proper way" 😛

k3KAW8Pnf7mkmdSMPHz27 avatar Jul 08 '21 11:07 k3KAW8Pnf7mkmdSMPHz27

@k3KAW8Pnf7mkmdSMPHz27 Would you mind taking ove the PR so we can finish it? Just add the caching

Siedlerchr avatar Jul 19 '21 18:07 Siedlerchr

Ok. I am very sorry for force-pushing to your repository @SuXiChangZhen, definitively last time I am trying that X) These are the changes we consider sufficient for this PR since it improves on the original JabRef behavior even if it doesn't completely solve the issue.

@Siedlerchr thank you for the git-support X)

k3KAW8Pnf7mkmdSMPHz27 avatar Jul 22 '21 21:07 k3KAW8Pnf7mkmdSMPHz27

The changes in this PR are the following

  • When an entry was cross-referencing a different entry, changes in (say author) didn't propagate unless JabRef were restarted (or possibly switched back/forth between tabs). Now you'll see the entry being visually updated in real-time when you make changes in the cross-referenced entry.

What remains after this PR,

  • If the entry being cross-referenced is changed, you need to restart/switch tabs again to make the updating work properly again

k3KAW8Pnf7mkmdSMPHz27 avatar Jul 22 '21 21:07 k3KAW8Pnf7mkmdSMPHz27

The crossref field is already in the array of the triggers for the CreateStringBinding-method?

calixtus avatar Jul 24 '21 20:07 calixtus

Yes, but due to the caching, the dependencies on the cross-refs entry’s fields won’t update when cross-ref does (we need to “depend” on all observables in the cross-referenced entry to know when to update, and that is funky to do)

This was why the caching was removed in the original solution.

k3KAW8Pnf7mkmdSMPHz27 avatar Jul 25 '21 00:07 k3KAW8Pnf7mkmdSMPHz27

Ah, ok. Rebuilding the cache from an invalidation listener?

calixtus avatar Jul 25 '21 05:07 calixtus

That would probably work. In “theory”, you shouldn't need to rebuild it.

Anyway, that part is why this issue is a bit of a mess. As is, this PR is imo an improvement on the previous behavior without that part as well.

k3KAW8Pnf7mkmdSMPHz27 avatar Jul 25 '21 11:07 k3KAW8Pnf7mkmdSMPHz27

We shoudd make a decision here: Merge it, as it is an improvement to the current situation, although it does not solve the issue at all, or try to fix it completly. Putting this on line for next dev call.

calixtus avatar Oct 25 '21 21:10 calixtus