airbnb / RxGroups

Easily group RxJava Observables together and tie them to your Android Activity lifecycle
Apache License 2.0
693 stars 44 forks source link

Make ObservableGroup#resubscribe visible to generated code, artificat for rx-processor #38

Closed BenSchwab closed 7 years ago

BenSchwab commented 7 years ago

I don't love making tag public, but the other option is generating all code under com.airbnb.rxgroups which feels wrong.

We could create some sort of hacky getter/setter that requires some special private class which is added to the generated code. Worth it? other ideas?

BenSchwab commented 7 years ago

cc @elihart

elihart commented 7 years ago

ah, I remember putting this problem off for later, and then forgot about it :P I agree that we should avoid a public tag

I think a hacky solution is ok as long as it is transparent to the user. Could we have a package private super class for AutoResubscribingObserver.java, maybe AutoResubscribingObserverImpl that has a package private access to set the tag? If the tag needs to have a public getter we could expose that.

This way users would still extend AutoResubscribingObserver and not see any of the tag ugliness.

BenSchwab commented 7 years ago

@elihart We need to have the setter be public because the generated code which can be in an arbitrary package can use it.

package com.some.package;

/**
 * Generated file. Do not modify! */
public class MainActivity_ObservableResubscriber {
  public MainActivity_ObservableResubscriber(MainActivity target, ObservableGroup group) {
    target.observer.setTag("MainActivity_observer"); //needs to be public
    group.resubscribe(target.observer);
  }
}

Not quite sure I follow how a super class could provide access to a tag setter or getter, and a sub class could hide it.

My feeling is that the best bet is to keep things simple -- so make the tag public, but add a JavaDoc explaining what the setTag method is for, and that generally, it should not need to be called by the user.

elihart commented 7 years ago

@BenSchwab Ah sorry, I mixed things up. The ObservableResubscriber would extend a base class that is in the rxgroups package. That base class can have a protected method to set the tag, and it would be in the same package as the observer so it can change the tag. There wouldn't be a need for a base class for AutoResubscribingObserver.

public class MainActivity_ObservableResubscriber extends BaseObservableResubscriber {
  public MainActivity_ObservableResubscriber(MainActivity target, ObservableGroup group) {
    setTag(target.observer., "MainActivity_observer"); //needs to be public
    group.resubscribe(target.observer);
  }
}

I don't think that adds much complexity, but leaving the tag public and documenting is probably ok.