Closed Bug 724194 Opened 8 years ago Closed 8 years ago

Allow editing bookmarks in AwesomeScreen

Categories

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

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 13
Tracking Status
firefox13 --- verified
firefox14 --- verified

People

(Reporter: bnicholson, Assigned: bnicholson)

References

Details

Attachments

(1 file, 3 obsolete files)

Add UI to edit bookmarks.
Attached patch patch (obsolete) — Splinter Review
Attachment #594383 - Flags: review?(mark.finkle)
Depends on: 721776, 722184
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?
(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.
Priority: -- → P4
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.
Attachment #594383 - Flags: review?(mark.finkle) → review-
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.
Attached patch patch v2 (obsolete) — Splinter Review
rebased
Attachment #594383 - Attachment is obsolete: true
Attachment #604195 - Flags: review?(mark.finkle)
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.
Attachment #604195 - Flags: review?(mark.finkle) → review-
Attached patch patch v3 (obsolete) — Splinter Review
Updated with changes. DB access on different thread, toaster, and empty URL check.
Attachment #604195 - Attachment is obsolete: true
Attachment #604846 - Flags: review?(mark.finkle)
Attached patch patch v3Splinter Review
second attempt; forgot to refresh patch
Attachment #604846 - Attachment is obsolete: true
Attachment #604846 - Flags: review?(mark.finkle)
Attachment #604847 - Flags: review?(mark.finkle)
Attachment #604847 - Flags: review?(mark.finkle) → review+
Flags: in-testsuite?
Flags: in-litmus?(fennec)
https://hg.mozilla.org/mozilla-central/rev/0c42c66992f1
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13
Depends on: 737782
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
Status: RESOLVED → VERIFIED
Flags: in-litmus?(fennec) → in-litmus+
You need to log in before you can comment on or make changes to this bug.