Page MenuHomePhabricator

Replace Otto event bus
Closed, ResolvedPublic3 Story Points

Description

Otto is now deprecated. From the deprecation notice:

This project is deprecated in favor of RxJava and RxAndroid. These projects permit the same event-driven programming model as Otto, but they’re more capable and offer better control of threading.
If you’re looking for guidance on migrating from Otto to Rx, this post is a good start.

Details

Related Gerrit Patches:
apps/android/wikipedia : masterRx: Remove Otto event bus!

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 2 2017, 8:17 PM
Mholloway added a subscriber: Mholloway.
Charlotte set the point value for this task to 3.Feb 26 2018, 5:46 PM
yashasvi added a subscriber: Dbrant.EditedFeb 27 2018, 8:50 AM

@Niedzielski

I was planning to pick this task in the upcoming hackathon but seems like it's already been picked up. Please correct me if I am wrong.

@yashasvi You're welcome to pick it up, but we'd like it to be completed before the hackathon.

yashasvi claimed this task.Mar 2 2018, 3:33 PM

@yashasvi You're welcome to pick it up, but we'd like it to be completed before the hackathon.

Sure @Dbrant , I'd love to pick this up (Have been wanting to contribute more for some time now ).

Will start investigating and update here.

Hi @Dbrant , I went through the code. Some observations/questions below (Correct me if I am wrong anywhere) :

  • We emit the events only on the main thread (ref: ThreadSafeBus class ). I am assuming we did this because of a self-defined contract which states that
    • we are using event bus only for consuming events that require change in the ui.
    • consumers should not do any intensive-work which could have been done on a background thread.

So, this is sort of a single pipeline. If there are multiple events at the same time, they would be emitted sequentially.

We can mimic this kind of single pipeline using Rx as described in the approach here. The baseline here is that we are using a

single observable for all the events

and therefore while consuming the events, we'll have to check specifically if this is the event we want to consume, similar to the code here.
While I don't like this style, this exactly mimics our current state and can be done as a first step.

The second solution is multiple pipeline model in which we'll basically create

one observable per event.

In this approach, we'll be transforming each of our *Event class into *ObservableEvent class.

I find the second approach to be more modular and testable.

Let me know what you think.

Dbrant added a comment.Mar 5 2018, 4:10 PM

@yashasvi For the sake of simplicity, let's proceed with the single pipeline approach for now, and then evaluate whether expanding to multiple pipelines will be worth the effort.

One other thing I'd like to monitor is the increase in size that this will cause for our APK, as well as the increase in method count. Last time I checked, we were getting rather close to the dex limit, and I'd like to know how much closer this will push it.

Change 416492 had a related patch set uploaded (by Yashasvi; owner: Yashasvi):
[apps/android/wikipedia@master] Introducing Rx as a replacement for event bus

https://gerrit.wikimedia.org/r/416492

@Dbrant ,

As you expected, it exceeds the 64k method count. I've made a proof of concept patch (had to enable multidex) which replaces otto with rx for ThemeChangeEvent.

minify was not enabled at that time. After rebasing, I noticed that we have enabled minifying for debug builds as well. Multidex is not needed now.
Here's the dex method count for a dev debug build:

master: 48603

After including rx: 48773

@Dbrant ,
As you expected, it exceeds the 64k method count. I've made a proof of concept patch (had to enable multidex) which replaces otto with rx for ThemeChangeEvent.

Change 416492 merged by jenkins-bot:
[apps/android/wikipedia@master] Rx: Remove Otto event bus!

https://gerrit.wikimedia.org/r/416492

Dbrant closed this task as Resolved.Oct 9 2018, 1:12 PM