Add "Share" and "Add to Home Screen" to pinned sites context menus

RESOLVED FIXED in Firefox 29

Status

()

RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: u421692, Assigned: oogunsakin)

Tracking

(Blocks: 1 bug)

29 Branch
Firefox 29
All
Android
Points:
---
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(firefox26 affected, firefox27 affected, firefox28 affected, firefox29 verified, fennec+)

Details

(Whiteboard: [mentor=margaret][lang=java])

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

5 years ago
Since "Share" and "Add to Home Screen" are present for every list entry in about:home(top sites, bookmarks, reading list, history), "Share" and "Add to Home Screen" should also be added to pinned sites context menus.

Updated

5 years ago
tracking-fennec: --- → ?
Ian - Do we want to add one or both of these? "Add to Home Screen" might be useful. "Share" might be less needed.
Flags: needinfo?(ibarlow)
Yeah, and I would suggest adding both for consistency's sake. I don't feel too strongly about Share, but it seems kind of arbitrary to not have it for thumbnails if you have it elsewhere.
Flags: needinfo?(ibarlow)
tracking-fennec: ? → +
Whiteboard: [mentor=margaret][lang=java]
Assignee: nobody → jdover
Assignee: jdover → Olusola
(Assignee)

Comment 3

5 years ago
Created attachment 8356380 [details] [diff] [review]
x.patch

Added "Share" and "Add to Home Screen" to pinned sites context menus.

Test:
- Was able to "Share" and "Add to Home Screen" on pinned and unpinned items
Attachment #8356380 - Flags: review?(liuche)
We may want a Try server run here, to check that adding the menu items does not break an existing test. If we have not tests for the presence of the menu items, maybe we should add one.

Updated

5 years ago
Status: NEW → ASSIGNED
Comment on attachment 8356380 [details] [diff] [review]
x.patch

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

This is a good start! However, I started noticing that there is a lot of duplicated code between the HomeFragment contextmenu and the top sites contextmenu. Let's combine them, and use visibility to display the relevant menu items.

::: mobile/android/base/home/TopSitesPage.java
@@ +22,5 @@
>  import org.mozilla.gecko.home.PinSiteDialog.OnSiteSelectedListener;
>  import org.mozilla.gecko.home.TopSitesGridView.OnEditPinnedSiteListener;
>  import org.mozilla.gecko.home.TopSitesGridView.TopSitesGridContextMenuInfo;
>  import org.mozilla.gecko.util.ThreadUtils;
> +import org.mozilla.gecko.GeckoAppShell;

Nit: move this up to alphabetize imports

