Closed Bug 722591 Opened 12 years ago Closed 12 years ago

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

Categories

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

defect

Tracking

(firefox15 verified, firefox16 verified, firefox17 verified, fennec+)

VERIFIED FIXED
Firefox 16
Tracking Status
firefox15 --- verified
firefox16 --- verified
firefox17 --- verified
fennec + ---

People

(Reporter: wesj, Assigned: wesj)

References

Details

Attachments

(2 files, 6 obsolete files)

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.
What about the long tap menu on the title including "paste and go" -- maybe "copy URL" and "copy title"?

Just thinking out loud here...
Assignee: nobody → wjohnston
tracking-fennec: --- → +
Priority: -- → P3
Attached patch WIP Patch (obsolete) — Splinter Review
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.
Attached patch Patch v2 (obsolete) — Splinter Review
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...
Attachment #601138 - Attachment is obsolete: true
Attachment #627347 - Flags: review?(margaret.leibovic)
In the name of keeping things simple, I think we should probably get rid of "Copy Title" which isn't ever very useful.
Attached patch Patch v2.5 (obsolete) — Splinter Review
Updated with the missing file.
Attachment #627347 - Attachment is obsolete: true
Attachment #627347 - Flags: review?(margaret.leibovic)
Attachment #628425 - Flags: review?(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.
Attachment #628425 - Flags: review?(margaret.leibovic) → feedback+
Attached patch Unbitrotted Patch (obsolete) — Splinter Review
Unbitrotted and feedback adjusted. Thanks!
Attachment #628425 - Attachment is obsolete: true
Attachment #630309 - Flags: review?(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.
Attachment #630309 - Flags: review?(margaret.leibovic) → review-
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)
Keywords: uiwanted
Attached patch PatchSplinter Review
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.
Attachment #630379 - Flags: review?(margaret.leibovic)
Attached patch Cleanup Types (obsolete) — Splinter Review
This converts AwesomeBar.Type to AwesomeBar.Target with values NEW_TAB and CURRENT_TAB. Helps me not make mistakes.
Attachment #630309 - Attachment is obsolete: true
Attachment #630380 - Flags: review?(margaret.leibovic)
Margaret and Wes, I also agree that the pasted URL should affect the current tab, so it's unanimous!
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.
Attachment #630379 - Flags: review?(margaret.leibovic) → review+
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"!
Attachment #630380 - Flags: review?(margaret.leibovic) → review-
Keywords: uiwanted
https://hg.mozilla.org/mozilla-central/rev/78394bf55c75
https://hg.mozilla.org/mozilla-central/rev/8dc147df2d0f

(Merged by Ed Morley)
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Depends on: 763165
Depends on: 763167
This introduced a NPE crash in bug 763167.
Attached patch Patch (obsolete) — Splinter Review
Uhmm... I ignored your comment... :(
Attachment #637291 - Flags: review?(margaret.leibovic)
Attachment #637291 - Flags: review?(margaret.leibovic)
Attached patch Uploaded PatchSplinter Review
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.
Attachment #630380 - Attachment is obsolete: true
Attachment #637291 - Attachment is obsolete: true
Attachment #637297 - Flags: review+
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.
Attachment #637297 - Flags: approval-mozilla-aurora?
Attachment #637297 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.