Closed Bug 966047 Opened 10 years ago Closed 10 years ago

Hide home banner when there are no panels enabled

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox29 fixed, firefox30 verified, fennec29+)

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- verified
fennec 29+ ---

People

(Reporter: jdover, Assigned: Margaret)

References

Details

Attachments

(1 file, 8 obsolete files)

We may want to hide the home banner on the awesomescreen if the user has disabled all panels. Though, this may cause the user to miss important snippets?
Ian, any input here? My take on this is that we should always show snippets, even all panels are disabled.
Flags: needinfo?(ibarlow)
Heh, I just mid-aired with Lucas, and I was going to say I don't think there will be any snippets important enough to merit showing a banner when the user chose to have a totally empty about:home. Doesn't it seem kinda weird to show a banner and nothing else?
Part of me agrees with Lucas, part of me agrees with Margaret. 

On the one hand, snippets are a great way to talk to people, and on the other hand I can see the kind of person who turns everything off getting really pissed if they see a snippet. Hiding everything sends a pretty clear message that the user doesn't want to be disturbed, and wants to go their own route. I'm not sure if "stop spamming me with your Mozilla crap" is a kind of future bugzilla discussion I really feel like engaging in. So for personal reasons I think I just convinced myself side with Margaret on this one. ;)
Flags: needinfo?(ibarlow)
Fair points. I'd be more inclined to agree if we were talking just about the Mozilla snippets. However, the same UI element (the 'home banner') can also be used by add-ons. Not showing banners triggered by add-ons just because the user disabled all panels might be a bit confusing because the connection between "I want no panels in my start page" and "I don't want to see any snippets" is not that obvious. I feel like these are (almost) orthogonal choices.

Just showing a white blank page with snippets at the bottom of the screen might look a bit weird though because you don't have a clear sense that you're in the home page i.e. "it's just a blank page. Why am I seeing these notifications here?". Bug 965361 is especially important in this regard.

I don't feel strongly about this but we might be getting a bit ahead of ourselves by assuming that "hiding all panels" necessarily means a "don't show me any snippets" intent, especially in the presence of add-ons.
Yeah, the watermark would help. I'm not so worried about how it *looks* as I am about the user's intent when they said "I don't want a homepage experience at all". 

Good point about add-ons though. I still think I'm more on the side of "just hide everything", but I could be persuaded otherwise.
Assignee: nobody → jdover
Is there a way we could include the "also hide the Snippets banner" in the same UI for hiding the various home panels that isn't terrible?
Do we have a solution for 29?
tracking-fennec: --- → ?
tracking-fennec: ? → 29+
Blocks: home-banner
Attached patch patch (obsolete) — Splinter Review
Hides the banner when there are no panels enabled, also fixes bug where the banner was not showing when only one panel was enabled.
Attachment #8381048 - Flags: review?(margaret.leibovic)
Status: NEW → ASSIGNED
Comment on attachment 8381048 [details] [diff] [review]
patch

