1005352 - Improve listener implementations

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Josh Dover [:jdover]]

1. Listeners are a great way to decouple functionality.
2. Listeners are inconsistently available as single listeners or multiple listeners
    i.  addListener vs. setListener
    ii. When originally created, we only needed one listener, later we find we need multiple.
3. Lots of boilerplate code needed to add a new listener interface.

There are at least 2 different approaches that could be taken to making our listeners
(1) have less boilerplate, (2) be more consistent, (3) be easier to access.

=====

The first is to use some sort of generics / reflection trick to create a single
"ListenerManager" interface that we could use throughout the code. Something like:

class Toolbar {
  public final ListenerManager<OnTapListener> onTap = new ListenerManager<OnTapListener>();
}

class SomeUiElement implements onTapListener {
  SomeUiElement(Toolbar toolbar) {
    toolbar.onTap.addListener(this);
  }

  @Override onTap() { doSomething(); }
}

A more detailed attempt with a comparison of pros / cons [1]

=====

The second is to use an event bus library, such as Square's Otto [2] (which I know has
been suggested before). Benefits of this include a more stable and well tested
implementation, a more generic mechanism that could be used for more than just
listeners (ie. PrivateModeChangedEvent, ThemeChangedEvent), and (depending on the
event bus) a way of filtering events so that not every subscriber has to
receive all events.

=====

This bug should serve to be a place where we could find one use case and try each
solution to compare the resulting code / API usage.

[1] https://pastebin.mozilla.org/5023067
[2] http://square.github.io/otto/

Comment 1

[Josh Dover [:jdover]]

Attachment: Add Otto library

Includes keeping annotated classes from being eaten from Proguard.

Comment 2

[Josh Dover [:jdover]]

Attachment: Add SharedBus singleton

Comment 3

[Josh Dover [:jdover]]

Attachment: Refactor OnTitleClickListener to use an event

This is an example usage of the otto bus to replace an listener interface. Notice
how HomePager and TabMenuStripLayout have no prior knowledge of one another.
Yay decoupling!

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 8416794 [details] [diff] [review]
Refactor OnTitleClickListener to use an event

I've been pushing for using an UI event bus every now and again. So, I like this :-) But I think we should put some thought on a few questions:

1. How such system would interact with our Gecko event bus? It would be nice if the UI and Gecko event buses worked nicely together in a transparent way.
2. What kind of event model we want to implement for the UI? Our EventDispatcher has a generic type of event (GeckoEvent) that can subclassed. Maybe we want to consolidate event types for both buses (UI and Gecko)?
3. Fennec has a multi-thread architecture but Otto is only meant to be used in the UI thread. Maybe something like Green Robot's EventBus[1] is a more appropriate event bus implementation for us?
4. I really like Otto's simplicity but I'm slightly concerned about its performance (given that Fennec is very event-heavy). Again, maybe EventBus is better alternative here?  

[1] https://github.com/greenrobot/EventBus

Comment 5

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.