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

SuXiChangZhen · 2021-05-19T05:09:57Z

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

Change in CHANGELOG.md described in a way that is understandable for the average user (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.

src/main/java/org/jabref/gui/maintable/BibEntryTableViewModel.java

k3KAW8Pnf7mkmdSMPHz27 · 2021-05-28T17:10:19Z

src/main/java/org/jabref/gui/maintable/BibEntryTableViewModel.java

        ArrayList<Observable> observables = new ArrayList<>(List.of(entry.getObservables()));
+        Optional<BibEntry> referenced = fieldValueFormatter.getValue().getReferencedEntry(entry);
+        referenced.ifPresent(bibEntry -> observables.addAll(List.of(bibEntry.getObservables())));


I don't know enough of JavaFX to tell you for sure how to address this. I'd suggest not removing the caching (fieldValues.get(fields);) and instead deal with the dependencies differently.

You can extend StringBinding and use .bind and .unbind to "manually" deal with the dependencies. (this is probably not a good approach)

You can extend a different binding (ObjectBinding?) and have it handle changes to the referenced entry. In this scenario, the StringBinding has this ObjectBinding among its dependencies instead of the referenced bibentry's getObservables().

At least superficially this seems like a good application for EasyBind because,

You can create an optional binding, e.g., entry.getFieldBinding(StandardField.CROSSREF)

This means you can create an OptionalBinding<BibEntry> referencedBibEntry

It should be possible to keep an observableArrayList with extractor (FXCollections.observableArrayList(BibEntry::getObservables)) updated with ~~the ifValuePresent method of the OptionalBinding<BibEntry>~~ EasyBind.subscribe. That should keep the list up-to-date when the referenced BibEntry is changed/removed and when the observables in the referenced bibEntry.getObservables are modified

This list should now be usable as a dependency

Note that the two last approaches should be used or called from the constructor since they will update on changes to the referenced BibEntry.

I'd appreciate it if someone with more experience of JavaFX chimes in with opinions 😛

Discussion:

@artcile{a, crossref={b} } @book{b}

to

@artcile{a, crossref={c} } @book{b} @book{c}

Changes to c are not reflected accordingly.

koppor

We discussed it in our DevCall and have some comments to move forward here.

src/main/java/org/jabref/gui/maintable/BibEntryTableViewModel.java

koppor · 2021-07-05T18:56:21Z

src/main/java/org/jabref/gui/maintable/BibEntryTableViewModel.java

-        ObservableValue<String> value = fieldValues.get(fields);
-        if (value != null) {
-            return value;
-        }
-


This should be kept. If it does not work on your side, please comment. This caching helps us to gain performance (in the case of large databases, as this is initialized at the start of JabRef).

We need to update the cache if the referenced crossref entry is changed.

Decision: We need to keep this code - and will handle the issue for changed crossref value later (and incorporate the comments of https://github.com/JabRef/jabref/pull/7754/files#r641697015)

koppor · 2021-07-05T19:02:43Z

src/main/java/org/jabref/gui/maintable/BibEntryTableViewModel.java

        ArrayList<Observable> observables = new ArrayList<>(List.of(entry.getObservables()));
+        Optional<BibEntry> referenced = fieldValueFormatter.getValue().getReferencedEntry(entry);
+        referenced.ifPresent(bibEntry -> observables.addAll(List.of(bibEntry.getObservables())));


Discussion:

@artcile{a, crossref={b} } @book{b}

to

@artcile{a, crossref={c} } @book{b} @book{c}

Changes to c are not reflected accordingly.

SuXiChangZhen · 2021-07-06T02:28:30Z

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! 😊

k3KAW8Pnf7mkmdSMPHz27 · 2021-07-08T11:18:05Z

@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 #7754 (comment) but I make no guarantees that it will work nor that it is the "proper way" 😛

Siedlerchr · 2021-07-19T18:42:15Z

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

k3KAW8Pnf7mkmdSMPHz27 · 2021-07-22T21:51:57Z

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 · 2021-07-22T21:58:22Z

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

calixtus · 2021-07-24T20:51:28Z

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

k3KAW8Pnf7mkmdSMPHz27 · 2021-07-25T00:43:17Z

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.

calixtus · 2021-07-25T05:28:25Z

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

k3KAW8Pnf7mkmdSMPHz27 · 2021-07-25T11:49:28Z

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.

calixtus · 2021-10-25T21:05:02Z

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.

# Conflicts: # src/main/java/org/jabref/gui/maintable/BibEntryTableViewModel.java

…fix-for-issue-7730 # Conflicts: # src/main/java/org/jabref/gui/maintable/BibEntryTableViewModel.java

github-actions

Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.

In case of issues with the import order, double check that you activated Auto Import.
You can trigger fixing imports by pressing Ctrl+Alt+O to trigger Optimize Imports.

k3KAW8Pnf7mkmdSMPHz27 reviewed May 28, 2021

View reviewed changes

src/main/java/org/jabref/gui/maintable/BibEntryTableViewModel.java Outdated Show resolved Hide resolved

k3KAW8Pnf7mkmdSMPHz27 reviewed May 28, 2021

View reviewed changes

koppor requested changes Jul 5, 2021

View reviewed changes

k3KAW8Pnf7mkmdSMPHz27 mentioned this pull request Jul 20, 2021

Display attached files in refererenced crossref entry #7731

Open

SuXiChangZhen and others added 4 commits July 22, 2021 17:32

fix for issue 7730

0468fa8

fix checkstyle

d0a522a

Fix caching

abcdcad

Remove wrapper for getReferencedEntry

1b50be6

k3KAW8Pnf7mkmdSMPHz27 force-pushed the fix-for-issue-7730 branch 2 times, most recently from 828a066 to 1b50be6 Compare July 22, 2021 21:47

k3KAW8Pnf7mkmdSMPHz27 changed the title ~~Add referenced entry's observers to update the crossref entry's preview table~~ Add referenced entry's observers to update the entry in the main-table Jul 22, 2021

calixtus added the status: devcall label Oct 25, 2021

koppor assigned k3KAW8Pnf7mkmdSMPHz27 Nov 8, 2021

koppor removed the status: devcall label Nov 8, 2021

ThiloteE added the maintable label Apr 20, 2022

Merge branch 'main' into fix-for-issue-7730

d14b099

ThiloteE added the entry-editor label May 12, 2022

calixtus added the jabcon label Aug 15, 2022

ThiloteE removed the jabcon label Sep 13, 2022

k3KAW8Pnf7mkmdSMPHz27 mentioned this pull request Apr 13, 2023

Display files from referenced crossref in entry table (HighCrit version) #9717

Draft

11 tasks

k3KAW8Pnf7mkmdSMPHz27 added 2 commits May 2, 2023 16:58

Merge branch 'main' into fix-for-issue-7730

3f6ba17

# Conflicts: # src/main/java/org/jabref/gui/maintable/BibEntryTableViewModel.java

Merge remote-tracking branch 'SuXiChangZhen/fix-for-issue-7730' into …

a58e5d3

…fix-for-issue-7730 # Conflicts: # src/main/java/org/jabref/gui/maintable/BibEntryTableViewModel.java

koppor force-pushed the main branch from 0594cb2 to c444c73 Compare October 10, 2023 21:26

koppor mentioned this pull request Oct 26, 2023

Display files from referenced crossref in entry table (Toro520 version) #10577

Closed

6 tasks

Merge branch 'main' into fix-for-issue-7730

d04621a

koppor unassigned k3KAW8Pnf7mkmdSMPHz27 Dec 9, 2024

Merge branch 'main' into fix-for-issue-7730

d2242d0

github-actions bot reviewed Dec 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

SuXiChangZhen commented May 19, 2021 •

edited by k3KAW8Pnf7mkmdSMPHz27

Loading

k3KAW8Pnf7mkmdSMPHz27 May 28, 2021 •

edited

Loading

koppor Jul 5, 2021

koppor left a comment

koppor Jul 5, 2021

koppor Jul 5, 2021

koppor Jul 5, 2021

SuXiChangZhen commented Jul 6, 2021

k3KAW8Pnf7mkmdSMPHz27 commented Jul 8, 2021

Siedlerchr commented Jul 19, 2021

k3KAW8Pnf7mkmdSMPHz27 commented Jul 22, 2021

k3KAW8Pnf7mkmdSMPHz27 commented Jul 22, 2021 •

edited

Loading

calixtus commented Jul 24, 2021

k3KAW8Pnf7mkmdSMPHz27 commented Jul 25, 2021 •

edited

Loading

calixtus commented Jul 25, 2021

k3KAW8Pnf7mkmdSMPHz27 commented Jul 25, 2021

calixtus commented Oct 25, 2021

github-actions bot left a comment

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

Are you sure you want to change the base?

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

Conversation

SuXiChangZhen commented May 19, 2021 • edited by k3KAW8Pnf7mkmdSMPHz27 Loading

k3KAW8Pnf7mkmdSMPHz27 May 28, 2021 • edited Loading

Choose a reason for hiding this comment

koppor Jul 5, 2021

Choose a reason for hiding this comment

koppor left a comment

Choose a reason for hiding this comment

koppor Jul 5, 2021

Choose a reason for hiding this comment

koppor Jul 5, 2021

Choose a reason for hiding this comment

koppor Jul 5, 2021

Choose a reason for hiding this comment

SuXiChangZhen commented Jul 6, 2021

k3KAW8Pnf7mkmdSMPHz27 commented Jul 8, 2021

Siedlerchr commented Jul 19, 2021

k3KAW8Pnf7mkmdSMPHz27 commented Jul 22, 2021

k3KAW8Pnf7mkmdSMPHz27 commented Jul 22, 2021 • edited Loading

calixtus commented Jul 24, 2021

k3KAW8Pnf7mkmdSMPHz27 commented Jul 25, 2021 • edited Loading

calixtus commented Jul 25, 2021

k3KAW8Pnf7mkmdSMPHz27 commented Jul 25, 2021

calixtus commented Oct 25, 2021

github-actions bot left a comment

Choose a reason for hiding this comment

SuXiChangZhen commented May 19, 2021 •

edited by k3KAW8Pnf7mkmdSMPHz27

Loading

k3KAW8Pnf7mkmdSMPHz27 May 28, 2021 •

edited

Loading

k3KAW8Pnf7mkmdSMPHz27 commented Jul 22, 2021 •

edited

Loading

k3KAW8Pnf7mkmdSMPHz27 commented Jul 25, 2021 •

edited

Loading