Closed Bug 900234 Opened 7 years ago Closed 6 years ago

Force reader to always be the left most icon in page actions

Categories

(Firefox for Android :: General, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 26

People

(Reporter: wesj, Assigned: shilpanbhagat)

Details

Attachments

(2 files, 5 obsolete files)

See description.

Reader mode is a good differentiator for us. We want to show it off.
Assignee: nobody → sbhagat
Insertion is required for forcing priority pageactions to always remain visible. This patch replaces LinkedHashMap with an ArrayList for the same.
Attachment #786413 - Flags: review?(wjohnston)
Forgot to clean up an integer. This should be it.
Attachment #786413 - Attachment is obsolete: true
Attachment #786413 - Flags: review?(wjohnston)
Attachment #786418 - Flags: review?(wjohnston)
With this patch, you can now set priority pageactions which will strive be visible over other pageactions. Currently reader mode is the only one and hence appears on the leftmost side of the page action layout.
Attachment #786512 - Flags: review?(wjohnston)
Comment on attachment 786418 [details] [diff] [review]
Part 1: Replacing LinkedHashMap with ArrayList to allow insertions

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

::: mobile/android/base/PageActionLayout.java
@@ +256,5 @@
>              mPageActionsMenu.inflate(0);
>              mPageActionsMenu.setOnMenuItemClickListener(new GeckoPopupMenu.OnMenuItemClickListener() {
>                  @Override
>                  public boolean onMenuItemClick(MenuItem item) {
> +                    mPageActionList.get(item.getItemId()).onClick();

Hmm.. does this cause issues if items are added/removed while the menu is showing?
Attachment #786418 - Flags: review?(wjohnston) → review+
Comment on attachment 786512 [details] [diff] [review]
Part 2: Force reader mode to be the left most pageaction

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

Looks pretty good to me. I just want to see the cleaned up insertAt algorithm.

::: mobile/android/base/PageActionLayout.java
@@ +121,5 @@
> +        // Add priority pageactions at the very end of the list.
> +        if (mPageActionList.size() == 0) {
> +            mPageActionList.add(pageAction);
> +        } else {
> +            for(int i =  mPageActionList.size() - 1; i >= 0; i--) {

This is all pretty confusing. Can we do something simpler?

int insertAt = mPageActionList.size() - 1;
while(insertAt > 0 && mPageActionList.get(insertAt).isImportant()) {
  insertAt--;
}
mPageActionList.add(insertAt, pageAction);

@@ +300,5 @@
>          private OnPageActionClickListeners mOnPageActionClickListeners;
>          private Drawable mDrawable;
>          private String mTitle;
>          private String mId;
> +        private boolean hasPriority;

Lets just call this priority... or important?

::: mobile/android/chrome/content/browser.js
@@ +1652,5 @@
>          type: "PageActions:Add",
>          id: id,
>          title: aOptions.title,
> +        icon: resolveGeckoURI(aOptions.icon),
> +        hasPriority: (aOptions.hasPriority != undefined) ? aOptions.hasPriority : false

I think we typically do "hasPriority" in aOptions ? aOptions.hasPriority : false
Attachment #786512 - Flags: review?(wjohnston)
Carry forward from last r+ for part 1.
Attachment #786418 - Attachment is obsolete: true
Attachment #789054 - Flags: review+
Made changes as mentioned
Attachment #786512 - Attachment is obsolete: true
Attachment #789111 - Flags: review?(wjohnston)
Comment on attachment 789111 [details] [diff] [review]
Part 2: Force reader mode to be the left most pageaction

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

r- because this throws and fails

::: mobile/android/base/PageActionLayout.java
@@ +118,5 @@
> +    public void addPageAction(final String id, final String title, final String imageData, final OnPageActionClickListeners mOnPageActionClickListeners, boolean mImportant) {
> +        final PageAction pageAction = new PageAction(id, title, null, mOnPageActionClickListeners, mImportant);
> +
> +        int insertAt = mPageActionList.size();
> +        if (mPageActionList.size() > 0) {

Do you need this if? Won't insertAt > 0 cover this already?

@@ +119,5 @@
> +        final PageAction pageAction = new PageAction(id, title, null, mOnPageActionClickListeners, mImportant);
> +
> +        int insertAt = mPageActionList.size();
> +        if (mPageActionList.size() > 0) {
> +            while (insertAt > 0 && mPageActionList.get(insertAt).isImportant()) {

mPageActionList.get(insertAt) will fail when insertAt = size.
Attachment #789111 - Flags: review?(wjohnston) → review-
With revised insertAt algorithm
Attachment #789111 - Attachment is obsolete: true
Attachment #790329 - Flags: review?(wjohnston)
Comment on attachment 790329 [details] [diff] [review]
Part 2: Force reader mode to be the left most pageaction

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

Nice and clean. Land and nom this for uplift too!

::: mobile/android/chrome/content/browser.js
@@ +1639,5 @@
>          type: "PageActions:Add",
>          id: id,
>          title: aOptions.title,
> +        icon: resolveGeckoURI(aOptions.icon),
> +        isImportant: "isImportant" in aOptions ? aOptions.isImportant : false

I'd probably rename this property "important" and drop the "is" part.
Attachment #790329 - Flags: review?(wjohnston) → review+
Carry forward r+ with mentioned changes
Attachment #790329 - Attachment is obsolete: true
Attachment #792492 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7b7b35e4350c
https://hg.mozilla.org/mozilla-central/rev/a550304775f1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
Nom this for uplift.
Flags: needinfo?(sbhagat)
Hmm... Too late to uplift. This has shipped :)
Flags: needinfo?(shilpanbhagat)
You need to log in before you can comment on or make changes to this bug.