886472 - The DoorHanger divider should default to hidden, not shown

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[:Margaret Leibovic]

We only show the divider on doorhangers if there are multiple ones inside a doorhanger popup, and we do this by manually calling showDivider/hideDivider in DoorHangerPopup:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/DoorHangerPopup.java#298

However, now that DoorHanger views can be used outside of DoorHangerPopup, it makes more sense for it to default to hidden, so that we don't need to manually hide it after creating a DoorHanger.

Also, the showDividers() logic in DoorHangerPopup isn't quite correct, since we only want to make sure we hide the divider for the last *visible* DoorHanger, not just the last one. You can see this bug if you trigger a notification, open a different tab that triggers a different notification, then switch back to the first tab.

Comment 1

[Chris Kitching [:ckitching]]

Attachment: Patch causing DoorHanger dividers to default to hidden, the logic for showdividers() to hide divider of final hanger, and removing redundant call to hideDivider.

Ta-Da.

Comment 2

[:Margaret Leibovic]

Comment on attachment 768027 [details] [diff] [review]
Patch causing DoorHanger dividers to default to hidden, the logic for showdividers() to hide divider of final hanger, and removing redundant call to hideDivider.

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

Looking good! I'm just giving an r- because I would like to see setting the visibility of the divider moved to the XML. When you have a new version of the patch, you can upload it and flag me for review again :)

::: mobile/android/base/DoorHanger.java
@@ +93,5 @@
>          LayoutInflater.from(context).inflate(R.layout.doorhanger, this);
>          mTextView = (TextView) findViewById(R.id.doorhanger_title);
>          mChoicesLayout = (LinearLayout) findViewById(R.id.doorhanger_choices);
>          mDivider = findViewById(R.id.divider_doorhanger);
> +        mDivider.setVisibility(View.GONE);

You should do this with an attribute in doorhanger.xml. In general, we prefer to set things in the XML if possible because it's faster.

::: mobile/android/base/DoorHangerPopup.java
@@ +294,5 @@
>              setFocusable(true);
>          }
>      }
>  
>      private void showDividers() {

If you like, you could add some documentation above this method to say what it's doing. One sentence would suffice :)

@@ +296,5 @@
>      }
>  
>      private void showDividers() {
>          int count = mContent.getChildCount();
> +        DoorHanger lastVisibleHanger = null;

Nit: Call this lastVisibleDoorHanger.

@@ +301,5 @@
>  
>          for (int i = 0; i < count; i++) {
>              DoorHanger dh = (DoorHanger) mContent.getChildAt(i);
>              dh.showDivider();
> +            if(dh.getVisibility() == View.VISIBLE) {

Nit: Put a space between the if and the open paren (we generally try not to be super picky about style, but this would be consistent with the rest of the file).

@@ +308,2 @@
>          }
> +        if(lastVisibleHanger != null) {

Same here.

Comment 3

[Chris Kitching [:ckitching]]

Attachment: Revised patch.

Comment 4

[Chris Kitching [:ckitching]]

Ah. It occurs to me that this is a patch for the patch. What is the proper procedure here? Should I have created a patch that is the composition of the two I have submitted, or is what I just did correct?

Comment 5

[Chris Kitching [:ckitching]]

Attachment: Composed, corrected patch

In the sort of likely event that this is really what was wanted - here you go.

Comment 6

[:Margaret Leibovic]

Comment on attachment 768405 [details] [diff] [review]
Composed, corrected patch

Nice.

Comment 7

[:Margaret Leibovic]

You should upload a new patch with a final commit message (with bug number and r=margaret), and then add the "checkin-needed" keyword for someone to come along and land it (it makes their job easier if the final commit message is already in the patch :).

Comment 8

[Chris Kitching [:ckitching]]

Attachment: Bug 886472 - Dividers default to invisible, redundant call to hideDivier removed, showDividers() now hides divider of final visible DoorHanger. r=margaret.leibovic

Okay - that should do it. (Did I miss a flag?)

Comment 9

[Chris Kitching [:ckitching]]

Attachment: Bug 886472 - Dividers default to invisible, redundant call to hideDivier removed, showDividers() now hides divider of final visible DoorHanger. r=margaret.leibovic

Actually correcting the commit message as opposed to breaking the patch...

Comment 10

[Chris Kitching [:ckitching]]

Attachment: Bug 886472 - Dividers default to invisible, redundant call to hideDivier removed, showDividers() now hides divider of final visible DoorHanger. r=margaret.leibovic

Comment 11

[:Margaret Leibovic]

Oops, I think I wasn't totally clear in my directions... you need to set the "checkin-needed" keyword on the bug, that's the thing that people who land patches will be searching for. The checkin? flag on the patch isn't necessary, I think it's usually just used when people have multiple patches in a bug and they want to be clear about the landing state of all of them.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78a0757431df

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/78a0757431df