AddEventSystem by obraliar · Pull Request #1028 · JabRef/jabref

obraliar · 2016-03-23T19:39:51Z

Introduce an EventBus object being passed around. This enables better testing.
Make DatabaseChangeEvent abstract and add subclasses according to DatbaseChangeEvent
Rewrite the currently existing code to use that event bus instead of net.sf.jabref.model.database.BibDatabase.addDatabaseChangeListener(DatabaseChangeListener) and net.sf.jabref.model.database.BibDatabase.removeDatabaseChangeListener(DatabaseChangeListener)
Changes described in CHANGELOG.md?
Tests created for changes?
Tests green?

obraliar · 2016-03-23T19:43:38Z

Issue: #969

obraliar · 2016-03-23T19:52:36Z

This commit should also demonstrate the structure of the new event system.

Discussions are welcome.

oscargus · 2016-03-24T15:47:18Z

I just want to point out that there is an event system for BibDatabase. Which is not that much used. Maybe it can be used as inspiration/replaced.

koppor · 2016-03-25T18:15:21Z

This does demonstrate how the API of Google Guava works, but not how it can really be used at JabRef. Should we use one bus for everything? Should we use one bus per "thing" such as BibDatabase etc. - This is also discussed at the documentation.

Please double check the usage of net.sf.jabref.model.database.DatabaseChangeEvent. I think, the whole code around net.sf.jabref.model.database.DatabaseChangeEvent should be replaced with the Guava thing. Instead of net.sf.jabref.model.database.DatabaseChangeEvent.ChangeType there should be different classes. Then, it is possible to use the power of the EventBus. Important excerpts of the documentation: "which accepts a single argument of the type of event desired; " and "The EventBus instance will determine the type of event and route it to all registered listeners.". This enables to use a single bus with dedicated events such as FieldContentChanged etc.

Thus, please update this PR to

~~Introduce a Global EventBus into net.sf.jabref.Globals~~ Introduce an EventBus object being passed around. This enables better testing.
Make DatabaseChangeEvent abstract and add subclasses according to DatbaseChangeEvent
Rewrite the currently existing code to use that event bus instead of net.sf.jabref.model.database.BibDatabase.addDatabaseChangeListener(DatabaseChangeListener) and net.sf.jabref.model.database.BibDatabase.removeDatabaseChangeListener(DatabaseChangeListener)

obraliar · 2016-03-25T18:33:11Z

Yes, it actually only demonstrates the base function. I think we should have a separate package for that. So I created a new one and created a interface, which should normalize the usage of Google guava. Now we are going to replace the existing listeners and as @koppor already observed to prepare a global event bus.

tobiasdiez · 2016-03-25T18:38:15Z

Mhh...I'm not sure that a global event bus is the right way to go. It adds another global object, makes debugging harder, everyone gets informed about everything (it is hard to listen only to changes to a certain database) etc. But of course it also has its advantages. What is the opinion of the other @JabRef/developers here?

obraliar · 2016-03-25T19:11:52Z

I think its OK to use one. Therefore we use different Event objects, which should prevent the chaos.

koppor · 2016-03-25T19:21:58Z

On the one hand, I don't like to pass around the EventBus object everywhere. It is a global thing and used everywhere. Maybe we should place it in net.sf.jabref.gui.GUIGlobals.

On the other hand, I agree that a global object makes testing difficult.

Let's hear about design ideas by the others. This, however, doesn't stop @obraliar replacing the old event system. The "location" of the EventBus object can be changed later on.

tobiasdiez · 2016-03-25T19:35:56Z

+ * to pass objects on event occurrence.
+ */
+
+public class Event {


Why not use https://docs.oracle.com/javase/7/docs/api/java/util/EventObject.html for this?

Oh, I didn't know that there is a existing class for this purpose :)

I see no reason for using java-util. We are implementing something contrary when using Guava. The only argument I see is that we can trace which events are existing. For that thing, I would agree to define an abstract class JabRefEvent.

I agree with @koppor From the api doc the EventObject looks like that is's main purpose is to be used in GUI swing/awt events.

koppor · 2016-03-25T20:11:16Z

@obraliar Please do not take further action until @JabRef/developers decided which way to go.

obraliar · 2016-03-29T15:02:56Z

No opinions? If not, we would begin to discuss the details internally within the stupro group.

lenhard · 2016-03-29T15:15:02Z

Sorry, but (in contrast to other developers) I do not see why we need an event system. Everything we need should be doable with the plain old observer pattern and the benefit of an event bus is not clear to me.

This will be discussed in a devcall, which may or may not take place soon.

obraliar · 2016-04-24T14:10:23Z

Now all entry events only base upon Google's Guava Bus System.
The integrated event system (listener registration, event posts & receivements, etc.) works.

Here are some new findings:

We have to use one EventBus for each BibDatabase, cause there may be more than one opened. A main event such as AddEntryEvent should not affect all opened databases.
For all other events that do not affect BibDatabase we could introduce one general EventBus. This part of introduction is very easy, but I see no need for that yet.

koppor · 2016-05-07T12:53:09Z

        this.setType(type.getName());
    }

