Closed Bug 856558 Opened 7 years ago Closed 7 years ago

Awesome bar cannot recongnize bookmark keyword with space

Categories

(Firefox for Android :: Awesomescreen, defect)

20 Branch
ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 23

People

(Reporter: angelc04, Assigned: capella)

Details

Attachments

(3 files, 2 obsolete files)

Reproduce steps:
1. Launch Fennec
2. Browse "www.google.com" and add it to Bookmark
3. Tap on Awesome bar and open Awesome screen
4. Go to Bookmarks
5. Long tap on "Google", Click on "Edit"
   Input "Google Search" as keyword
6. Click "OK" to save
7. In awesome bar, input Google search
   => The "www.google.com" bookmark does not appear in the list
8. Click on "Enter", Search result for "Google Search" appears

Expected result:
In step 7, the bookmark link should appear in the list. It seems that awesome bar cannot recongnize bookmark keyword with space in it.

Devices used:
Sumsung Galaxy S3 I9300, Android 4.0.4
Sumsung GT-P7510, Android 4.0.4
Android 4.2.2; Nexus 7
This works for me on trunk (mozilla-central) (04/01). Can you try out Nightly?
I think there is some misunderstanding here. The keyword I mentioned here is not Bookmark name. Please see attached screenshot.
(In reply to Aaron Train [:aaronmt] from comment #1)
> Created attachment 731847 [details]
> Nightly (04/01) - Screenshot
> 
> This works for me on trunk (mozilla-central) (04/01). Can you try out
> Nightly?

I tried on Nightly, behavior is the same. From your screenshot, I can see that yoru Bookmark's name is "Google Search" that's why when you input "Google Search" this bookmark appears in the list.

But if we set the 'keyword' as "Google Search"(pls see attached screenshot), when we input such keyword in awesome bar and click enter. The awesome bar didn't use this keyword to look for the existing bookmark, but goes to the default search engine to search for this keyword.

According to my understanding, this keyword for bookmark is used as a shortcut for the bookmark. 
After we create such keyword for a bookmark, firefox should be able to use such keyword to get the real URL in bookmark. 
For this case, a keyword with space does not work.
I am confused. A keyWORD with a space? How is this even allowed or expected?
Note that keywords with spaces also do not work in desktop Firefox.
Rather than allowing keywords with spaces, maybe we should morph this bug into preventing users from adding spaces to keywords in Edit?
(In reply to Brian Nicholson (:bnicholson) from comment #6)
> Rather than allowing keywords with spaces, maybe we should morph this bug
> into preventing users from adding spaces to keywords in Edit?

I'd buy that. Ian?
Flags: needinfo?(ibarlow)
Attached patch Patch (v1) (obsolete) — Splinter Review
I'm not sure if ya'll plan to move anywhere with this, but I had fun hacking this patch to provide the solution in comment 6/7
Attachment #739883 - Flags: feedback?(mark.finkle)
Comment on attachment 739883 [details] [diff] [review]
Patch (v1)

LGTM
Attachment #739883 - Flags: feedback?(mark.finkle) → feedback+
Attachment #739883 - Flags: review?(bnicholson)
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Comment on attachment 739883 [details] [diff] [review]
Patch (v1)

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

Except for the onTextChanged() implementations, these classes are the same. Could you create another class (EditBookmarkTextWatcher?) that holds the shared code for these classes? Then each of these TextWatchers could extend that class and would only need to implement their respective onTextChanged() methods without duplicating all the other code.
Attached patch Patch (v2) (obsolete) — Splinter Review
Something closer to this?
Comment on attachment 740570 [details] [diff] [review]
Patch (v2)

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

Better, but it would be nice if this was factored out even more. Could you put these lines in the abstract class?

+            boolean enabled = mEnabled && (mPairedTextWatcher == null || mPairedTextWatcher.isEnabled());
+            mDialog.getButton(AlertDialog.BUTTON_POSITIVE).setEnabled(enabled);

Then have the overridden classes call super()?

Also, if you want, EditBookmarkTextWatcher could have a constructor, and each extending class would have a constructor that calls super(). Or you could have empty constructors and have a create/init method that takes the dialog, paired watcher, and corresponding EditText that sets everything up in one method.
Attachment #740570 - Flags: feedback+
Attached patch Patch (v3)Splinter Review
Wow! I really like this level of abstraction ... I stopped short of the final idea you mentioned, in favor of this one.

Let me know if you think of any further cool twists :D
Attachment #742520 - Flags: review?(bnicholson)
Comment on attachment 742520 [details] [diff] [review]
Patch (v3)

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

::: mobile/android/base/AwesomeBar.java
@@ +520,5 @@
>      }
>  
> +    private abstract class EditBookmarkTextWatcher implements TextWatcher {
> +        protected AlertDialog mDialog;
> +        protected EditBookmarkTextWatcher mPairedTextWatcher = null;

Nit: For consistency, I'd either remove the null initializer here or also explicitly initialize mDialog to null.

@@ +541,5 @@
> +            // Disable if the we're disabled or paired partner is disabled
> +            boolean enabled = mEnabled && (mPairedTextWatcher == null || mPairedTextWatcher.isEnabled());
> +            mDialog.getButton(AlertDialog.BUTTON_POSITIVE).setEnabled(enabled);
> +        }
> + 

Nit: remove trailing whitespace

@@ +549,5 @@
> +        public void beforeTextChanged(CharSequence s, int start, int count, int after) {}
> +    }
> +
> +    private class LocationTextWatcher extends EditBookmarkTextWatcher implements TextWatcher {
> +        public LocationTextWatcher(AlertDialog aDialog) { super(aDialog); }

Nit: I don't think we normally one-line methods like this, so I'd add newlines before/after the super call.

@@ +560,5 @@
> +        }
> +    }
> +
> +    private class KeywordTextWatcher extends EditBookmarkTextWatcher implements TextWatcher {
> +        public KeywordTextWatcher(AlertDialog aDialog) { super(aDialog); }

Same here.
Attachment #742520 - Flags: review?(bnicholson) → review+
Pushed to TRY:
https://tbpl.mozilla.org/?tree=Try&rev=964f51800192

Ack! RoboCopy test failure ! Added correction to patch and pushed to TRY again:
https://tbpl.mozilla.org/?tree=Try&rev=ac1ae3594e51
on to inbound with the (robocop) corrected patch
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f5c7725deb9
https://hg.mozilla.org/mozilla-central/rev/8f5c7725deb9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Attachment #739883 - Attachment is obsolete: true
Attachment #739883 - Flags: review?(bnicholson)
Attachment #740570 - Attachment is obsolete: true
Flags: needinfo?(ibarlow)
You need to log in before you can comment on or make changes to this bug.