923368 - Home panel items should allow for copying URL

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Richard Newman [:rnewman]]

We let you tap-and-hold to get a menu, and in that menu you can do things like Share. But you can't copy the URL. Let's do that!

Comment 1

[:Margaret Leibovic]

Let's loop in ibarlow here, but if this is something we want to do, it would make for a good mentor bug.

Comment 2

[Rahul]

Hello Everyone,
I am new to open source and only have an academic to programming. I am interested in picking this bug up and would like some help/guidance on how to get started. Thanks.
-Rahul

Comment 3

[Ian Barlow (:ibarlow)]

(In reply to :Margaret Leibovic from comment #1)
> Let's loop in ibarlow here, but if this is something we want to do, it would
> make for a good mentor bug.

This sounds ok to me.

Comment 4

[:Margaret Leibovic]

(In reply to rahulb_2010 from comment #2)
> Hello Everyone,
> I am new to open source and only have an academic to programming. I am
> interested in picking this bug up and would like some help/guidance on how
> to get started. Thanks.
> -Rahul

Hi Rahul! First of all, do you have a local build environment set up? You'll want to follow the directions listed here to get started:
https://wiki.mozilla.org/Mobile/Fennec/Android

After that, you'll want to get a sense of how our context menus work on about:home. This logic can be a bit tricky, but here are some places to start looking:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/menu/home_contextmenu.xml
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomeListView.java#128
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomeFragment.java#72

We already have a "Copy Address" string in the tree, so we can probably just use that for this new context menu item:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/locales/en-US/android_strings.dtd#224

You can see the implementation of the browse toolbar "Copy Address" context menu item for an example of how to implement this:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#693

Looking through this context menu code in BrowserApp/HomeFragment, it feels like we should consider factoring out some of this onContextItemSelected() logic into shared methods, but we can do that as follow-up work.

Comment 5

[Peter Retzer (:pretzer)]

Ian, how does this fit into your new context menu strategy proposed in bug 931021? It seems like a copy action is no longer desired...

If it is still desired, why limit it to history items only? All items (bookmarks, top sites, etc.) would benefit from a copy action, not just for the sake of consistency. In fact I proposed so in bug 777724 some time ago already.

Comment 6

[Ian Barlow (:ibarlow)]

(In reply to Peter Retzer (:pretzer) from comment #5)
> Ian, how does this fit into your new context menu strategy proposed in bug
> 931021? It seems like a copy action is no longer desired...

In the interest of streamlining and simplifying, you're right -- I am less a fan of adding this than I was previously. Note that we're also removing Share from these context menus, and making them more about just opening and editing content.

Sure, we *could* add Copy, but is it just because we can, or is there a use case I'm not considering here?

Comment 7

[Richard Newman [:rnewman]]

(In reply to Ian Barlow (:ibarlow) from comment #6)

> Sure, we *could* add Copy, but is it just because we can, or is there a use
> case I'm not considering here?

I filed the bug because I tried to do it :D

Trying to include a previously visited URL in an email, forum post, whatever. Seems cruel to make the user load the page and wait so they can copy it from the URL bar.

ISTM that wherever there's an entity that's a "page", the user should have the same basic toolbox available -- open it (if they're not already in that context), share it, save it (to reading list), pin it, remove it from this list, copy the URL.

That toolbox should apply wherever I encounter a page -- in the tab tray, top sites, bookmarks, history, or the forthcoming search results pane. As a user, I find it surprising to have a rich set of capabilities in one place, and a very sparse selection in another.

Comment 8

[Zhanpeng Zeng (:marcus)]

Attachment: 923368.patch

i add a copy address item in all homefragments.
it is my first contribution so i appreciate some advice on the patch or me.
thank you

Comment 9

[:Margaret Leibovic]

(In reply to marcus.zeng1224 from comment #8)
> Created attachment 8424835 [details] [diff] [review]
> 923368.patch
> 
> i add a copy address item in all homefragments.
> it is my first contribution so i appreciate some advice on the patch or me.
> thank you

Thsnks so much for the patch! I'd like to double check with ibarlow to make sure he's okay with us adding this new item, as well as ask him where he would prefer it goes in the order of context menu items.

Personally, I agree with rnewman that I have tried to copy URLs from the home page, and have been sad that there's no context menu item.

Comment 10

[:Margaret Leibovic]

Comment on attachment 8424835 [details] [diff] [review]
923368.patch

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

This looks great! Just giving feedback+ for now until we hear back from ibarlow.

::: mobile/android/base/home/HomeFragment.java
@@ +137,5 @@
> +            if (info.url == null) {
> +                Log.e(LOGTAG, "Can't copy address because URL is null");
> +                return false;
> +            }
> +            

Please remove this trailing whitespace.

Comment 11

[Zhanpeng Zeng (:marcus)]

Comment on attachment 8424835 [details] [diff] [review]
923368.patch

># HG changeset patch
># Parent 41a54c8add09fe29b472fb4b7787b60a45dca1c9
># User Marcus366<[e-mail]>
>Add "Copy Address" ContextMenuItem to all HomeFragments namely HISTORY, TOP SITES, BOOKMARKS and READING LIST
>
>
>diff --git a/mobile/android/base/home/HomeFragment.java b/mobile/android/base/home/HomeFragment.java
>--- a/mobile/android/base/home/HomeFragment.java
>+++ b/mobile/android/base/home/HomeFragment.java
>@@ -12,16 +12,17 @@ import org.mozilla.gecko.GeckoProfile;
> import org.mozilla.gecko.R;
> import org.mozilla.gecko.ReaderModeUtils;
> import org.mozilla.gecko.Tabs;
> import org.mozilla.gecko.Telemetry;
> import org.mozilla.gecko.TelemetryContract;
> import org.mozilla.gecko.db.BrowserContract.Combined;
> import org.mozilla.gecko.db.BrowserDB;
> import org.mozilla.gecko.favicons.Favicons;
>+import org.mozilla.gecko.util.Clipboard;
> import org.mozilla.gecko.util.ThreadUtils;
> import org.mozilla.gecko.util.UiAsyncTask;
> 
> import android.content.ContentResolver;
> import android.content.Context;
> import android.content.Intent;
> import android.content.res.Configuration;
> import android.net.Uri;
>@@ -127,16 +128,26 @@ abstract class HomeFragment extends Frag
>         final Context context = getActivity();
> 
>         final int itemId = item.getItemId();
> 
>         // Track the menu action. We don't know much about the context, but we can use this to determine
>         // the frequency of use for various actions.
>         Telemetry.sendUIEvent(TelemetryContract.Event.ACTION, TelemetryContract.Method.CONTEXT_MENU, getResources().getResourceEntryName(itemId));
> 
>+        if (itemId == R.id.home_copyurl) {
>+            if (info.url == null) {
>+                Log.e(LOGTAG, "Can't copy address because URL is null");
>+                return false;
>+            }
>+
>+            Clipboard.setText(info.url);
>+            return true;
>+        }
>+
>         if (itemId == R.id.home_share) {
>             if (info.url == null) {
>                 Log.e(LOGTAG, "Can't share because URL is null");
>                 return false;
>             } else {
>                 GeckoAppShell.openUriExternal(info.url, SHARE_MIME_TYPE, "", "",
>                                               Intent.ACTION_SEND, info.getDisplayTitle());
> 
>diff --git a/mobile/android/base/resources/menu/home_contextmenu.xml b/mobile/android/base/resources/menu/home_contextmenu.xml
>--- a/mobile/android/base/resources/menu/home_contextmenu.xml
>+++ b/mobile/android/base/resources/menu/home_contextmenu.xml
>@@ -9,16 +9,19 @@
>           android:title="@string/contextmenu_open_new_tab"/>
> 
>     <item android:id="@+id/home_open_private_tab"
>           android:title="@string/contextmenu_open_private_tab"/>
> 
>     <item android:id="@+id/home_open_in_reader"
>           android:title="@string/contextmenu_open_in_reader"/>
> 
>+    <item android:id="@+id/home_copyurl"
>+          android:title="@string/contextmenu_copyurl"/>
>+
>     <item android:id="@+id/home_share"
>           android:title="@string/contextmenu_share"/>
> 
>     <item android:id="@+id/top_sites_edit"
>           android:title="@string/contextmenu_top_sites_edit"/>
> 
>     <item android:id="@+id/top_sites_pin"
>           android:title="@string/contextmenu_top_sites_pin"/>

Comment 12

[Zhanpeng Zeng (:marcus)]

Attachment: 923368 rev2.patch

sorry for my last comment because of my misoperation.
this patch is the same as previouse one but delete the trailing whitespace

Comment 13

[Peter Retzer (:pretzer)]

AFAICS the current patch adds the action to all home panel items, not just to the history panel, correct? Updating the summary to reflect that and duping my older bug 777724 here...

Comment 15

[:Margaret Leibovic]

I chatted with ibarlow on IRC, and he's okay with this change, although still sad to see the context menu grow longer.

Comment 16

[:Margaret Leibovic]

Comment on attachment 8425237 [details] [diff] [review]
923368 rev2.patch

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

Looks good, thanks!

Comment 17

[:Margaret Leibovic]

Here's a try push:
https://tbpl.mozilla.org/?tree=Try&rev=85d3788c3b82

If this is all green, you can upload a patch with an updated commit message (to include bug number and reviewer), and set the "checkin-needed" keyword in the keyword field up above for someone to come check in this patch for you.

Comment 18

[Zhanpeng Zeng (:marcus)]

(In reply to :Margaret Leibovic from comment #17)
> Here's a try push:
> https://tbpl.mozilla.org/?tree=Try&rev=85d3788c3b82

It is pity that it can't pass the Android 2.2 Armv6 Opt rc1 test cases.

It seem that it is related to the bug "application timed out after 330 seconds with no output" about the test framework.I have asked for gcp to help me retrigger the test. I hope that it is not my fault...

Comment 19

[Zhanpeng Zeng (:marcus)]

Attachment: Bug 923368 - Home panel items should allow for copying URL; r=margaret.patch

Luckily, it turn out that previous failing is just a intermittent bug on try server.Now I submit a properly formatted patch and add the keywork checkin-needed.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/e6a0337da8d4

Thanks for the patch! I pushed it with a minor tweak to your name in the commit information so that it shows up better in the commit history. If you could please make the same adjustment locally, it would be appreciated :)

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e6a0337da8d4

Comment 22

[Teodora Vermesan (:TeoVermesan)]

Copy Address is present on the context menu on Home Panels (History, Top Sites, Bookmarks, Reading List, Wikipedia Panel etc.) and works as expected.
Verified fixed on:
Device: LG Nexus 4 (Android 4.4.2)
Build: FIrefox for Android 32.0a1 (2014-05-25)