Review of attachment 8381048 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/home/HomePager.java
@@ +309,5 @@
>                      mDefaultPageIndex = i;
>                      setCurrentItem(i, false);
> +
> +                    // Android doesn't call this when there is only one item
> +                    if (count == 1 && i == 0) {

If count is 1, won't i always be 0? I think it would be better to just put this down below the for loop.

@@ +310,5 @@
>                      setCurrentItem(i, false);
> +
> +                    // Android doesn't call this when there is only one item
> +                    if (count == 1 && i == 0) {
> +                        mPageChangeListener.onPageSelected(0);

It feels smelly to be calling one of a listener's methods directly, especially as this affects mDecor, not just mHomeBanner.

Instead of doing this, I think you should just call mHomeBanner.setEnabled(true) in this case, since that's all we're really aiming to achieve.
Attachment #8381048 - Flags: review?(margaret.leibovic) → review-
Attached patch patch (obsolete) — Splinter Review
(In reply to :Margaret Leibovic from comment #9)
> If count is 1, won't i always be 0? I think it would be better to just put
> this down below the for loop.

Removed if check from loop.

> It feels smelly to be calling one of a listener's methods directly,
> especially as this affects mDecor, not just mHomeBanner.
> 
> Instead of doing this, I think you should just call
> mHomeBanner.setEnabled(true) in this case, since that's all we're really
> aiming to achieve.

I would argue that this a bug in Android, and that if someone were to come along and add anything else to the OnPageChangeListener, they will be confused why it's not being called when there or is only one page. Or they may not even notice, which could create more issues. I find this to be safer, as well simpler by keeping the control of the HomeBanner in one place inside of HomePager.
Attachment #8381048 - Attachment is obsolete: true
Attachment #8381548 - Flags: review?(margaret.leibovic)
(In reply to Josh Dover from comment #10)

> > It feels smelly to be calling one of a listener's methods directly,
> > especially as this affects mDecor, not just mHomeBanner.
> > 
> > Instead of doing this, I think you should just call
> > mHomeBanner.setEnabled(true) in this case, since that's all we're really
> > aiming to achieve.
> 
> I would argue that this a bug in Android, and that if someone were to come
> along and add anything else to the OnPageChangeListener, they will be
> confused why it's not being called when there or is only one page. Or they
> may not even notice, which could create more issues. I find this to be
> safer, as well simpler by keeping the control of the HomeBanner in one place
> inside of HomePager.

How does this interact with the tab strip on tablets? Before this patch, was that experiencing a similar problem?
Comment on attachment 8381548 [details] [diff] [review]
patch

Review of attachment 8381548 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/home/HomePager.java
@@ +312,5 @@
>                  }
>              }
> +
> +            // Android doesn't call this when there is only one item
> +            if (count == 1 && i == 0) {

You don't need to check i == 0.

@@ +314,5 @@
> +
> +            // Android doesn't call this when there is only one item
> +            if (count == 1 && i == 0) {
> +                mPageChangeListener.onPageSelected(0);
> +            }

Looking at the context here more closely, this doesn't even look like the right place to put this, since it's in an else statement, so we won't ever call this if we're setting the current panel based on mInitialPanelId.
Using my pocket panel add-on I just noticed that the banner can also appear if you directly open about:home to a non-default panel (using the "page" param on the about:home URL). I wonder if calling the page change listener would fix that issue.
Comment on attachment 8381548 [details] [diff] [review]
patch

Review of attachment 8381548 [details] [diff] [review]:
-----------------------------------------------------------------

After sitting on this overnight, you've won me over, and I agree it's okay to call onPageSelected. However, I still think we should move where we're doing this, so that we call it regardless of whether or not we're setting the current panel from mInitialPanelId. Address this issue, and I'll give this patch an r+.

This also makes me realize that we don't set mDefaultPageIndex if we follow that if case, but I think that's fine, since that will just mean we may not end up showing the banner until we follow the case where we iterate through the enabled panels.
Attachment #8381548 - Flags: review?(margaret.leibovic) → review-
Attached patch patch (obsolete) — Splinter Review
(In reply to :Margaret Leibovic from comment #13)
> Using my pocket panel add-on I just noticed that the banner can also appear
> if you directly open about:home to a non-default panel (using the "page"
> param on the about:home URL). I wonder if calling the page change listener
> would fix that issue.

Fixed your comments, but I couldn't reproduce this bug, so I'm not sure if this is related. Regardless, anything that uses setCurrentItem(x) which will now trigger the onPageSelected even when there is only 1 item.
Attachment #8381548 - Attachment is obsolete: true
Attachment #8382572 - Flags: review?(margaret.leibovic)
Attachment #8382572 - Flags: review?(margaret.leibovic) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4bc6027f5638
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
Josh, can you request to uplift this to aurora?
Flags: needinfo?(jdover)
Comment on attachment 8382572 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): refactoring of HomeBanner in bug 960359.
User impact if declined: Banner will show when a user has specified to have no home content, which is new since bug 960359.
Testing completed (on m-c, etc.): landed on m/c 02/28
Risk to taking this patch (and alternatives if risky): similar to other HomeBanner bugs, the hide/show logic is complex and it could show the banner when not appropriate.
String or IDL/UUID changes made by this patch: none
Attachment #8382572 - Flags: approval-mozilla-aurora?
Flags: needinfo?(jdover)
Attachment #8382572 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 978741
Backed out for causing bug 978741:
https://hg.mozilla.org/releases/mozilla-aurora/rev/a48b46a28ee7
https://hg.mozilla.org/integration/fx-team/rev/8b7f3531e937

I'm also backing out the patch we landed in bug 978741, which didn't work to fix the issue.

After talking on IRC, I think we know what the problem there was. Let's just start with a new patch here, let that bake on Nightly, and then uplift to Aurora once we know that's good.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch patch (obsolete) — Splinter Review
This preserves the mDecor.onPageSelected call that was being made before and was accidentally removed.
Attachment #8382572 - Attachment is obsolete: true
Attachment #8386392 - Flags: review?(margaret.leibovic)
Comment on attachment 8386392 [details] [diff] [review]
patch

Review of attachment 8386392 [details] [diff] [review]:
-----------------------------------------------------------------

r- because this doesn't look like it's based on the right revision. However, I also think we need to either just call mHomeBanner.setActive directly (my preferred approach), or just replace the mDecor.onPageSelected(item) call with mPageListener.onPageSelected(item).

::: mobile/android/base/home/HomePager.java
@@ +220,5 @@
>  
>          // Android doesn't call onPageSelected when there is only one page. Make sure we activate
>          // the banner in this case.
>          if (mHomeBanner != null && getAdapter().getCount() == 1) {
>              mHomeBanner.setActive(true);

This doesn't look right... you should base your patch off of the most recent fx-team.

@@ +226,5 @@
> +
> +        // Android doesn't call this when there is only one item
> +        if (getAdapter().getCount() == 1 && mPageChangeListener != null) {
> +            mPageChangeListener.onPageSelected(0);
> +        }

I know you're not a fan of this, but I would really prefer if we just put in a mHomeBanner.setActive call, and avoid this attempt to call the page change listener itself.

Looking through the history here, this mDecor.onPageSelected call was added to address bug 907175. Although there isn't much useful history in that bug, I bet there was some similar underlying issue.

Actually, thinking about this change to call the listener itself, why aren't we just always calling it, similarly to what was done for mDecor? I don't really want to rat-hole on trying to get this listener approach to work, but if we are going to go for it, maybe we should just include:

if (mPageChangeListener != null) {
    mPageListener.onPageSelected(item);
}

Instead of including a check for the count and hard-coding the index.
Attachment #8386392 - Flags: review?(margaret.leibovic) → review-
Attached patch patch (obsolete) — Splinter Review
I guess I was taking the 'change as little as possible' approach after dealing with the bug from before, but this is definitely better to couple it all together.
Attachment #8386392 - Attachment is obsolete: true
Attachment #8386441 - Flags: review?(margaret.leibovic)
Comment on attachment 8386441 [details] [diff] [review]
patch

Review of attachment 8386441 [details] [diff] [review]:
-----------------------------------------------------------------

I just applied this to test, and the banner still appears when disabling all the panels (the whole point of this bug). Looks like you left out this bit:
https://hg.mozilla.org/mozilla-central/rev/4bc6027f5638#l1.72

However, I was testing with that back in place, and when changing the default panel, the banner would disappear, probably because we're not updating mDefaultPageIndex. I think this is a problem that existed with the original patch that we just didn't find earlier, so I think we need to change where we put that mHomeBanner.setActive(false); call. We should probably only do that when there actually are no panels enabled. I think having the banner hang around on a non-default panel immediately after the default changes is less awkward than having it disappear then reappear.

::: mobile/android/base/home/HomePager.java
@@ +218,5 @@
>      public void setCurrentItem(int item, boolean smoothScroll) {
>          super.setCurrentItem(item, smoothScroll);
>  
> +        if (mPageChangeListener != null) {
> +            mPageChangeListener.onPageSelected(item);

Taking a step back and remembering the root issue here, I actually don't think we should just call mPageChangeListener.onPageSelected all the time, since we're going to end up with duplicate calls in this case. The real issue is that onPageSelected isn't called when there is only one page, so we should just be doing this when there's only one page. However, that doesn't explain the issue with mDecor, so there must be something else going on here, and I feel like I don't want to spend more time investigating this.

Along the lines of 'change as little as possible', I would actually like to see us just address the actual home banner issue here, and just add something like:

// Android doesn't call the page change listener when there is only one item,
// so we need to make the banner active ourselves here.
if (getAdapter().getCount() == 1 && mHomeBanner != null) {
    mHomeBanner.setActive(true);
}
Attachment #8386441 - Flags: review?(margaret.leibovic) → review-
Attached patch patch (obsolete) — Splinter Review
This simply deactivates the banner when there is only one item in the adapter. Also found a bug where the banner could be activated after focusing and then bluring the toolbar (when there were no items). I decided to remove the banner completely when all panels are hidden. This means if the user shows panels later, the banner will not show until the next time Fennec is launched (which seems acceptable to me).
Attachment #8386441 - Attachment is obsolete: true
Attachment #8386967 - Flags: review?(margaret.leibovic)
(In reply to Josh Dover from comment #26)
> Created attachment 8386967 [details] [diff] [review]
> patch
> 
> This simply deactivates the banner when there is only one item in the
> adapter. Also found a bug where the banner could be activated after focusing
> and then bluring the toolbar (when there were no items). I decided to remove
> the banner completely when all panels are hidden. This means if the user
> shows panels later, the banner will not show until the next time Fennec is
> launched (which seems acceptable to me).

Good tradeoff. Honestly, people shouldn't be changing around their configurations that often, so I think we should just optimize for not having them end up in a broken-looking state after reconfiguring. I think this makes bug 976185 a WONTFIX.
Comment on attachment 8386967 [details] [diff] [review]
patch

Review of attachment 8386967 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch with the editing mode issue. I would normally say that's out of scope for this bug, but at this point I would rather us just fix things here than file more bugs. However, I think we should take a different approach to fix it.

::: mobile/android/base/home/HomePager.java
@@ +295,5 @@
>              mTabStrip.setVisibility(View.INVISIBLE);
> +            // Remove the HomeBanner completely so that it doesn't accidentally get activated by
> +            // something else such as a toolbar blur.
> +            ((ViewGroup) getParent()).removeView(mHomeBanner);
> +            mHomeBanner = null;

I don't like this. Using getParent() to get the view is not robust, since it will break if we ever decide to change the layout. There are also other places where we use mHomeBanner, and this could cause NPEs.

I think what we really should be doing is making sure we clear mDefualtPageIndex when we disable all panels, and if that's the case, we shouldn't ever show the banner on a non-default (or non-existent) panel.
Attachment #8386967 - Flags: review?(margaret.leibovic) → review-
Attached patch updated patch (obsolete) — Splinter Review
I started playing around with this myself, and this is what I came up with. This patch will also fix bug 976185 and bug 976670.

I found that the issue causing bug 976670 is that the onPageSelected listener isn't called when we open the pager to the first item, which is what we do on tablets, so I decided that we should just always call setActive within setCurrentItem. I realize this now matches your previous attempts to just call the onPageSelected listener in here, but I would prefer to just be explicit within setCurrentItem, in case we add more things to the onPageSelected listener later (since in this case we aren't actually working around an Android bug, but just want different behavior).

If you think this is good, we can get a third opinion (since we now both worked on this patch), and then land it.

Playing around with this on my N7, I couldn't get into a broken state, but I'd encourage you to try to find a flaw with it :)
Attachment #8387157 - Flags: feedback?(jdover)
Attachment #8386967 - Attachment is obsolete: true
Comment on attachment 8387157 [details] [diff] [review]
updated patch

(In reply to :Margaret Leibovic from comment #29)
> attempts to just call the onPageSelected listener in here, but I would
> prefer to just be explicit within setCurrentItem, in case we add more things
> to the onPageSelected listener later (since in this case we aren't actually
> working around an Android bug, but just want different behavior).

Are we not working around an bug or at least a difference in the way this we interact with the HomePager on tablets (bug 976670)? I still think it's more likely that in a future addition, someone adds something to setCurrentItem or onPageSelected but not the other and it creates some sort of inconsistency (that would be hard to detect). That being said, I don't see this as a important issue to keep discussing to block this bug any further if you feel this is the right way to go ahead.
Attachment #8387157 - Flags: feedback?(jdover) → feedback+
Comment on attachment 8387157 [details] [diff] [review]
updated patch

At this point, I feel like this is the lowest risk approach, so I'd like to go with this. Let's ask Lucas for review, since he also reviewed the change to add that mDecor.onPageSelected call in there.
Attachment #8387157 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8387157 [details] [diff] [review]
updated patch

Review of attachment 8387157 [details] [diff] [review]:
-----------------------------------------------------------------

mDefaultPageIndex might not be updated if mInitialPanelId is set to a valid position in the pager (the 'if' at the end of the method). This might cause problems to the setActive(item == mDefaultPageIndex) call above.

::: mobile/android/base/home/HomePager.java
@@ +301,5 @@
> +
> +            mDefaultPageIndex = -1;
> +            if (mHomeBanner != null) {
> +                mHomeBanner.setActive(false);
> +            }

I'd prefer this code to be placed together with the loop that sets mDefaultPageIndex down at the end of this method to avoid confusion.
Attachment #8387157 - Flags: review?(lucasr.at.mozilla) → review-
Josh, I can just finish this bug and let you focus on other things :)
Assignee: jdover → margaret.leibovic
Good catch, we had previously discussed the issue that mDefaultPageIndex wasn't updated in that if statement, but we didn't actually fix that. This patch breaks the "updating mDefaultPageIndex" logic out of the setCurrentItem logic, to make sure we always update mDefaultPageIndex.

This logic assumes that one of the panels in the panelConfig will indeed be the default, but I assume the HomeConfig/PanelConfig code that maintains this invariant. If my assumption is wrong, we should probably fix that, since the UI will end up in a busted state if there's no default panel (and with this patch the app would actually crash in setCurrentItem if that were the case).
Attachment #8387157 - Attachment is obsolete: true
Attachment #8387684 - Flags: review?(lucasr.at.mozilla)
Actually, here's a version that makes some of the comments in here clearer.
Attachment #8387684 - Attachment is obsolete: true
Attachment #8387684 - Flags: review?(lucasr.at.mozilla)
Attachment #8387687 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8387687 [details] [diff] [review]
(v2) Hide banner when there are no panels enabled

Review of attachment 8387687 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

::: mobile/android/base/home/HomePager.java
@@ +320,2 @@
>              for (int i = 0; i < count; i++) {
>                  final PanelConfig panelConfig = enabledPanels.get(i);

While at it, maybe simplify this to:

if (enabledPanels.get(i).isDefault()) {
     ....
}
Attachment #8387687 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/b1150afb0f41
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Comment on attachment 8387687 [details] [diff] [review]
(v2) Hide banner when there are no panels enabled

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 960359
User impact if declined: home banner will appear when there are no panels enabled
Testing completed (on m-c, etc.): landed on m-c 3/10
Risk to taking this patch (and alternatives if risky): low-risk, cleans up some logic to show/hide the banner 
String or IDL/UUID changes made by this patch: none
Attachment #8387687 - Flags: approval-mozilla-aurora?
Attachment #8387687 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: