calling super from onOptionsItemSelected in ActivityMenuComponent

[ghost]

the usual implementation of onOptionsItemSelected in Activity subclasses needs to call super.onOptionsItemSelected if it doesn't handle the selection (i.e. return true)... how do you think the best way to tackle that is? Or am I missing an obvious solution that's already in place?

[rehos]

+1 I was missing the call to super too.

[johncarl81]

I missed this... I'll try to tackle the implementation this evening.

[johncarl81]

So, the question is here, should we call the super automatically or pass into the call-through method an object that can call the super, ie:

Automatically calls super:

client code:

public boolean onOptionsItemSelected(MenuItem menuItem){
    //same at today
}

generated code:

public boolean onOptionsItemSelected(MenuItem menuItem){
    boolean call = delegate.onOptionsItemSelected(menuItem);
    if(!call){
        return super.onOptionsItemSelected(menuItem);
    }
    return true;
}

Or with an object to call the super (super handling is in the hands of the user): client code:

public boolean onOptionsItemSelected(MenuItem menuItem, SuperCaller caller){
    //handles menus
    //default case:
    superCaller.call();
}

generated code:

public boolean onOptionsItemSelected(MenuItem menuItem){
    //define SuperCaller?
    return delegate.onOptionsItemSelected(menuItem, new SuperCaller());
}

Super caller would look like this:

class SuperCaller{
    public boolean call(){
        return MenuActivity.super.onOptionsItemSelected(menuItem$$0);
    }
}

I feel like introducing the SuperCaller respects the api more and leaves the implementation in the hands of the developer. Thoughts?

[ghost]

The supercaller is exactly what I was thinking and far more flexible.

[johncarl81]

Ok, threw together an implementation on this branch: https://github.com/johncarl81/transfuse/tree/supercaller Let me know how this works for you.

My open question is: do we need a SuperCallable on every call-through method?

[ghost]

Do you think this is going to be flexible enough? This approach doesn't allow me to control the params passed to the super method or to call any other protected super methods that I may need to access..

[johncarl81]

Im not 100% convinced it will be, thus this branch. What other super methods would you like to access?

[ghost]

I guess I can't really think of a circumstance I'd want to call a protected method that wasn't the one I'm overriding.. but there are probably some cases where I'd want to change the params... although I can't think of any off-hand...

[ghost]

It looks like I need to access super in more spots...

http://stackoverflow.com/a/7953801/2031

can the SuperCallable be made optional? If it's present in the delegate's method signature it's passed in, otherwise the current behavior is the norm?

[johncarl81]

I'd call onDestroyView() a lifecycle event, not a call-through event. These can, and most of the time, call super. In this case should super.onDestoryView() be called last?

[ghost]

Well, it looks like this particular go is a red herring... the fix was in another castle.

I think ultimately giving the delegate the option to call super at the begining, end or middle could be useful. I like the default behavior 99% of the time as it's clean and easy... but I think the ability to get a handle to a SuperCallable would be a huge benefit where you need to hack around a lifecycle weirdness or whatever.

[johncarl81]

Reworking the core of transfuse allowed an implementation of the super caller. The general idea is for Transfuse generate a class in the Activity component that would have control of the super methods. This super-calling method is defined by giving an annotation with a method signature on either a class or method like so:

@Activity public class Menu {
    @ManualSuper(name = "onOptionsItemSelected", parameters = MenuItem.class)
    public boolean onOptionsItemSelected(MenuItem menuItem) {
        //...
    }
}

Would generate the following:

public class MenuActivity extends android.app.Activity {
    public MenuActivity.SuperCaller SUPER = new MenuActivity.SuperCaller();
    //...
    public class SuperCaller {
        public boolean onOptionsItemSelected(android.view.MenuItem menuItem$$0) {
            return MenuActivity.super.onOptionsItemSelected(menuItem$$0);
        }
    }
}

Transfuse then defines an instance of this super caller class that allows you to make the relavent calls:

@ManualSuper(name = "onOptionsItemSelected", parameters = MenuItem.class)
    public boolean onOptionsItemSelected(MenuItem menuItem) {
        return ((MenuActivity) context).SUPER.onOptionsItemSelected(menuItem);
    }

