724194 - Allow editing bookmarks in AwesomeScreen

Status: VERIFIED FIXED

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

Description

[Brian Nicholson (:bnicholson)]

Add UI to edit bookmarks.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: patch

Comment 2

[:Margaret Leibovic]

Comment on attachment 594383 [details] [diff] [review]
patch

A few drive-by comments...

>diff --git a/mobile/android/base/AwesomeBar.java b/mobile/android/base/AwesomeBar.java
> 
>-        String url = "";
>+        final String url;
>+        final String title;
>         byte[] b = null;
>-        String title = "";
>+        Cursor cursor = null;
>         if (mContextMenuSubject instanceof Cursor) {
>-            Cursor cursor = (Cursor)mContextMenuSubject;
>+            cursor = (Cursor)mContextMenuSubject;
>             url = cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.URL));
>             b = cursor.getBlob(cursor.getColumnIndexOrThrow(URLColumns.FAVICON));
>             title = cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.TITLE));
>         } else if (mContextMenuSubject instanceof Map) {
>             Map map = (Map)mContextMenuSubject;
>             url = (String)map.get(URLColumns.URL);
>             b = (byte[]) map.get(URLColumns.FAVICON);
>             title = (String)map.get(URLColumns.TITLE);

Hm, now that you're setting a cursor, maybe we can update the changes I just landed for bug 724756 to use that instead of creating an id variable up here (see the first version of that patch). I can also do this myself in a follow-up :)

>@@ -478,16 +481,55 @@ public class AwesomeBar extends Activity
>         mContextMenuSubject = null;
> 
>         switch (item.getItemId()) {
>             case R.id.open_new_tab: {
>                 GeckoApp.mAppContext.loadUrl(url, AwesomeBar.Type.ADD);
>                 Toast.makeText(this, R.string.new_tab_opened, Toast.LENGTH_SHORT).show();
>                 break;
>             }
>+            case R.id.edit_bookmark: {
>+                final ContentResolver resolver = Tabs.getInstance().getContentResolver();

You're not actually using this variable anywhere...

>+                AlertDialog.Builder editPrompt = new AlertDialog.Builder(this);
>+                View editView = getLayoutInflater().inflate(R.layout.bookmark_edit, null);
>+                editPrompt.setTitle(R.string.contextmenu_edit_bookmark);
>+                editPrompt.setView(editView);
>+
>+                final EditText nameText = ((EditText) editView.findViewById(R.id.edit_bookmark_name));
>+                final EditText locationText = ((EditText) editView.findViewById(R.id.edit_bookmark_location));
>+                final EditText keywordText = ((EditText) editView.findViewById(R.id.edit_bookmark_keyword));
>+                nameText.setText(title);
>+                locationText.setText(url);
>+
>+                int column = cursor.getColumnIndex(URLColumns.KEYWORD);
>+                if (column == -1) {
>+                    // keyword column is not available in android db
>+                    keywordText.setVisibility(View.GONE);
>+                } else {
>+                    keywordText.setText(cursor.getString(column));
>+                }
>+
>+                editPrompt.setPositiveButton(R.string.button_ok, new DialogInterface.OnClickListener() {
>+                    public void onClick(DialogInterface dialog, int whichButton) {
>+                        BrowserDB.updateBookmark(mResolver, url, locationText.getText().toString(),
>+                                                 nameText.getText().toString(), keywordText.getText().toString());

... did you mean to use it here instead of mResolver?

Comment 3

[Brian Nicholson (:bnicholson)]

(In reply to Margaret Leibovic [:margaret] from comment #2)
>
> You're not actually using this variable anywhere...
> 

Thanks, good catch. I've rebased this patch several times (...and it looks like it'll need to be changed again with the incoming bookmarks observer), so I must have missed this line. I meant to delete it since I replaced all these local resolver instances with a single mResolver in another patch.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 594383 [details] [diff] [review]
patch

This patch looks fine, but has been bitrot a bit. Can you unbitrot and post a new patch? I have been debating whether we would take this for initial release. It's a small patch, so I think we would.

Comment 5

[:Margaret Leibovic]

I wanted to write a test for the awesome screen context menu, so maybe you (or I) could also do that if it's not too hard.

Comment 6

[Brian Nicholson (:bnicholson)]

Attachment: patch v2

rebased

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 604195 [details] [diff] [review]
patch v2

>diff --git a/mobile/android/base/AwesomeBar.java b/mobile/android/base/AwesomeBar.java

>+            case R.id.edit_bookmark: {

>+                editPrompt.setPositiveButton(R.string.button_ok, new DialogInterface.OnClickListener() {
>+                    public void onClick(DialogInterface dialog, int whichButton) {
>+                        BrowserDB.updateBookmark(mResolver, url, locationText.getText().toString(),
>+                                                 nameText.getText().toString(), keywordText.getText().toString());
>+                    }

* Should this be in a Runnable off the main UI?
* We should make sure the new URL is not empty. Do not try to save an empty URL. You might also want to add code to disable the OK button if the Location edit box is empty.
* We should probably show a Toast - "Bookmark updated"

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd

>+<!ENTITY contextmenu_edit_bookmark "Edit">

>+<!ENTITY contextmenu_edit_name "Name">
>+<!ENTITY contextmenu_edit_location "Location">
>+<!ENTITY contextmenu_edit_keyword "Keyword">

* Change these from "contextmenu_edit_*" to "bookmark_edit_*"
* Add: bookmark_edit_title "Edit Bookmark"  and use it for the title of the dialog

r- for the runnable question and empty URL check. need a new patch.

Comment 8

[Brian Nicholson (:bnicholson)]

Attachment: patch v3

Updated with changes. DB access on different thread, toaster, and empty URL check.

Comment 9

[Brian Nicholson (:bnicholson)]

Attachment: patch v3

second attempt; forgot to refresh patch

Comment 10

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/0c42c66992f1

Comment 11

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/0c42c66992f1

Comment 12

[Paul Feher]

Verified/fixed on:

Aurora Fennec 13.0a2 (2012-03-29)
Nightly Fennec 14.0a1 (2012-03-29)
Device: Samsung Nexus S
OS: Android 2.3.6

Testcase added in Litmus
https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=53249