722591 - Longtap on page title (not awesomescreen) should show an editbox context menu

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Wesley Johnston (:wesj)]

I've been pasting a url in a lot lately because I constantly restart. Pasting the url requires tapping urlbar, waiting for the awesomescreen to show, and then long tapping and pasting into the awesomescreen textbox. We should show the same url (and maybe also open the awesomescreen?) when you long tap on the page title.

Comment 1

[Madhava Enros [:madhava]]

What about the long tap menu on the title including "paste and go" -- maybe "copy URL" and "copy title"?

Just thinking out loud here...

Comment 2

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

As part of my 30min/day of paper cut fixes, a WIP to make this happen. Works. Needs string and id cleanup, and a test of course.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch v2

I needed a little break, so this adds "Paste & Go", "Paste", "Share", "Copy Location", "Copy Title", and "Add to Home Screen" to the titlebar context menu. I always feel like this context menu code is a mess, so hoping margaret can tell me the right, clean way to do it.

Now back to real work...

Comment 5

[Wesley Johnston (:wesj)]

In the name of keeping things simple, I think we should probably get rid of "Copy Title" which isn't ever very useful.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Patch v2.5

Updated with the missing file.

Comment 7

[:Margaret Leibovic]

Comment on attachment 628425 [details] [diff] [review]
Patch v2.5

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

Holding off review until there's a new (un-bitrotted) version of the patch. Here are some comments to start you off, though :) I promise to get back to you sooner next time around!

::: mobile/android/base/BrowserToolbar.java
@@ +119,5 @@
> +                Tab tab = Tabs.getInstance().getSelectedTab();
> +                if (tab != null) {
> +                    String url = tab.getURL();
> +                    if (url == null) {
> +                        menu.findItem(R.id.copyurl).setVisible(false);

We probably also want to hide any tab-dependant menu items when tab is null (like share, etc.).

::: mobile/android/base/GeckoApp.java
@@ +507,5 @@
>      }
>  
> +    private boolean shareCurrentUrl() {
> +      Tab tab = Tabs.getInstance().getSelectedTab();
> +      if (tab != null) {

Nit: I prefer just returning if tab is null, to avoid indenting the rest of the body of the method.

@@ +515,5 @@
> +
> +          GeckoAppShell.openUriExternal(url, "text/plain", "", "",
> +                                        Intent.ACTION_SEND, tab.getTitle());
> +      }
> +      return true;

I know you just copied this, but why are we returning true here even if the tab is null, considering we return false if the url is null? Reading the docs, it looks like we always just want to return true, since that's just used to indicate we're handing the menu item:
http://developer.android.com/reference/android/app/Activity.html#onOptionsItemSelected%28android.view.MenuItem%29

I propose we make shareCurrentUrl just return void, then stick the return true back below it in onOptionsItemSelected.

@@ +2898,5 @@
>      public boolean linkerExtract() {
>          return false;
>      }
> +
> +    public boolean onContextItemSelected(MenuItem item) {

I think you want an @Override above this.

@@ +2914,5 @@
> +                if (text != null && !TextUtils.isEmpty(text)) {
> +                    showAwesomebar(AwesomeBar.Type.ADD, text);
> +                    return true;
> +                }
> +                break;

I don't like that there's a mix of breaks and returns in here. I think we should replace the breaks with returns.

@@ +2917,5 @@
> +                }
> +                break;
> +            }
> +            case R.id.share: {
> +                return shareCurrentUrl();

This would need to change if you change shareCurrentUrl like I suggested above.

@@ +2944,5 @@
> +                }
> +                return true;
> +            }
> +        }
> +        return false;

My same questions about returning true/false apply to this method:
http://developer.android.com/reference/android/app/Activity.html#onContextItemSelected%28android.view.MenuItem%29

It seems to me like we should always be returning true if the user taps on one of the menuitems that we created. I'm not sure what returning false would mean.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Unbitrotted Patch

Unbitrotted and feedback adjusted. Thanks!

Comment 9

[:Margaret Leibovic]

Comment on attachment 630309 [details] [diff] [review]
Unbitrotted Patch

I'm giving this an r- mainly because I think we should get UX feedback on what Paste and Paste&Go should do w.r.t. new/current tabs.

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
>@@ -652,16 +644,30 @@ abstract public class GeckoApp
 
