Allow creating a homescreen shortcut when an bookmark is made

RESOLVED WORKSFORME

Status

()

defect
RESOLVED WORKSFORME
6 years ago
3 years ago

People

(Reporter: wesj, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [leave-open])

Attachments

(8 attachments, 6 obsolete attachments)

6.03 KB, patch
bnicholson
: review+
Details | Diff | Splinter Review
14.48 KB, patch
bnicholson
: review+
Details | Diff | Splinter Review
19.61 KB, patch
bnicholson
: review-
Details | Diff | Splinter Review
248.11 KB, image/png
Details
178.91 KB, image/png
Details
20.33 KB, patch
bnicholson
: review+
Details | Diff | Splinter Review
8.10 KB, patch
bnicholson
: review+
Details | Diff | Splinter Review
7.19 KB, patch
bnicholson
: review+
Details | Diff | Splinter Review
We've talked before about creating a sort of Super Toast notification to better expose our "Add to homescreen" functionality.
Posted image Screenshot (obsolete) —
Screenshot of this on my S3. I used the string "Add shortcut" because we need something short. The implementation supports an icon, but I didn't have one for here.
Posted patch Patch 1/3 - ButtonToast class (obsolete) — Splinter Review
This is a slightly modified version of the undobar from here:

http://code.google.com/p/romannurik-code/source/browse/misc/undobar/

Modified to allow passing in an icon resource, button text, and to use a string instead of a Parcelable (which seems like overkill for our uses). It doesn't handle multiple toasts being shown simultaneously, but I think that's fine for now?