@@ +361,5 @@
>          }
>  
> +        if (itemId == R.id.home_share) {
> +            if (info.url == null) {
> +                Log.e(LOGTAG, "Can't share because URL is null");

Nit: let's make this message a little clearer out of context, maybe something like "Share not enabled for context menu because URL is null." No need to make it an Error level log, warn should be sufficient.

I noticed that there is actually a lot of duplicated code now, between HomeFragment and TopSites - it looks like the context menu resources are almost exactly the same now, so why don't we actually clean those up and unify them?

@@ +370,5 @@
> +        }
> +
> +        if (itemId == R.id.home_add_to_launcher) {
> +            if (info.url == null) {
> +                Log.e(LOGTAG, "Can't add to home screen because URL is null");

Same as above comment for the message.

::: mobile/android/base/resources/menu/top_sites_contextmenu.xml
@@ +17,5 @@
>      <item android:id="@+id/top_sites_pin"
>            android:title="@string/contextmenu_top_sites_pin"/>
>  
>      <item android:id="@+id/top_sites_unpin"
>            android:title="@string/contextmenu_top_sites_unpin"/>

Nit: newline between items.

@@ +20,5 @@
>      <item android:id="@+id/top_sites_unpin"
>            android:title="@string/contextmenu_top_sites_unpin"/>
> +    <item android:id="@+id/home_share"
> +          android:title="@string/contextmenu_share"/>
> +          

Nit: no trailing whitespace.
Attachment #8356380 - Flags: review?(liuche) → review-
(Assignee)

Updated

5 years ago
Assignee: Olusola → oogunsakin
Specifically:

We can combine resources/menu/home_contextmenu.xml and resources/menu/top_sites_contextmenu.xml.

Then take a look at how we hide/show specific contextmenu items, like [1].

Basically, we should at least combine HomeFragment.onCreateContextMenu and TopSitesPage.onCreateContextMenu, and consider combining onContextItemSelected (by calling through super.onContextItemSelected).

[1] http://hg.mozilla.org/mozilla-central/file/ce3dd14c1840/mobile/android/base/home/HomeFragment.java#l90
Actually, going through the code some more, doing a refactor will be a little more complicated and is kind of out of the scope of this "good-first-bug," so let's eat the code duplication a little longer and think about whether the refactor is worth it.
(Assignee)

Comment 9

5 years ago
Created attachment 8356924 [details] [diff] [review]
bug-950610-fix-v2.patch
Attachment #8356380 - Attachment is obsolete: true
Attachment #8356924 - Flags: review?(liuche)
(Assignee)

Comment 10

5 years ago
Created attachment 8356926 [details] [diff] [review]
bug-950610-fix-v2.patch
Attachment #8356924 - Attachment is obsolete: true
Attachment #8356924 - Flags: review?(liuche)
Attachment #8356926 - Flags: review?(liuche)
Comment on attachment 8356926 [details] [diff] [review]
bug-950610-fix-v2.patch

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

This looks good, except it seems to be a separate patch on top of your original patch. You'll want to address the comments and then combine the two patches. You can use hg qfold to do this:
Leave only the first patch applied (pop any other ones). Then 'hg qfold <second-patchname>' to fold the second patch into the first. Then upload the combined patches.

Almost there!

::: mobile/android/base/resources/menu/top_sites_contextmenu.xml
@@ +18,5 @@
>            android:title="@string/contextmenu_top_sites_pin"/>
>  
>      <item android:id="@+id/top_sites_unpin"
>            android:title="@string/contextmenu_top_sites_unpin"/>
> +          

Nit: it looks like your editor might be adding in spaces automatically - there are still leading/trailing spaces that should be removed.

@@ +23,3 @@
>      <item android:id="@+id/home_share"
>            android:title="@string/contextmenu_share"/>
>            

Same: extra spaces.
Attachment #8356926 - Flags: review?(liuche) → review-
(Assignee)

Comment 12

5 years ago
Created attachment 8357202 [details] [diff] [review]
bug-950610-fix3.patch
Attachment #8356926 - Attachment is obsolete: true
Attachment #8357202 - Flags: review?(liuche)
Comment on attachment 8357202 [details] [diff] [review]
bug-950610-fix3.patch

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

Two last little things, and then this is good to go!

Also, update the patch comment to terminate with a . instead of ;, and add r=liuche to the end. (Bug 950610 - Add "Share" and "Add to Home Screen" to pinned sites context menus. r=liuche)

When you're done making the changes, add checkin-needed to the whiteboard.

::: mobile/android/base/home/TopSitesPage.java
@@ +28,5 @@
>  import android.app.Activity;
>  import android.content.ContentResolver;
>  import android.content.Context;
>  import android.content.res.Configuration;
> +import android.content.Intent;

Tiny nit: alphabetize this after android.content.Context

::: mobile/android/base/resources/menu/top_sites_contextmenu.xml
@@ +19,5 @@
>  
>      <item android:id="@+id/top_sites_unpin"
>            android:title="@string/contextmenu_top_sites_unpin"/>
>  
> +    <item android:id="@+id/home_share"

Move this to be above Edit, so it's consistent with the other menus.
Attachment #8357202 - Flags: review?(liuche) → review+
(Assignee)

Comment 14

5 years ago
Created attachment 8357302 [details] [diff] [review]
bug-950610-fix4.patch
Attachment #8357202 - Attachment is obsolete: true
Attachment #8357302 - Flags: review?(liuche)
(Assignee)

Updated

5 years ago
Attachment #8357302 - Flags: checkin?(liuche)
Comment on attachment 8357302 [details] [diff] [review]
bug-950610-fix4.patch

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

One last whitespace error. Also, tiny tiny nit, but a space between the . and r= in the comment.

Sorry to be gating an so many small insignificant-sounding things; in general, large projects benefit from consistent code style for maintenance and readability reasons, because the code might be around for a long time and be touched by many people.

::: mobile/android/base/resources/menu/top_sites_contextmenu.xml
@@ +12,5 @@
>            android:title="@string/contextmenu_open_private_tab"/>
>  
> +    <item android:id="@+id/home_share"
> +          android:title="@string/contextmenu_share"/>
> +        

Whoops, looks like you introduced another trailing whitespace.
Attachment #8357302 - Flags: checkin?(liuche)

Comment 16

5 years ago
Comment on attachment 8357302 [details] [diff] [review]
bug-950610-fix4.patch

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

::: mobile/android/base/home/TopSitesPage.java
@@ +365,5 @@
> +                Log.w(LOGTAG, "Share not enabled for context menu because URL is null.");
> +            } else {
> +                GeckoAppShell.openUriExternal(info.url, SHARE_MIME_TYPE, "", "",
> +                                              Intent.ACTION_SEND, info.getDisplayTitle());
> +            }

Drive-by: Shouldn't you return true in here?
Comment on attachment 8357302 [details] [diff] [review]
bug-950610-fix4.patch

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

::: mobile/android/base/home/TopSitesPage.java
@@ +365,5 @@
> +                Log.w(LOGTAG, "Share not enabled for context menu because URL is null.");
> +            } else {
> +                GeckoAppShell.openUriExternal(info.url, SHARE_MIME_TYPE, "", "",
> +                                              Intent.ACTION_SEND, info.getDisplayTitle());
> +            }

Huh, that's true. Sola, can you return true here, and also add it to the share code in HomeFragment? It should return true there as well.
(Assignee)

Comment 18

5 years ago
Created attachment 8357442 [details] [diff] [review]
bug-950610-fix5.patch
Attachment #8357302 - Attachment is obsolete: true
Attachment #8357442 - Flags: review?(margaret.leibovic)
Attachment #8357442 - Flags: review?(liuche)
Attachment #8357442 - Flags: checkin?(liuche)
Comment on attachment 8357442 [details] [diff] [review]
bug-950610-fix5.patch

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

Just small changes, then r+! (Do be aware of whitespace in the future though.)

::: mobile/android/base/home/TopSitesPage.java
@@ +372,5 @@
> +        }
> +
> +        if (itemId == R.id.home_add_to_launcher) {
> +            if (info.url == null) {
> +                Log.w(LOGTAG, "Cannot add to home page for context menu because URL is null.");

Nit: this comment is a little confusing, maybe something like "Not enabling 'Add to home page' because URL is null."

::: mobile/android/base/resources/menu/top_sites_contextmenu.xml
@@ +12,5 @@
>            android:title="@string/contextmenu_open_private_tab"/>
>  
> +    <item android:id="@+id/home_share"
> +          android:title="@string/contextmenu_share"/>
> +        

Extra whitespace.
Attachment #8357442 - Flags: review?(liuche)
Attachment #8357442 - Flags: review+
Attachment #8357442 - Flags: checkin?(liuche)
(Assignee)

Comment 20

5 years ago
Created attachment 8357504 [details] [diff] [review]
bug-950610-fix6.patch
Attachment #8357442 - Attachment is obsolete: true
Attachment #8357442 - Flags: review?(margaret.leibovic)
Attachment #8357504 - Flags: review?(margaret.leibovic)
Attachment #8357504 - Flags: review?(liuche)
Attachment #8357504 - Flags: checkin?(liuche)
Comment on attachment 8357504 [details] [diff] [review]
bug-950610-fix6.patch

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

Looks good!

Now just add 'checkin-needed' into the whiteboard field of the bug, and a sheriff (or someone else) will land it. In general, don't set checkin flags or whiteboard until the patch is completely ready to land, because otherwise it might actually get checked in!
Attachment #8357504 - Flags: review?(margaret.leibovic)
Attachment #8357504 - Flags: review?(liuche)
Attachment #8357504 - Flags: review+
Attachment #8357504 - Flags: checkin?(liuche)
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/ad1fec1c3678
Keywords: checkin-needed
Whiteboard: [mentor=margaret][lang=java] → [mentor=margaret][lang=java][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/ad1fec1c3678
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=margaret][lang=java][fixed-in-fx-team] → [mentor=margaret][lang=java]
Target Milestone: --- → Firefox 29

Updated

5 years ago
Flags: in-moztrap?(fennec)

Comment 24

5 years ago
Build: Firefox Nightly 29.0a1 2014-01-10
Device: Galaxy Nexus 
OS: Android 4.3 

The two options are present for the pinned top sites. Setting the according flag to verified. 
Will this be uplifted to Aurora/Beta?
status-firefox29: affected → verified

Comment 25

5 years ago
TC updated in MozTrap for Mobile Fx 29 and Mobile Fx 29 tablet versions: https://moztrap.mozilla.org/manage/case/6030/
Flags: in-moztrap?(fennec) → in-moztrap+
You need to log in before you can comment on or make changes to this bug.