>+    private void shareCurrentUrl() {
>+      Tab tab = Tabs.getInstance().getSelectedTab();
>+      if (tab == null)
>+        return;
>+
>+      String url = tab.getURL();
>+      if (url == null)
>+          return;
>+
>+      GeckoAppShell.openUriExternal(url, "text/plain", "", "",
>+                                    Intent.ACTION_SEND, tab.getTitle());
>+      return;

You don't need this return at the end of the method :)

>@@ -2759,29 +2765,35 @@ abstract public class GeckoApp

>     public boolean showAwesomebar(AwesomeBar.Type aType) {
>+      return showAwesomebar(aType, null);
>+    }
>+
>+    public boolean showAwesomebar(AwesomeBar.Type aType, String aUrl) {
>         Intent intent = new Intent(getBaseContext(), AwesomeBar.class);
>         intent.addFlags(Intent.FLAG_ACTIVITY_NO_ANIMATION | Intent.FLAG_ACTIVITY_NO_HISTORY);
>         intent.putExtra(AwesomeBar.TYPE_KEY, aType.name());
> 
>-        if (aType != AwesomeBar.Type.ADD) {
>+        if (aUrl != null || aType != AwesomeBar.Type.ADD) {

Can we change the second part of this to |if aType == AwesomeBar.Type.EDIT|? That's the only other case, and it makes it clearer what's going on here.

>             // if we're not adding a new tab, show the old url
>-            Tab tab = Tabs.getInstance().getSelectedTab();
>-            if (tab != null) {
>-                String url = tab.getURL();
>-                if (url != null) {
>-                    intent.putExtra(AwesomeBar.CURRENT_URL_KEY, url);
>+            if (aUrl == null) {
>+                Tab tab = Tabs.getInstance().getSelectedTab();
>+                if (tab != null) {
>+                    aUrl = tab.getURL();
>                 }
>             }
>+            if (aUrl != null) {
>+                intent.putExtra(AwesomeBar.CURRENT_URL_KEY, aUrl);
>+            }
>         }

This logic feels too messy, especially since your new use case only needs to handle a call with Type.ADD. Maybe we should only do something with the extra aUrl param in the ADD case? I just think we need to be less clever here, and tease apart the different cases (and some comments explaining the logic would be good). I also think we should get some UX feedback here on whether or not the result of "Paste" would be expected to open a new tab or in the current tab, especially since Paste&Go as you have it overrides the current tab.

>@@ -3078,9 +3090,56 @@ abstract public class GeckoApp

>+    @Override
>+    public boolean onContextItemSelected(MenuItem item) {
>+        switch(item.getItemId()) {
...
>+        }
>+        return true;

I think the correct thing to do is to return false at the end of this method in case there's somehow a context menu item that we didn't put there, but return true inside each of the case statements when we know for sure we handled the menu item.

Comment 10

[:Margaret Leibovic]

Ian, can you help us out here? Do you think Paste/Paste&Go should just modify the url of the currently selected tab? Because that's what I think :)

(The other option is for one or both of them to open the url in a new tab)

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Patch

I'll wear you down eventually!

I agree, these should open in the same tab. Not sure why I did that the way I originally did. Also cleaned up the showAwesomebar logic a bit.

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Cleanup Types

This converts AwesomeBar.Type to AwesomeBar.Target with values NEW_TAB and CURRENT_TAB. Helps me not make mistakes.

Comment 13

[Ian Barlow (:ibarlow)]

Margaret and Wes, I also agree that the pasted URL should affect the current tab, so it's unanimous!

Comment 14

[:Margaret Leibovic]

Comment on attachment 630379 [details] [diff] [review]
Patch

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

Looking good! Just one last comment I found...

::: mobile/android/base/BrowserToolbar.java
@@ +119,5 @@
> +                Tab tab = Tabs.getInstance().getSelectedTab();
> +                if (tab != null) {
> +                    String url = tab.getURL();
> +                    if (url == null) {
> +                        menu.findItem(R.id.copyurl).setVisible(false);

I just realized that share and add to home screen won't work out the url is null, so we can also hide those here.

Comment 15

[:Margaret Leibovic]

Comment on attachment 630380 [details] [diff] [review]
Cleanup Types

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

::: mobile/android/base/AwesomeBar.java
@@ +63,2 @@
>  
>      private String mType;

I think we should also rename mType to mTarget. Kill "type"!

Comment 16

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8dc147df2d0f
https://hg.mozilla.org/integration/mozilla-inbound/rev/78394bf55c75

Comment 17

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/78394bf55c75
https://hg.mozilla.org/mozilla-central/rev/8dc147df2d0f

(Merged by Ed Morley)

Comment 18

[Aaron Train [:aaronmt]]

This introduced a NPE crash in bug 763167.

Comment 19

[Wesley Johnston (:wesj)]

Attachment: Patch

Uhmm... I ignored your comment... :(

Comment 20

[Wesley Johnston (:wesj)]

Attachment: Uploaded Patch

Oh wait, I did make this change on checkin. Here's the patch that went. I assume the r+ was from irc.... I hope. I'm looking at moving this to Aurora because I have another aurora+ patch that depends on it. I'm gonna unbit that around this though. I'll nom it anyway to protect other patches that might depend on this.

Comment 21

[Wesley Johnston (:wesj)]

Comment on attachment 637297 [details] [diff] [review]
Uploaded Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Initial Fennec work had this.
User impact if declined: None. This is purely to make development a little easier by using better variable names. The point is to save us some potential conflicts with other patches going to Aurora.
Testing completed (on m-c, etc.): Several months ago.
Risk to taking this patch (and alternatives if risky): Very low risk. No changes.
String or UUID changes made by this patch: None.

Comment 22

[Paul Feher]

Verified fixed on:
Htc Desire Z (2.3.3)
Using:
Nightly Fennec 16.0a1 (2012-07-09)

The patch has not been added to aurora yet, although it was approved.
Waiting for the patch to land in Aurora.

Comment 23

[Wesley Johnston (:wesj)]

Whoops. Thanks:

http://hg.mozilla.org/releases/mozilla-aurora/rev/4d9dc42a89d8
http://hg.mozilla.org/releases/mozilla-aurora/rev/911a0c5b1ce0