Conductor: Menu items for both controllers are visible in toolbar during transaction

[dimsuz]

I have an app with two simple Controllers and it performs an animated transaction (with a HorizontalChangeHandler()) between them. Each controller has setHasOptionsMenu(true) and has its own set of toolbar icons inflated in onCreateOptionsMenu().

The problem I am seeing is that right after switching animation starts both sets of icons are visible in toolbar, i.e. new ones are added right away. After animation finishes only second set of items is left visible, so the end state is correct.

But it's a bit weird to see them jump like that and seeing both sets at once. Is this an expected behavior (for which there's some traditional solution)? Or is this some bug?

I am using a freshly baked Conductor version: 2.0.1

[EricKuck]

Currently controllers can have menu items if they're attached. In your case, both the old and new controllers are attached while the transition is occurring. This can (should) be updated so that views transitioning out (or maybe in?) don't have their items visible during the transition.

One thing I'm not sure about though - which controller should "win" in this scenario? Would the old controller's menu items disappear as soon as the transition starts? Or would the new controller's menu items not appear until the transition is complete?

[dimsuz]

Yes, this is a bit two-fold. For my scenario I think that showing new actions after transition is completed is a way to go, but I suspect that there might be other scenarios in which the opposite will be true...

I have also seen that it is probably not so complex to implement either behaviour by calling controller.setOptionsMenuHidden() at the correct times in some router change listener, but I didn't try (yet).

Decided to bring this up in case it has some clear solution which I do not know about (only starting to learn this nice library)...

[EricKuck]

You're right, setting hidden to true when the transition starts and back to false when it re-appears would fix this completely. It would be nice if this just worked the way people wanted, but I'm not really sure what the right approach is. I'm pretty sure the same issue exists with fragments, right?

[dimsuz]

Sadly (or either happily) I'm not a big fragments user, so can't say right away...

OK, so I can go for hiding/showing for now, if you'll decide to leave it be, it's safe to close this issue I guess. Maybe docs can be updated somewhere if applicable...

[PaulWoitaschek]

Any update on this?

For me it worked like this

    router.addChangeListener(new ControllerChangeHandler.ControllerChangeListener() {
      @Override
      public void onChangeStarted(@Nullable Controller to, @Nullable Controller from, boolean isPush, @NonNull ViewGroup container, @NonNull ControllerChangeHandler handler) {
        if (from != null) from.setOptionsMenuHidden(true);
      }

      @Override
      public void onChangeCompleted(@Nullable Controller to, @Nullable Controller from, boolean isPush, @NonNull ViewGroup container, @NonNull ControllerChangeHandler handler) {
        if (from != null) from.setOptionsMenuHidden(false);
      }
    });

What about including a solution for this that is a little more intelligent and tracks the option menu state that was set before based on the controller instance id and restores the correct state afterwards?

[EricKuck]

Yeah, I have a very similar block of code in just about every app I write as well, which is pretty annoying. I'm not sure how to add a change of behavior like this to the library without potentially messing up people's existing code. It seems like this either needs to be an opt-in behavior or wait until the next major version release. Any thoughts?

[PaulWoitaschek]

Thats a good idea. Make it opt-in and upon the next release opt-out.

[EricKuck]

Preferences on how you'd opt-in? Maybe setHasOptionsMenu(boolean hasOptionsMenu, boolean handleHideOnChange)?

Also, what should the functionality of this be? I just realized that what I typically do and what you wrote above are actually pretty different. Here's how I handle it (in a base controller):

    override fun onChangeStarted(changeHandler: ControllerChangeHandler, changeType: ControllerChangeType) {
        super.onChangeStarted(changeHandler, changeType)
        setOptionsMenuHidden(!changeType.isEnter)
    }

This means that the menu shown is always for the controller that is either visible or in the process of being transitioned in.

[PaulWoitaschek]

setHasOptionsMenu(boolean hasOptionsMenu, boolean handleHideOnChange) sounds good.

I do not understand enough of the internals of conductor. What is the behavior difference between these two ways?

[dimsuz]

On a side note: this is where we need documentation. I've used both ways in different situations and each time had to do some experimenting to understand where exactly in lifecycle these get called.

[dimsuz]

For example I had questions like "is controller attached at this point?" Both for enter and exit callbacks/arguments.

[nomisRev]

Ideally some crossfade option should be available. But I guess that's up for the user to implement. So perhaps a good demo / documentation would solve this.

On a side note, I don't think menu items should change a lot. Unless you have a good use case. Care to give examples?

Normally I only add back buttons or animated from semi-transparent to full color. To indicate a level deeper in the app.

[PaulWoitaschek]

I think crossfade is really out of scope. In my apps I use a lot of menu items. For example material full screen dialogs with a "DONE" text to the right, filter icons, ...

[PaulWoitaschek]

Something else: I just refactored my open source project to not use the actionbar at all. That way I can handle the Toolbar as a simple view and don't have these issues at all.
Also I don't need to have this weird state tracking with invalidateOptionsMenu, keeping track of changes and applying them in onOptionsMenuPrepared. I just set it up in onCreateView and I'm done.