This also cancels any pre-defined super call that would be made on a generated method, effectively allowing you to manually call the super wherever is appropriate. Additionally, this allows you to not call the super method for predefined methods (except for creation methods, like onCreate).

I've pushed and deployed a working version of this here: b79dc635845e975ac6ad1c63621f5522719bb457 and under 0.3.0-SNAPSHOT. Would love some feedback.

[dbachelder]

It looks like with @OnBackPressed, the call to super is now canceled even if @ManualSuper is not used.

Additionally, I really dislike casting to the generated component to access the SUPER.. couldn't that be passed into me? Or injected into the constructor, or something? Or put on some interface... e.g. SuperActivity.getSuperCaller()

[dbachelder]

This is affecting onCreates in services too.. I think it's pretty systemic.

[dbachelder]

the ServicePlugin is missing .superCall() on all method defs.. ActivityPlugin is missing it on onBackPressed.. this fixed my local build enough to keep going. I didn't make the changes on a fork, but I can do that tomorrow if you don't get to it.

[dbachelder]

here is a PR for the fix https://github.com/johncarl81/transfuse/pull/136

[johncarl81]

Awesome, thanks Dan. I was struggling with what should require a super call and what shouldn't... I'll need to review a bit.

In terms of the super caller, yes, we should make an injection for this, probably by type:

@Inject MenuActivity.Super superCaller;

Is this what you're talking about?

[dbachelder]

That injection would be an improvement although it still stinks needing to reference the generated component... I've always envisioned getting something that would have a callSuper(Object... params)...

class MyClass {
  private final SuperRegistry sr

  @Inject
  MyClass(SuperRegistry sr) {
   this.sr = sr;
  }

  @ManualSuper("onCreate")
  @OnCreate
  public void onCreate(Bundle savedInstanceState) {
    sr.lookup("onCreate").callSuper(savedInstanceState);
  } 

  @ManualSuper("onBackPressed")
  @OnBackPressed
  public void onBackPressed() {
    sr.lookup("onBackPressed").callSuper();
  }
}

or even better....

class MyClass {
  @ManualSuper()
  @OnCreate
  public void onCreate(SuperCaller sc, Bundle savedInstanceState) {
    sc.callSuper(savedInstanceState);
  } 

  @ManualSuper()
  @OnBackPressed
  public void onBackPressed(SuperCaller sc) {
    sc.callSuper();
  }
}

[dbachelder]

re: "I was struggling with what should require a super call and what shouldn't... I'll need to review a bit."

Everything should be semantically the same as before this refactor, right? So, anywhere that was calling super.*() (everywhere?) should still be calling super still?

[johncarl81]

I like your super caller syntax better, and it would support the base class in a more general way. What do you think about the following:

interface SuperCaller{
    <T> T callSuper(String methodName, Object... parameters);
}

I'm leaning towards injection instead of a method parameter, mainly because the super call may not correspond to the given event method, ie:

class MyClass {
    private final SuperCaller sc

    @Inject
    MyClass(SuperCaller sc) {
        this.sc = sc;
    }

    @ManualSuper(name = "someMethod", parameters = {Value.class, Value.class})
    @OnBackPressed
    public void onBackPressed() {
        sr.callSuper("someMethod", value1, value2);
    }
}

One note on SuperCaller, it is generated after the onCreate() method is underway, so it will not block the creation of the onCreate() super call.

This lines up nicely with the existing functionality.. just need to add the injection node builder, and the API around SuperCaller.

[dbachelder]

The string lookup bothers me a bit.. we could eliminate that if the super was passed in via the actual method being called, but that would hurt the use case you have in mind for calling it from another method (although I have never encountered this...). I think as long as that callSuper() fails fast when it's called with an unknown method name, this is reasonable... and better than casting to the generated component as we're doing today... is there any reason that "someMethod" has to correspond to an actual method name? as long as the name="BLAH" matches callSuper("BLAH",...) we could use a constant... right?

[johncarl81]

The string lookup bothers me as well, not to mention the Object varargs. These would have to be validated at run time (unless we got really crafty with the Trees api). Thinking deeper about the calling-super from a different method, I think that's actually a non-issue. We should only be calling super from the corresponding event method.