This patch doesn't quite build because it defines a few id's that aren't in this patch.
This brings the toast class in to BrowserApp and styles it.
This actually uses the ButtonToast class when showing the "Bookmark Added" prompt.
Saving my place again.
Attachment #749665 - Attachment is obsolete: true
We should probably do this regardless, but it was useful here.
Attachment #754300 - Flags: review?(bnicholson)
Posted patch Patch 4/5 - Use for bookmarks (obsolete) — Splinter Review
Attachment #749667 - Attachment is obsolete: true
Attachment #749668 - Attachment is obsolete: true
Posted image Screenshot
Mark had the idea to show a "More" button here and then use a context menu. This is how that looks. Holding off on any reviews here until we've had a chance to see what UX thinks.
Attachment #749664 - Attachment is obsolete: true
Posted image Screenshot 2 - Menu
(In reply to Wesley Johnston (:wesj) from comment #10)
> Created attachment 754303 [details]
> Screenshot
> 
> Mark had the idea to show a "More" button here and then use a context menu.
> This is how that looks. Holding off on any reviews here until we've had a
> chance to see what UX thinks.

I like this approach. Maybe "Options" instead of "More"?
Comment on attachment 754300 [details] [diff] [review]
Patch 3/5 - Move EditBookmark into its own class

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

::: mobile/android/base/EditBookmarkDialog.java
@@ +17,5 @@
> +import android.text.TextWatcher;
> +import android.view.View;
> +import android.widget.EditText;
> +import android.widget.Toast;
> +import android.database.Cursor;

Nit: Move this import after "android.content.DialogInterface" to keep alphabetical order.

@@ +21,5 @@
> +import android.database.Cursor;
> +
> +public class EditBookmarkDialog {
> +    private static final String LOGTAG = "GeckoEditBookmarkDialog";
> +    private Activity mActivity;

Can you use Context here instead of passing a full-blown Activity?

@@ +97,5 @@
> +            super.onTextChanged(s, start, before, count);
> +       }
> +    }
> +
> +    public void show(final String url) {

Could you add Javadocs here and for the following method? It'd be nice if we could make a habit of documenting all of our new code that is exposed in our API.

@@ +127,5 @@
> +    }
> +
> +    public void show(final int id, final String title, final String url, final String keyword) {
> +        AlertDialog.Builder editPrompt = new AlertDialog.Builder(mActivity);
> +        final View editView = mActivity.getLayoutInflater().inflate(R.layout.bookmark_edit, null);

You could use LayoutInflater.from(mContext) so you don't need an activity.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +1235,5 @@
> +                         new String[] { url },
> +                         null);
> +            if (c.getCount() == 0) {
> +              c.close();
> +              c = null;

Nit: indentation

@@ +1237,5 @@
> +            if (c.getCount() == 0) {
> +              c.close();
> +              c = null;
> +            }
> +        } catch (NullPointerException e) {

Why are you catching a NPE here?
I think this fixes the comments. I added some JavaDocs and removed the NPE catch which I think I copy-pasted before. I'm not sure why we have any of those though...

I'm also going to have you review all of this if Ian's willing to give it a try!
Attachment #754300 - Attachment is obsolete: true
Attachment #754300 - Flags: review?(bnicholson)
Attachment #755071 - Flags: review?(bnicholson)
This updates the string to "Options" and removes the icon (I played with using the "menu" three-dots icon a bit, but couldn't really find anything that looked nice). I'm leaving the capability to add icons for now, but we could remove it.
Attachment #754301 - Attachment is obsolete: true
Attachment #755073 - Flags: review?(bnicholson)
Comment on attachment 754302 [details] [diff] [review]
Patch 5/5 - Add buttons to the toast api

This supports icons as well. If we got rid of that, I still want some of this refactoring because I'd eventually like to add support for icons in context menus.
Attachment #754302 - Flags: review?(bnicholson)
Attachment #754299 - Flags: review?(bnicholson)
Comment on attachment 754298 [details] [diff] [review]
Patch 1/5 - ButtonToast class

This is mostly taken straight from:

http://code.google.com/p/romannurik-code/source/browse/misc/undobar/

It doesn't handle multiple toasts shown simultaneously very well. I'd love to fix that at some point, but thought it was better to keep it purerer for this first landing.
Attachment #754298 - Flags: review?(bnicholson)
Comment on attachment 755071 [details] [diff] [review]
Patch 3/5 - Move EditBookmark into its own class

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

::: mobile/android/base/EditBookmarkDialog.java
@@ +29,5 @@
> +public class EditBookmarkDialog {
> +    private static final String LOGTAG = "GeckoEditBookmarkDialog";
> +    private Context mContext;
> +
> +    public EditBookmarkDialog(Context activity) {

Nit: s/activity/context/

::: mobile/android/base/db/LocalBrowserDB.java
@@ +1234,5 @@
> +                            null);
> +        if (c == null) {
> +            return c;
> +        } else if (c.getCount() == 0) {
> +          c.close();

Nit: needs 4-space indentation
Attachment #755071 - Flags: review?(bnicholson) → review+
Comment on attachment 754298 [details] [diff] [review]
Patch 1/5 - ButtonToast class

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

It seems weird to me that this class acts on a given view instead of extending from a view directly like all other custom views we have. I guess an advantage of this composition approach is that any generic view can be used as the toast view.
Attachment #754298 - Flags: review?(bnicholson) → review+
Comment on attachment 754299 [details] [diff] [review]
Patch 2/5 - Integrate into browserApp

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

::: mobile/android/base/resources/drawable/toast_button.xml
@@ +1,2 @@
> +<!--
> +  Copyright 2012 Roman Nurik

If this is still from third party code, maybe it would be better to merge this, the images, and the styles into the first patch? That way the first patch would fully import all the external code, and this patch could be responsible for BrowserApp integration and any custom changes to XML/images/styles.
Attachment #754299 - Flags: review?(bnicholson) → review+
(In reply to Brian Nicholson (:bnicholson) from comment #20)
> If this is still from third party code, maybe it would be better to merge
> this, the images, and the styles into the first patch? That way the first
> patch would fully import all the external code, and this patch could be
> responsible for BrowserApp integration and any custom changes to
> XML/images/styles.

I actually threw out most of the styling from the original third party code becuase... it was kinda ugly. But the new images are ALSO third party taken from the Android source...
Comment on attachment 755073 [details] [diff] [review]
Patch 4/5 - Use for bookmarks

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

::: mobile/android/base/BrowserApp.java
@@ +60,5 @@
>  import android.view.ViewGroup;
>  import android.view.animation.Interpolator;
>  import android.widget.LinearLayout;
>  import android.widget.RelativeLayout;
> +import android.widget.PopupMenu;

Unused import?

@@ +86,5 @@
>  
>      private static final int READER_ADD_SUCCESS = 0;
>      private static final int READER_ADD_FAILED = 1;
>      private static final int READER_ADD_DUPLICATE = 2;
> +    private static final String ADD_SHORTCUT_TOAST = "add_shortcut_toast";

Nit: new line above here

@@ +363,5 @@
>  
>          RelativeLayout actionBar = (RelativeLayout) findViewById(R.id.browser_toolbar);
>  
>          mToast = new ButtonToast(findViewById(R.id.toast), new ButtonToast.ToastListener() {
>              public void onButtonClicked(CharSequence token) {

I overlooked this in the last patch, but there should be an @Overrides annotation here to make it immediately obvious that this method is implementing an interface.

@@ +364,5 @@
>          RelativeLayout actionBar = (RelativeLayout) findViewById(R.id.browser_toolbar);
>  
>          mToast = new ButtonToast(findViewById(R.id.toast), new ButtonToast.ToastListener() {
>              public void onButtonClicked(CharSequence token) {
> +                Log.i(LOGTAG, "Clicked " + token);

Remove this

@@ +477,5 @@
>          });
>      }
>  
> +    private void showBookmarkDialog() {
> +        final Prompt ps = new Prompt(BrowserApp.this, new Prompt.PromptCallback() {

s/BrowserApp.this/this/ ?

@@ +478,5 @@
>      }
>  
> +    private void showBookmarkDialog() {
> +        final Prompt ps = new Prompt(BrowserApp.this, new Prompt.PromptCallback() {
> +            public void onPromptFinished(String result) {

Add @Overrides

@@ +482,5 @@
> +            public void onPromptFinished(String result) {
> +                int itemId = -1;
> +                try {
> +                  itemId = new JSONObject(result).getInt("button");
> +                } catch(Exception ex) { }

s/Exception/JSONException/. Also, I'd add a Log.e() here to help debug any future changes to the JSON that might break this code.

@@ +484,5 @@
> +                try {
> +                  itemId = new JSONObject(result).getInt("button");
> +                } catch(Exception ex) { }
> +
> +                Tab tab = Tabs.getInstance().getSelectedTab();

Since the prompt service is asynchronous, couldn't the selected tab change between the prompt being created and finished? If so, you should probably get this tab reference right before creating the prompt.
Attachment #755073 - Flags: review?(bnicholson) → review+
Comment on attachment 754302 [details] [diff] [review]
Patch 5/5 - Add buttons to the toast api

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

Looks OK overall, but I think there's some kinks that need to be ironed out first.

::: mobile/android/base/BrowserApp.java
@@ +367,4 @@
>                  if (ADD_SHORTCUT_TOAST.equals(token)) {
>                      showBookmarkDialog();
> +                } else if (token.toString().startsWith(GECKO_TOAST)) {
> +                    GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Toast:Click", token.toString().replace(GECKO_TOAST, "")));

It looks like you're trying to support multiple toasts at the same time, but I think we'd run into issues if we tried to create multiple button toasts at the same time since the text/images/tokens would replace each other. We should either a) stack them like doorhangers, or, probably easier, b) queue them up so they're displayed in turn like Android toasts do.

@@ +1349,5 @@
>              }
>          });
>  
>          if (info.icon != null) {
> +            BitmapUtils.getDrawable(BrowserApp.this, info.icon, new BitmapUtils.BitmapLoader() {

s/BrowserApp.this/this/

@@ +1350,5 @@
>          });
>  
>          if (info.icon != null) {
> +            BitmapUtils.getDrawable(BrowserApp.this, info.icon, new BitmapUtils.BitmapLoader() {
> +                public void onBitmapFound(Drawable d) {

@Override

::: mobile/android/base/GeckoApp.java
@@ +159,5 @@
>  
>      protected DoorHangerPopup mDoorHangerPopup;
>      protected FormAssistPopup mFormAssistPopup;
>      protected TabsPanel mTabsPanel;
> +    protected ButtonToast mToast;

I don't like how this is handled in BrowserApp but created in GeckoApp. Aside from the disconnect in logic, doesn't this mean that toast buttons won't work for webapps?

@@ +744,5 @@
>              @Override
>              public void run() {
> +                if (!TextUtils.isEmpty(buttonId) &&
> +                   (!TextUtils.isEmpty(buttonText) || !TextUtils.isEmpty(buttonIcon))) {
> +                    Log.i(LOGTAG, "Got icon: " + buttonIcon);

Drop this

::: mobile/android/base/gfx/BitmapUtils.java
@@ +34,5 @@
> +    public interface BitmapLoader {
> +      public void onBitmapFound(Drawable d);
> +    }
> +
> +    public static void getDrawable(final Context context, final String data, final BitmapLoader loader) {

Can you add Javadocs for this and the BitmapLoader interface?

@@ +41,5 @@
> +            return;
> +        }
> +
> +        if (data.startsWith("data")) {
> +            BitmapDrawable d = new BitmapDrawable(BitmapUtils.getBitmapFromDataURI(data));

No need for "BitmapUtils."

@@ +46,5 @@
> +            loader.onBitmapFound(d);
> +
> +        } else if (data.startsWith("jar:") || data.startsWith("file://")) {
> +            (new AsyncTask<Void, Void, Drawable>() {
> +                public Drawable doInBackground(Void... params) {

@Override

@@ +65,5 @@
> +                    }
> +                    return null;
> +                }
> +
> +                public void onPostExecute(Drawable drawable) {

@Override

::: mobile/android/base/widget/ButtonToast.java
@@ +84,5 @@
> +                     CharSequence token) {
> +        showInternal(immediate, message, buttonMessage, null, token);
> +
> +        BitmapUtils.getDrawable(mView.getContext(), buttonIcon, new BitmapUtils.BitmapLoader() {
> +            public void onBitmapFound(Drawable d) {

Add @Override annotation

::: mobile/android/chrome/content/browser.js
@@ +119,5 @@
>  XPCOMUtils.defineLazyServiceGetter(this, "Haptic",
>    "@mozilla.org/widget/hapticfeedback;1", "nsIHapticFeedback");
>  
> +XPCOMUtils.defineLazyServiceGetter(this, "idService",
> +  "@mozilla.org/uuid-generator;1", "nsIUUIDGenerator"); 

Nit: trailing whitespace

@@ +1546,5 @@
> +      };
> +
> +      if (aOptions.button) {
> +        msg.button = aOptions.button.label;
> +        msg.buttonIcon = resolveGeckoURI(aOptions.button.icon);

Can we allow a button with a label but no image, or vice-versa? Maybe we should require the button to have a label or an icon, but not necessarily both.

@@ +1547,5 @@
> +
> +      if (aOptions.button) {
> +        msg.button = aOptions.button.label;
> +        msg.buttonIcon = resolveGeckoURI(aOptions.button.icon);
> +        msg.buttonId = idService.generateUUID().toString(); 

Nit: trailing whitespace

@@ +1656,5 @@
> +    } else if (aTopic == "Toast:Click") {
> +      if (this.toast._callbacks[aData]) {
> +        this.toast._callbacks[aData]();
> +        delete this.toast._callbacks[aData];
> +      }

What if the toast button isn't clicked? You'll probably need something like a Toast:End to handle the cleanup so we don't leak callbacks.
Attachment #754302 - Flags: review?(bnicholson) → review-
nits:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2fbb5c6cfc7

Note, I didn't land the 5th, r- patch. Will try to fix it up soon, but I think we can live without a JS api for this for now.
We can't live with this timing out every Android 2.2 test under the sun, however. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0f39113f42b
Fixes gingerbread errors. Pushing to try;

https://tbpl.mozilla.org/?tree=Try&rev=95676be93cbe
Attachment #758054 - Flags: review?(bnicholson)
Comment on attachment 755071 [details] [diff] [review]
Patch 3/5 - Move EditBookmark into its own class

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

::: mobile/android/base/AwesomeBar.java
@@ +565,5 @@
>                  openUrlAndFinish(ReaderModeUtils.getAboutReaderForUrl(url, true));
>                  break;
>              }
>              case R.id.edit_bookmark: {
> +                new EditBookmarkDialog(this).show(id, title, url, keyword);

Drive-by: This call sounds different from the notion of how Dialogs are created and shown.
Ideally the call to show a dialog should be like:

EditBookmarkDialog.Builder builder = new EditBookmarkDialog.Builder(this);
builder.setXYZ(...);
builder.show();

Passing arguments to show() feels so unconventional.
Comment on attachment 758054 [details] [diff] [review]
Patch 6/5 - Gingerbread fixes

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

Even without the JS API, I'm still concerned about the multiple toasts as described in comment 23. Do we have any safeguards to prevent two toasts from colliding with each other? How hard do you think it would be to create a queue to make toasts shown one at a time in turn?

::: mobile/android/base/widget/ButtonToast.java
@@ +27,2 @@
>  import android.view.View;
>  import android.view.ViewPropertyAnimator;

Drop this import

@@ -106,2 @@
>  
> -        } else {

Could we keep this else here? If immediate is true, it seems like we're doing unnecessary work when we could just set the alpha to 0 and be done with it.
Just tested this + bug 880454, and they seem to work pretty well. I did hit a NPE that should be fixed before landing:

06-09 20:49:03.123  2503  2503 E GeckoAppShell: java.lang.NullPointerException
06-09 20:49:03.123  2503  2503 E GeckoAppShell: 	at org.mozilla.gecko.widget.ButtonToast$1.onClick(ButtonToast.java:82)
06-09 20:49:03.123  2503  2503 E GeckoAppShell: 	at android.view.View.performClick(View.java:4106)
...

which is at this line:

                        mListener.onButtonClicked(t.token);

It happens when holding down the options button, waiting for the toast to vanish, then releasing the button. The NPE is from t being null since mCurrentToast is set to null when the toast is removed.

The easy fix would be to just do a null check before this line; a better fix from a UX perspective would be to not let the toast disappear as long as it's still visible. I'm fine with whichever you choose (and maybe we can fix the disappearing issue in a follow-up if you choose the former).
(In reply to Brian Nicholson (:bnicholson) from comment #30)
> The easy fix would be to just do a null check before this line; a better fix
> from a UX perspective would be to not let the toast disappear as long as
> it's still visible.

Sorry, that made no sense. I meant "not let the toast disappear as long as it's still being touched".
Comment on attachment 758054 [details] [diff] [review]
Patch 6/5 - Gingerbread fixes

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

The NPE is really from bug 880454, so this looks fine.
Attachment #758054 - Flags: review?(bnicholson) → review+
Depends on: 885717
Depends on: 886359
Depends on: 899560
Depends on: 900385
isn't this done by now? Upon creating a bookmark, you have a small notification with 'settings' that allows you to add the bookmark.
Yes.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.