Skip to content
This repository was archived by the owner on Apr 19, 2018. It is now read-only.

onCreateOptionsMenu is called for every Fragment inside the ViewPager #48

Closed
ChrisSmith opened this issue Jul 28, 2011 · 17 comments
Closed

Comments

@ChrisSmith
Copy link
Contributor

When using Fragments inside the ViewPager, every Fragment that is partially visible (left page, right page and visible page) and has called setHasOptionsMenu(true) will have onCreateOptionsMenu called, causing the items to appear in the action bar.

The correct (implied?) behavior would be to only have the currently displayed Fragment show its menu items.

The issue comes down to how ViewPager operates, it holds up to three Fragments in its ViewGroup at a time, and they are all added to the FragmentManager. So when FragmentManager.dispatchCreateOptionsMenu is called, all of the menu items are created as-if all the Fragments were visible (because they technically are).

So far I've been unable to successfully prevent this behavior (using the public class methods) because of the order ViewPager.onPageSelected is called in. Since it is called before the next Fragment is added to the ViewGroup, I can't disable the fragments menu. My temporary solution was to modify ViewPager, but I doubt you want to go this route since ViewPager will likely change in future releases of the ACL.

I'm not sure how you want to handle this one, the issue is outside ActionBarSherlock but within the ACL. I guess filing a bug to source.android.com?

@JakeWharton
Copy link
Owner

Yes, file a bug on b.android.com and add the link to this issue. You can also post to the official android developers mailing list where Dianne or another engineer might post a workaround.

My temporary solution was to modify ViewPager, but I doubt you want to go this route since ViewPager will likely change in future releases of the ACL.

On the contrary, I have already fixed a few bugs in the library and would gladly accept more. My release cycle is a lot quicker than Google's compat lib so it allows any changes to be distributed more rapidly.

JakeWharton added a commit that referenced this issue Aug 10, 2011

Verified

This commit was signed with the committer’s verified signature.
connor4312 Connor Peet
…Refs #48.
@JakeWharton
Copy link
Owner

@ChrisSmith can you paste/email/pull request your solution so I can take a look at it and possibly implement it?

@ChrisSmith
Copy link
Contributor Author

Sure, I hadn't posted it because its a hack and I'm not really happy with it.

At the end of setCurrentItemInternal inside ViewPager, I just iterate over the fragments and disable their options menu.

    for(ItemInfo info : mItems){
            if(info.object instanceof Fragment){
                Fragment f = ((Fragment)info.object);
                f.setHasOptionsMenu(info.position==item);
            }
        }

It would be better if this could be handled by the OnPageChangeListener but the ArrayList<ItemInfo> mItems is private, and the ItemInfo class is too.

@JakeWharton
Copy link
Owner

You're right, that is a hack :) I filed this on b.android.com and will take a look at implementing a fix hopefully before the 3.1.3 release this weekend.

I'm fine with changing the behind-the-scenes code of the compat-lib sources as long as the exposed API doesn't change and it still functions as a drop-in replacement to the official jar.

@blork
Copy link

blork commented Aug 25, 2011

I've been plagued by this bug for ages. The fix above doesn't seem to work on the first item in a pager - I've found that adding it to the end of the populate() function works best.

@ChrisSmith
Copy link
Contributor Author

I think I ended up just manually setting it for the first one (but like I said this is a hack). I've got a better implementation now that also address Issue 56 that I'll post up soon

@JakeWharton
Copy link
Owner

Excellent I look forward to it. I was thinking of including a boolean in the info for each fragment as well to remember its original mHasMenu value. This way we handle a case where a fragment would (for who knows what reason) expose its menu in one situation but not another.

@ChrisSmith
Copy link
Contributor Author

That's actually pretty close to what I did.

I modified the OnPageChangeListener interface to include two more methods, and I adjusted the method I posted earlier

public void onPageDeselected(Object obj);
public void onPageSelected(Object obj);


public void updateSelectedPage(int item){
        if(mOnPageChangeListener==null) return;

        for(ItemInfo info : mItems){
            if(info.position==item){
                mOnPageChangeListener.onPageSelected(info.object);
            }else{
                mOnPageChangeListener.onPageDeselected(info.object);
            }
        }
    }

Then in my base Fragment class

    private boolean hasOptions = false;
    private boolean isHidden = false;

    public boolean isFragmentHidden(){
        return isHidden;
    }


    @Override
    public void setHasOptionsMenu(boolean hasOptions){
        this.hasOptions = hasOptions;
        super.setHasOptionsMenu(hasOptions);
    }

    @Override
    public void onFragmentShown() {
        isHidden = false;
        if(hasOptions) super.setHasOptionsMenu(true);
    }


    @Override
    public void onFragmentHidden() {
        isHidden = true;
        super.setHasOptionsMenu(false); 
    }

So the OnPageChangeListener is responsible for calling the two new methods as needed

        @Override
    public void onPageDeselected(Object obj) {
        ((MyFragment) obj).onFragmentHidden();
    }

    @Override
    public void onPageSelected(Object obj) {
        ((MyFragment) obj).onFragmentShown();
    }

This applies to #56 because inside public boolean onContextItemSelected(MenuItem item) we can first make a call to isFragmentHidden and return false, allowing other fragments to continue processing the context item.

JakeWharton added a commit that referenced this issue Aug 28, 2011
@JakeWharton
Copy link
Owner

Thanks for the patch. It inspired me to find a solution which did not require modifying OnPageChangeListener (in order to maintain compliance with the official compat lib).

Give it a try (a60d03a) and see what you think. I didn't get a change to test it against #56 so if you have a test case for that please give it a try as well.

@ChrisSmith
Copy link
Contributor Author

The menu items appear to be working correctly with your implementation.

However I feel like modifying the internals of FragmentManager has more consequences than modifying OnPageChangeListener, ie completely different behavior on the 3.0 SDK and the ACL. That said, I'd like to see Google acknowledge this issue and let us know if they are going to address this or not. Besides anyone using ABS will likely stick with it for a substancial period of time just for the ActionBar pre-3.0 support.

@JakeWharton
Copy link
Owner

I prefer something along the lines of your method where determining whether or not to contribute a menu is left to the fragment implementation rather than handled by the library directly. On the other hand, I don't like changing APIs that I didn't create, even if they seem like something that should have existed in the first place.

The ViewPager is only available with the compat lib and thus modifying Fragment and FragmentManager would only affect users of this library. Since you are forced to use the library-implementations of those two classes as well, it shouldn't make a difference that we are tweaking the functionality. If future versions of the SDK do implement ViewPager natively then I would without a doubt opt to mirror their implementation.

I posted to the Android developers group so we'll see if anyone has an opinion there. It would be nice to catch Dianne's attention and get her input as she seems to be the steward of the compat lib.

I might try to hop in the weekly Hangout on G+ with Reto Meier and the other Android devs and/or Romain Guy on IRC to see if they have any thoughts as to what would be an appropriate direction to steer this.

I will hold off releasing until I can think about it more.

JakeWharton added a commit that referenced this issue Aug 29, 2011
…booleans that come with participating in a ViewPager. Refs #48.
@JakeWharton
Copy link
Owner

So the more that I think about it the more that I think I am favoring a form of your solution. I was thinking of this method declaration:

void onPageSelectionStateChanged(int position, Object object, boolean selected);

Your use of two methods is a bit misleading because they may be called more than once for each state depending on the calls to populate(). I've just combined them into a single method which will allow you to react appropriately based on the selected argument. It also provides both the page position as the other callbacks do as well as the object to allow us to get our fragment instance out.

What do you think about that?

JakeWharton added a commit that referenced this issue Aug 30, 2011
…e technique that will rely on user implementation to fix the actual bugs. Samples to follow.
@ChrisSmith
Copy link
Contributor Author

That seems fine to me. Because of where I had placed the call to notify the listener ( inside setCurrentItemInternal) the methods were only called once and not repeatedly like populate() but it doesn't get called initially (as blork mentioned). The extra information will be useful to someone I'm sure.

@JakeWharton
Copy link
Owner

The reason I moved it to populate() was because its implementation removes items from the mItems array if you are jumping quickly or by more than one page via the API. This could probably be overcome by storing additional references to the ItemInfo instance for the last selected page and then placing the call only in setCurrentItemInternal. I'll have to experiment.

Also Dianne weighed in on the situation on the mailing list: https://groups.google.com/d/msg/android-developers/_opT-Lr_hgE/f4dLIpHbK-4J

@ChrisSmith
Copy link
Contributor Author

Nice to see the team taking a look at this.

@JakeWharton
Copy link
Owner

Released with 3.2.0

@mubashirmeddekar
Copy link

Can Anybody please help me, i am trying to use both Vertical and Horizontal Swipes in one Pager.? Please help to solve this.!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants