Updates to colored group indicator for cited entries

[k3KAW8Pnf7mkmdSMPHz27]

Fixes #6394.

An entry can belong to a specific group depending on,

1. Field or type in the BibEntry
2. The settings of the specific group

Hence, any changes in a field or type in the BibEntry, or any change in the groups should reevaluation wether or not the entry is still in any given group.

I am not very comfortable with the JavaFX/MVVC structure yet, so I have attempted an approach with as few structural changes as possible.

• Keep the count "badge" is up-to-date
• Refresh the selected groups
  • Deal with ConcurrentModificationException
  • Fix performance bottleneck
    • Occurs when switching library and the group is selected
• Check for potential memory leaks,
  • caused by extending Observable in TexGroup.java (how should the listeners be stored?)
  • dependence on deprecated method GroupTreeNode#setGroup(AbstractGroup newGroup)
  • use of EventBus without unregistering listener
    • Make a WeakInvalidationListener adapter?
• JabRef believes the .bib file has changed when the .aux file changes
  • Check if the isFilteredOut attribute of BibDatabaseContextChangedEvent is used to prevent saving
    • Add JavaDoc
• How does the current GroupInvalidationEvent affect the hierarchical group structure? (should it be fired from elsewhere so that only partial recalculations of group colors are necessary)
• Replace unnecessary adapter class

---

• 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.

[k3KAW8Pnf7mkmdSMPHz27]

Would any of the following be more/less valid approaches?

1. Make TexGroup implement Observable,
2. Make TexGroup.keysUsedInAux an ObservableSet (I'd argue this seem similar to how BibEntry#getObservables operates)

[k3KAW8Pnf7mkmdSMPHz27]

I added a minimum working example of implementing the Observable interface. Any comments/suggestions are welcome (including scratch this approach X) )

[Siedlerchr]

Thanks for tackling this! It would be interesting to see how it performs with many group (can provide you a test file via mail or so, cause its confidential)

[k3KAW8Pnf7mkmdSMPHz27]

I can't observe impactful slowdowns/memory leaks, but there shouldn't be many changes that affects the large database. Most potential memory leaks will only occur when an .aux file is linked.

I have update the description with what I think is left, and commented on a change that can mostly be reverted if performance is an issue. Sorry about the slow going, JavaFX is somewhat new to me.

[DominikVoigt]

Hey there!
Thank you for your contribution.
Seeing that your last commit was pushed 5 days ago,
I want to ask whether you are finished with your work?
Is this PR ready for review :)?

[k3KAW8Pnf7mkmdSMPHz27]

Hey @DominikVoigt . I am sorry but no, I thought I would make a 30 min fix in #7210 but unfortunately it didn't turn out that way o.O
I don't think this is worth reviewing yet, the JabRef believes the .bib file has changed when the .aux file changes bullet item most likely means I am going to have to change/extend the metadata updates which might/might not be ok from JabRef's point-of-view.

[Siedlerchr]

@k3KAW8Pnf7mkmdSMPHz27 The bib file is changed can be also #5967

[k3KAW8Pnf7mkmdSMPHz27]

Thank you for the link! It might be both o.O I should have time to continue on this later today *knock on wood*

…

On December 21, 2020 at 14:36:12, Christoph ***@***.******@***.***)) wrote: @k3KAW8Pnf7mkmdSMPHz27(https://github.com/k3KAW8Pnf7mkmdSMPHz27) The bib file is changed can be also #5967(#5967) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub(#7173 (comment)), or unsubscribe(https://github.com/notifications/unsubscribe-auth/AAT2NZ45K4ZHUSSHHOJCE4DSV6PSZANCNFSM4USF5PMQ).

[Siedlerchr]

As a first workaround try disabling autosave.

[k3KAW8Pnf7mkmdSMPHz27]

@Siedlerchr at least for now it happens independently of autosave.

The reason why JabRef believes that the database has changed is that I wanted to use GroupUpdatedEvent to notify any listener that a group has changed. GroupUpdatedEvent extends BibDatabaseContextChangedEvent which I guess triggers

jabref/src/main/java/org/jabref/gui/LibraryTab.java

Lines 356 to 359 in 08e824f

	@Subscribe
	public void listen(BibDatabaseContextChangedEvent event) {
	this.changedProperty.setValue(true);
	}

among others. (I wanted to view it as a change to the MetaData)
The issue with TexGroup is that it

1. requires an update without any interaction from the user (it updates on changes in an external file), and
2. these updates does not need to be saved.

I think it is the only group with these characteristics, hence I don't see it motivated to extend the GroupUpdatedEvent since it seems well established that BibDatabaseContextChangedEvent implies a change that requires saving.

I'll figure something out but I am temporarily a bit stuck 😱

[k3KAW8Pnf7mkmdSMPHz27]

Hum... TexGroup has a reference to the MetaData. Is it ok if I its EventBus to post updates instead?

[k3KAW8Pnf7mkmdSMPHz27]

It might make sense to make TexGroup observable or to use its FileUpdateMonitor directly in the GroupNodeViewModel.

[tobiasdiez]

Do you still have open questions or is this already ready for review?

[calixtus]

Hey @k3KAW8Pnf7mkmdSMPHz27 , you didn't change anything on this PR for 5 days. Is this PR ready for review?

[k3KAW8Pnf7mkmdSMPHz27]

Sorry. Yes, it is ready for review

…

On January 8, 2021 at 20:26:53, Carl Christian Snethlage ***@***.******@***.***)) wrote: Hey @k3KAW8Pnf7mkmdSMPHz27(https://github.com/k3KAW8Pnf7mkmdSMPHz27) , you didn't change anything on this PR for 5 days. Is this PR ready for review? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub(#7173 (comment)), or unsubscribe(https://github.com/notifications/unsubscribe-auth/AAT2NZ3MSNH2XHRIYUKLINTSY6WF3ANCNFSM4USF5PMQ).

[tobiasdiez]

Thanks!

The code looks very good to me. The only problem I see is that the UI-binding already happens in the model class. This should be moved to the gui classes that need such a wrapping.

[tobiasdiez]

The model and logic packages need to be free of gui stuff. The wrapping on the JavaFX thread should happen right before it is displayed.

[tobiasdiez]

@JabRef/developers Our architecture tests don't work anymore?! This is clearly a violation but the tests pass.

[k3KAW8Pnf7mkmdSMPHz27]

I'd appreciate an opinion on what should create an update of the group color region in the maintable (entry.getObservables()?). Also, should that part of #7325 be addressed in this PR? (the group color region not updating)

[calixtus]

Worked myself through the changes. Looks good also to me, so I will merge this PR.
Thanks for your work on this issue!

[systemoperator]

I have just tried this fix with the current master build on Ubuntu 16.04. Unfortunately, it seems, neither the colored group indicator, nor the badge icon are updated automatically in my situation:

• Step 1: Build tex file with additional reference. Observations:
  • Counter in badge icon for cited entries is not incremented.
  • Badge icon for cited entries is not highlighted when the bib entry is selected.
  • Colored group indicator for cited entries is not shown for the newly added reference.
• Step 2: Update group of cited entries: Just edit the group of cited entries and save it again. When a dialogue appears choose, to assign the original group's entries again to this group. Observations:
  • Counter in badge icon for cited entries is now incremented properly.
  • Badge icon for cited entries is now highlighted properly when the bib entry is selected.
  • Colored group indicator for cited entries is still not shown for the newly added reference.
• Step 3: Close and reopen the bib-file. Observations:
  • Everything is shown as expected.

[k3KAW8Pnf7mkmdSMPHz27]

Sorry about that. I am using Texpad with Mac OS X and the group gets updated of the .aux file as I'd expect on JabRef 5.3--2021-01-26--034cf8c.
Could you give me more details on "Step 1" (which application you are using etc.) and I'll try to get a virtual box with Ubuntu up and going in the coming days?

[systemoperator]

I have used a test .tex file with biblatex (see first example in https://www.overleaf.com/learn/latex/bibliography_management_with_biblatex). The library was configured for biblatex in JabRef as well. I have used TeXstudio, but I think it's independent of this, because I have even simply opened the .aux file with some editor and I have directly removed or added entries like \abx@aux@cite{XXX2008}. It seems, that the file monitor does not get triggered properly on a file change, thus, keysUsedInAux will not be cleared and the current group bindings are not invalidated.

[k3KAW8Pnf7mkmdSMPHz27]

directly removed or added entries like \abx@aux@cite{XXX2008}. It seems, that the file monitor does not get triggered properly on a file change, thus, keysUsedInAux will not be cleared and the current group bindings are not invalidated.

Yup. Adding/removing entries in that fashion works for me in OS X. I'll look into it. I don't really have a hypothesis for why it doesn't work in Ubuntu (yet)

[k3KAW8Pnf7mkmdSMPHz27]

You are using a local drive? (not remote)

Is it possible your editor is generating a "create event" instead of a "modify event"? (This doesn't have to be the same issue as with TeXstudio)

[systemoperator]

Yes, on a local drive. You are right: I have created a small java test program of your provided link.

• Creating the file "testFile.txt" triggers:

ENTRY_CREATE:testFile.txt

• Editing the file with the simple text editor Pluma triggers:

ENTRY_CREATE:testFile.txt

If this file is already opened in TeXstudio (and it has been modified within it before), it detects the change and it asks whether the file should be reloaded.

• Editing the file with TeXstudio triggers:

ENTRY_CREATE:testFile.txt.G29838
ENTRY_MODIFY:testFile.txt.G29838
ENTRY_MODIFY:testFile.txt.G29838
ENTRY_DELETE:testFile.txt.G29838
ENTRY_CREATE:testFile.txt

Apparently, other programs also observe re-ceating files, since I have never had such problems elsewhere before.

• Editing the file with VS Code triggers:

ENTRY_MODIFY:testFile.txt
ENTRY_MODIFY:testFile.txt

But when building the .pdf file from a .tex file with TeXstudio, 30 modification events are triggered with this java program for the .aux file:

ENTRY_MODIFY:main.aux
// 28 modification entries omitted
ENTRY_MODIFY:main.aux

But still no event gets triggered in JabRef.

The same applies when editing the .aux file with VS Code, which also triggers a modification event with the java test program but not within JabRef.

[systemoperator]

@tobiasdiez @k3KAW8Pnf7mkmdSMPHz27 Do you think, that it would make sense to also observe the ENTRY_CREATE event to make the detection of file-modifications more reliable, since two out of three of my tested editors trigger the ENTRY_CREATE event and other programs apparently also check on this event?

[tobiasdiez]

Yes, I guess it's a good idea to also listen for entry_create. At least I cannot think of a negative sideeffect.