Last Comment Bug 886472 - The DoorHanger divider should default to hidden, not shown
: The DoorHanger divider should default to hidden, not shown
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: Theme and Visual Design (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: Firefox 25
Assigned To: Chris Kitching [:ckitching]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-06-24 12:57 PDT by :Margaret Leibovic
Modified: 2013-07-02 12:40 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch causing DoorHanger dividers to default to hidden, the logic for showdividers() to hide divider of final hanger, and removing redundant call to hideDivider. (3.30 KB, patch)
2013-06-26 15:07 PDT, Chris Kitching [:ckitching]
margaret.leibovic: review-
Details | Diff | Splinter Review
Revised patch. (3.20 KB, patch)
2013-06-27 10:06 PDT, Chris Kitching [:ckitching]
no flags Details | Diff | Splinter Review
Composed, corrected patch (3.50 KB, patch)
2013-06-27 10:17 PDT, Chris Kitching [:ckitching]
margaret.leibovic: review+
Details | Diff | Splinter Review
Bug 886472 - Dividers default to invisible, redundant call to hideDivier removed, showDividers() now hides divider of final visible DoorHanger. r=margaret.leibovic (3.53 KB, patch)
2013-07-01 13:59 PDT, Chris Kitching [:ckitching]
chriskitching: review+
Details | Diff | Splinter Review
Bug 886472 - Dividers default to invisible, redundant call to hideDivier removed, showDividers() now hides divider of final visible DoorHanger. r=margaret.leibovic (3.49 KB, patch)
2013-07-01 14:01 PDT, Chris Kitching [:ckitching]
no flags Details | Diff | Splinter Review
Bug 886472 - Dividers default to invisible, redundant call to hideDivier removed, showDividers() now hides divider of final visible DoorHanger. r=margaret.leibovic (3.49 KB, patch)
2013-07-01 14:06 PDT, Chris Kitching [:ckitching]
no flags Details | Diff | Splinter Review

Description :Margaret Leibovic 2013-06-24 12:57:16 PDT
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] 2013-06-26 15:07:36 PDT
Created 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.

Ta-Da.
Comment 2 :Margaret Leibovic 2013-06-27 09:30:23 PDT
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] 2013-06-27 10:06:52 PDT
Created attachment 768396 [details] [diff] [review]
Revised patch.
Comment 4 Chris Kitching [:ckitching] 2013-06-27 10:09:40 PDT
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] 2013-06-27 10:17:00 PDT
Created attachment 768405 [details] [diff] [review]
Composed, corrected patch

In the sort of likely event that this is really what was wanted - here you go.
Comment 6 :Margaret Leibovic 2013-06-27 13:35:10 PDT
Comment on attachment 768405 [details] [diff] [review]
Composed, corrected patch

Nice.
Comment 7 :Margaret Leibovic 2013-06-27 13:37:19 PDT
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] 2013-07-01 13:59:27 PDT
Created attachment 769834 [details] [diff] [review]
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] 2013-07-01 14:01:49 PDT
Created attachment 769835 [details] [diff] [review]
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] 2013-07-01 14:06:40 PDT
Created attachment 769837 [details] [diff] [review]
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 2013-07-01 16:16:13 PDT
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] 2013-07-02 05:15:44 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/78a0757431df
Comment 13 Ryan VanderMeulen [:RyanVM] 2013-07-02 12:40:06 PDT
https://hg.mozilla.org/mozilla-central/rev/78a0757431df

Note You need to log in before you can comment on or make changes to this bug.