We will need this capability at least in the @On-Event and Call-through events, which means we need to be able to inject the CallThrough class... unless we add it to each of the Call-through event interfaces.

[johncarl81]

If we made this a property of only event and call-through methods, we may not even need the @ManualSuper declaration... we could just do the following:

@RegisterListener
class MyClass implements ActivityMenuComponent{

  @OnBackPressed
  public void onBackPressed(SuperCaller sc) {
    sc.callSuper();
  }

  boolean onCreateOptionsMenu(Menu menu, SuperCaller sc){
    sc.callSuper(menu);
  }
}

FYI, looking at #133, I may be open to relaxing the @Callthrough method declarations to not require the interface. Would like your input on that one as well.

[dbachelder]

IMO, this version of SuperCaller is ideal... assuming the param is optional and super() behavior is the current default when it's not present.

re: #133 I love this. but in some ways it really opens up a tight coupling between components which I have been forced to avoid up until now.

[johncarl81]

Ok, this is implemented on 0.3.0-SNAPSHOT (d8301d176c462b95e81a5329c3d22cad6f9c2455) for lifecycle events. I'll implement call-through methods in a bit.

[dbachelder]

Nice! We have a release this week but after that we'll try it out

[johncarl81]

Not sure about the call-through implmentation... Here's what it looks like:

public interface ActivityMenuComponent {
    @CallThrough
    boolean onCreateOptionsMenu(Menu menu, SuperCaller superCaller);
    @CallThrough
    boolean onPrepareOptionsMenu(Menu menu, SuperCaller superCaller);
    @CallThrough
    boolean onOptionsItemSelected(MenuItem menuItem, SuperCaller superCaller);
    @CallThrough
    boolean onMenuOpened(int featureId, Menu menu, SuperCaller superCaller);
    @CallThrough
    void onOptionsMenuClosed(Menu menu, SuperCaller superCaller);
}

This seems like a lot to add. @dbachelder, what do you think about relying on the SUPER reference for call-through methods?

[dbachelder]

Maybe I'm missing the point of call-throughs... Why are these special and requiring an interface at all? shouldn't they each be overridable in the delegate without bringing the rest along? I know I have a few cases where I am implementing one or two of these and the rest are just there as useless boilerplate because the interface requires them.

[johncarl81]

The hallmark of the call-through feature is that they have a non-void return type and a specific method signature. Probably the most common call-through is the ActivityMenuComponent which has the following signature:

boolean onCreateOptionsMenu(Menu menu)

In designing this, I wanted the user's code to be structured so that they couldn't make a mistake. Notice, in order to match the onCreateOptionsMenu method the user must choose the correct return type, the correct method name and the correct method parameter types, all dictated and structured by implementing the ActivityMenuComponent interface.

As you know, it has been proposed to make this less structured (#133), but that means we'll open up some additional chances for the user to make a mistake.

I decided to group together like-methods (for instance: menu call-throughs) to cut down on the number of interfaces that would have to be added to accommodate this. In retrospect, maybe this isn't best because of the boilerplate required (one of the primary things you use Transfuse to avoid).

Honestly I don't know which direction to go with this feature currently. I'm not a huge fan of adding another parameter to the callthroughs to support the SuperCaller to each of the interface methods. Maybe the best thing to do is to go the route of #133 and remove the interfaces and allow you to just tack on the SuperCaller as we do with the lifecycle event methods. This will just require some validation on Transfuse's part to help the user not get into an error case. Transfuse could also do some work around mapping your method to the original component methods, ie:

@CallThrough(name="onCreateOptionsMenu", parameters=Menu.class)
void makeMeAMenu(SuperCaller superCaller);

1. Transfuse would map parameters as it does with lifecycle events (allowing you to leave them off if they aren't used).
2. Methods could be named whatever the user wants
3. SuperCaller could be provided if requested
4. void could be used instead of the regular return type.

Heh, went a little off the rails there, did I answer your question? What are your thoughts on this?

[dbachelder]

I get it now. I much prefer the suggested @CallThrough technique to the interface boilerplate. I would also love to just see all of these things as well known annotations @OnCreateOptionsMenu... etc.. that seems like a bunch of work... but that would make it very clear what everything is without having to name things with strings or manually specify params.