+    /**


Why is the code moved in this class? Could you keep the old position? I think, the move was on purpose. See #1309

Done. Moved to old position.

…riable names.

tobiasdiez · 2016-05-07T14:19:49Z

+        return fieldName;
+    }
+
+    public String getValue() {


getNewValue and rename private variable.

tobiasdiez · 2016-05-07T14:36:05Z

+        if (entry.isPresent() && (entry.get() != newEntry)) {
+            entry.ifPresent(e -> e.unregisterListener(this.currentChangeFieldUpdateEvent));
+            this.currentChangeFieldUpdateEvent = new ChangeFieldUpdateEvent();
+            newEntry.registerListener(this.currentChangeFieldUpdateEvent);


I think, the registerListener should be moved after the if statement.

With a check if newEntry is not null.

and use ChangeFieldEvent instead. Remove private listener classes. Small refactorings.

koppor · 2016-05-08T13:10:43Z

+import net.sf.jabref.model.entry.BibEntry;
+
+/**
+ * <code>ChangedFieldEvent</code> is fired when a field of <code>BibEntry</code> has been modified.


Or removed. Or added. See eventBus.post(new ChangedFieldEvent(this, fieldName, null));. Please add this as comment.

koppor · 2016-05-09T12:35:04Z

simonharrer · 2016-05-09T12:42:01Z

+
+        if (entry.isPresent() && (entry.get() != newEntry)) {
+            entry.ifPresent(e -> e.unregisterListener(this));
        }


could be written simpler: entry.filter(e -> e != newEntry).ifPresent(e -> e.unregisterListener(this));

tobiasdiez · 2016-05-19T21:03:34Z

@JabRef/stupro could you please add some short notes to the wiki on how to push and subscribe to events (using the eventbus). Thanks!

obraliar · 2016-07-08T19:50:31Z

Done. See https://github.com/JabRef/jabref/wiki/Code-Howtos#using-the-eventsystem

koppor added stupro dev: code-quality Issues related to code or architecture decisions labels Mar 25, 2016

koppor assigned obraliar Mar 25, 2016

koppor mentioned this pull request Mar 25, 2016

Add event system to JabRef #969

Closed

koppor added the [outdated] type: question label Mar 25, 2016

tobiasdiez reviewed Mar 25, 2016
View reviewed changes

koppor added the status: devcall label Mar 25, 2016

simonharrer mentioned this pull request Mar 29, 2016

Search should be reevaluated if entry is pasted #1000

Closed

obraliar force-pushed the AddEventSystem branch from 91ad3ee to 2e8afac Compare April 4, 2016 01:32

obraliar force-pushed the AddEventSystem branch from 2e8afac to 7ad383a Compare April 13, 2016 23:40

koppor force-pushed the master branch from ec2cc84 to 5a1ed8e Compare April 17, 2016 17:54

obraliar force-pushed the AddEventSystem branch 5 times, most recently from 8ce8ea5 to 8e34f7d Compare April 24, 2016 14:08

koppor reviewed May 7, 2016
View reviewed changes

Add JavaDoc comments. Remove constructor (add definition). Improve va…

a3998a9

…riable names.

tobiasdiez reviewed May 7, 2016
View reviewed changes

Rename event classes and variables.

1342a27

tobiasdiez reviewed May 7, 2016
View reviewed changes

obraliar added 2 commits May 8, 2016 02:29

Fix various comments. Remove SpecialFieldUpdateListener

f6605c7

and use ChangeFieldEvent instead. Remove private listener classes. Small refactorings.

Fix comment.

740a257

koppor reviewed May 8, 2016
View reviewed changes

obraliar and others added 2 commits May 8, 2016 20:15

Extend ChangedEntryEvent. Fix comment.

7a38d87

Fix variable names

7f12e9f

koppor merged commit a851faf into JabRef:master May 9, 2016

koppor changed the title ~~[WIP] AddEventSystem~~ AddEventSystem May 9, 2016

koppor removed status: devcall status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels May 9, 2016

simonharrer reviewed May 9, 2016
View reviewed changes

obraliar mentioned this pull request May 26, 2016

Implementation of shared database support #1451

Merged

3 tasks

koppor mentioned this pull request Aug 9, 2016

Create blog entry for shared database support JabRef/blog.jabref.org#1

Open

Uh oh!

Conversation

obraliar commented Mar 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

obraliar commented Mar 23, 2016

Uh oh!

obraliar commented Mar 23, 2016

Uh oh!

oscargus commented Mar 24, 2016

Uh oh!

koppor commented Mar 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

obraliar commented Mar 25, 2016

Uh oh!

tobiasdiez commented Mar 25, 2016

Uh oh!

obraliar commented Mar 25, 2016

Uh oh!

koppor commented Mar 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koppor commented Mar 25, 2016

Uh oh!

obraliar commented Mar 29, 2016

Uh oh!

lenhard commented Mar 29, 2016

Uh oh!

obraliar commented Apr 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koppor commented May 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobiasdiez commented May 19, 2016

Uh oh!

obraliar commented Jul 8, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

obraliar commented Mar 23, 2016 •

edited

Loading

koppor commented Mar 25, 2016 •

edited

Loading