The DoorHanger divider should default to hidden, not shown

RESOLVED FIXED in Firefox 25

Status

()

Firefox for Android
Theme and Visual Design
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Margaret, Assigned: ckitching)

Tracking

Trunk
Firefox 25
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Assignee: nobody → ckitching
(Assignee)

Comment 1

4 years ago
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.
Attachment #768027 - Flags: review?(margaret.leibovic)
(Reporter)

Comment 2

4 years ago
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.
Attachment #768027 - Flags: review?(margaret.leibovic) → review-
(Assignee)

Comment 3

4 years ago
Created attachment 768396 [details] [diff] [review]
Revised patch.
Attachment #768027 - Attachment is obsolete: true
Attachment #768396 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 4

4 years ago
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?
(Assignee)

Comment 5

4 years ago
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.
Attachment #768405 - Flags: review?(margaret.leibovic)
(Reporter)

Comment 6

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

Nice.
Attachment #768405 - Flags: review?(margaret.leibovic) → review+
(Reporter)

Updated

4 years ago
Attachment #768396 - Attachment is obsolete: true
Attachment #768396 - Flags: review?(margaret.leibovic)
(Reporter)

Comment 7

4 years ago
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 :).
(Assignee)

Comment 8

4 years ago
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?)
Attachment #768405 - Attachment is obsolete: true
Attachment #769834 - Flags: review+
Attachment #769834 - Flags: checkin?
(Assignee)

Comment 9

4 years ago
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...
Attachment #769834 - Attachment is obsolete: true
Attachment #769834 - Flags: checkin?
Attachment #769835 - Flags: review?(margaret.leibovic)
Attachment #769835 - Flags: checkin?
(Assignee)

Comment 10

4 years ago
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
Attachment #769835 - Attachment is obsolete: true
Attachment #769835 - Flags: review?(margaret.leibovic)
Attachment #769835 - Flags: checkin?
Attachment #769837 - Flags: checkin?
(Reporter)

Comment 11

4 years ago
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.
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Attachment #769837 - Flags: checkin?
https://hg.mozilla.org/integration/mozilla-inbound/rev/78a0757431df
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/78a0757431df
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
You need to log in before you can comment on or make changes to this bug.