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

Shared database synchronized by FocusChangedEvent #6771

Merged
merged 8 commits into from
Sep 1, 2020
Merged

Shared database synchronized by FocusChangedEvent #6771

merged 8 commits into from
Sep 1, 2020

Conversation

m-mauersberger
Copy link
Contributor

This PR is related to #6663 and partly #4461.

Shared database synchronization event in DBMSSynchronizer is triggered by a FocusChangedEvent. This is added similar to FieldChangedEvent but has to be initiated from EntryEditor (?).

I have manually tested the new code - and it does not work. Maybe we can discuss changes through this pull request.

Thank you for your help.

m-mauersberger added 2 commits August 19, 2020 14:18
…lled. Focus is changed in EntryEditor.

CoarseChangeFilter with no special function -> does not work with shared databases.
* @param event {@link FocusChangedEvent} object
*/
@Subscribe
public void listen(FocusChangedEvent event) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the problem is that you are listening to an Event from the EventBus which is nowhere fired


if (fieldChange.getDelta() > 1 || isEditOnNewField) {
if (isEditOnNewField) {
lastFieldChanged = fieldChange.getField();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the eventBus.post method, the eventBus post method is reposinsible for firing the event.

@m-mauersberger
Copy link
Contributor Author

I tried firing the event via the BibEntry method setFocusedField. Again, this method is called by EntryEditor via setFocusToField.
But I don't know if this is generally possible.

Perhaps the second commit is a bit irritating, it is only uncommenting something in DBMSSynchronizer... Sorry for that.
The important commit is 87adec3b.

@Siedlerchr
Copy link
Member

Siedlerchr commented Aug 22, 2020

It's a bit complicated with all those listenes.
The EventBus is a local object. In fact it's similar to the classical EventLIstener in GUI. An even its relayed to it's parend until it's handled. The only difference is that we do this relaying here manually.
When you add a new Event in BibEntry you have to relay that to the call chain.

public void convertToSharedDatabase(DatabaseSynchronizer dmbsSynchronizer) {
this.dbmsSynchronizer = dmbsSynchronizer;
this.dbmsListener = new CoarseChangeFilter(this);
dbmsListener.registerListener(dbmsSynchronizer);
this.location = DatabaseLocation.SHARED;

And the CoraseChangeFilter register itself here:

public CoarseChangeFilter(BibDatabaseContext bibDatabaseContext) {
// Listen for change events
bibDatabaseContext.getDatabase().registerListener(this);
bibDatabaseContext.getMetaData().registerListener(this);
this.context = bibDatabaseContext;

it registers itselt as a listener on BibDAtabase.
And BibDatabase finally register itself on each entry:

public synchronized void insertEntries(List<BibEntry> newEntries, EntriesEventSource eventSource) {
Objects.requireNonNull(newEntries);
for (BibEntry entry : newEntries) {
entry.registerListener(this);
}
if (newEntries.isEmpty()) {
eventBus.post(new EntriesAddedEvent(newEntries, eventSource));
} else {
eventBus.post(new EntriesAddedEvent(newEntries, newEntries.get(0), eventSource));

So you need to listen for the FocusChangedEvent and relay that

Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for investigating this issue.

You already made a good analysis of the underlying problem in #6663. Your solution using a new variable tracking the focused field in the BibEntry, has sadly the disadvantage that the data object (the BibEntry) is tightly coupled to the UI state (focused field). That's something we try to avoid as much as possible. So we need to look for a solution that doesn't couple the model and gui. In the end, the problem comes from the fact that this if statement

if (fieldChange.getDelta() > 1 || isEditOnNewField) {
lastFieldChanged = fieldChange.getField();
eventBus.post(event);
}
sometimes prevent events from bubbling at all. I think, it would suffice to put an else statement there, which pushes the event say after 3 seconds. If a new small incremental event occurs, then the timer is just restarted. In this way, all events eventually trigger an update to the database. We actually have a class that should be able to do this: https://github.com/JabRef/jabref/blob/6319d05901266f98ac3163a7e66d7f0af72217d6/src/main/java/org/jabref/logic/util/DelayTaskThrottler.java

Can you try to implement something along these lines? That would be great!

Introduced DelayTaskThrottler on CoarseChangeFilter.
@m-mauersberger
Copy link
Contributor Author

Thank you for your help. Finally, I deleted all FocusEventChanged-related stuff and concentrated on CoarseChangeFilter.
There I introduced a DelayTaskThrottler for the eventBus.post(event). Also I revised the conditionals while introducing three conditions:

  • isNewEdit: Editing right after loading shared database.
  • isEditOnOtherField: Editing other field = changes on previous field should be synchronized.
  • isMajorChange: Some editing with much character input.

I found out that a throttled task will not interrupt any input. That means if a FieldChangedEvent is posted with delay, you can write further until database synchronization appears. In the worst case you lose some information because all characters typed in after the event are not synchronized. Furthermore, it appears that when the event is posted during delay JabRef hangs up...

Can you reproduce this problems? In general, it works as intended.

Another idea: What about throttling all FieldChangeEvents in order to not interrupt input process? Then we would have to ensure that all pending synchronization events would be ready before closing the database. We could add an indicator in the GUI for this purpose or add something on closing-database event.

@koppor
Copy link
Member

koppor commented Aug 27, 2020

Side note: We have an internal discussion ongoing, why not using JavaFX Observables everywhere - instead of the event bus. Not fully checked all aspects until now. Will (hopefully) be done at JabCon. Do you have a "feeling" on that?

@m-mauersberger
Copy link
Contributor Author

I have not much experience with JavaFX Observables but it seems like they are directly bound to GUI elements.
Thus, events maybe could only be fired less in accordance to the GUI... On the other hand it would be an advantage to use GUI events from everywhere with little effort.
I think the CoarseChangeFilter would become obsolete, wouldn't it?
So why not giving Observables a try...

@tobiasdiez
Copy link
Member

Thanks a lot @m-mauersberger! I've fixed a few very minor things that I noticed and will merge now. Looking forward to your next PR.

@tobiasdiez tobiasdiez merged commit c74c557 into JabRef:master Sep 1, 2020
Siedlerchr added a commit that referenced this pull request Sep 1, 2020
* upstream/master:
  Feature/add ui for query parsing (#6805)
  Shared database synchronized by FocusChangedEvent (#6771)
@m-mauersberger m-mauersberger deleted the request-focus branch September 3, 2020 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants