Closed
Bug 923368
Opened 11 years ago
Closed 10 years ago
Home panel items should allow for copying URL
Categories
(Firefox for Android Graveyard :: Awesomescreen, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 32
People
(Reporter: rnewman, Assigned: marcus)
References
Details
(Whiteboard: [mentor=margaret][lang=java])
Attachments
(1 file, 2 obsolete files)
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•11 years ago
|
||
Let's loop in ibarlow here, but if this is something we want to do, it would make for a good mentor bug.
Whiteboard: [mentor=margaret][lang=java]
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
Updated•11 years ago
|
Flags: needinfo?(margaret.leibovic)
Comment 3•11 years ago
|
||
(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•11 years ago
|
||
(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.
Assignee: nobody → rahulb_2010
Flags: needinfo?(margaret.leibovic)
Comment 5•11 years ago
|
||
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.
Flags: needinfo?(ibarlow)
Comment 6•11 years ago
|
||
(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?
Flags: needinfo?(ibarlow)
Reporter | ||
Comment 7•11 years ago
|
||
(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.
Assignee | ||
Comment 8•10 years ago
|
||
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
Attachment #8424835 -
Flags: review?(margaret.leibovic)
Comment 9•10 years ago
|
||
(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.
Assignee: rahulb_2010 → marcus.zeng1224
Flags: needinfo?(ibarlow)
Comment 10•10 years ago
|
||
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.
Attachment #8424835 -
Flags: review?(margaret.leibovic) → feedback+
Assignee | ||
Comment 11•10 years ago
|
||
Comment on attachment 8424835 [details] [diff] [review] 923368.patch ># HG changeset patch ># Parent 41a54c8add09fe29b472fb4b7787b60a45dca1c9 ># User Marcus366<marcus.zeng1224@gmail.com> >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"/>
Assignee | ||
Comment 12•10 years ago
|
||
sorry for my last comment because of my misoperation. this patch is the same as previouse one but delete the trailing whitespace
Attachment #8424835 -
Attachment is obsolete: true
Attachment #8425237 -
Flags: review?(margaret.leibovic)
Comment 13•10 years ago
|
||
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...
Summary: History view should allow for copying URL → Home panel items should allow for copying URL
Comment 15•10 years ago
|
||
I chatted with ibarlow on IRC, and he's okay with this change, although still sad to see the context menu grow longer.
Flags: needinfo?(ibarlow)
Comment 16•10 years ago
|
||
Comment on attachment 8425237 [details] [diff] [review] 923368 rev2.patch Review of attachment 8425237 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks!
Attachment #8425237 -
Flags: review?(margaret.leibovic) → review+
Comment 17•10 years ago
|
||
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.
Assignee | ||
Comment 18•10 years ago
|
||
(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...
Assignee | ||
Comment 19•10 years ago
|
||
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.
Attachment #8425237 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 20•10 years ago
|
||
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 :)
Keywords: checkin-needed
Whiteboard: [mentor=margaret][lang=java] → [mentor=margaret][lang=java][fixed-in-fx-team]
Comment 21•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e6a0337da8d4
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=margaret][lang=java][fixed-in-fx-team] → [mentor=margaret][lang=java]
Target Milestone: --- → Firefox 32
Comment 22•10 years ago
|
||
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)
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•