872388 - Allow creating a homescreen shortcut when an bookmark is made

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

We've talked before about creating a sort of Super Toast notification to better expose our "Add to homescreen" functionality.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Screenshot

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.

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch 1/3 - ButtonToast class

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.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Path 2/3 - Integrate ButtonToast in BrowserApp

This brings the toast class in to BrowserApp and styles it.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch 3/3 - Use ButtonToast for Add bookmark toasts

This actually uses the ButtonToast class when showing the "Bookmark Added" prompt.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch 1/5 - ButtonToast class

Saving my place again.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Patch 2/5 - Integrate into browserApp

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch 3/5 - Move EditBookmark into its own class

We should probably do this regardless, but it was useful here.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Patch 4/5 - Use for bookmarks

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch 5/5 - Add buttons to the toast api

Comment 10

[Wesley Johnston (:wesj)]

Attachment: 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.

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Screenshot 2 - Menu

Comment 12

[Ian Barlow (:ibarlow)]

(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 13

[Brian Nicholson (:bnicholson)]

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?

Comment 14

[Wesley Johnston (:wesj)]

Attachment: Patch 3/5 - Move EditBookmark into its own class

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!

Comment 15

[Wesley Johnston (:wesj)]

Attachment: Patch 4/5 - Use for bookmarks

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.

Comment 16

[Wesley Johnston (:wesj)]

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.

Comment 17

[Wesley Johnston (:wesj)]

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.

Comment 18

[Brian Nicholson (: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

Comment 19

[Brian Nicholson (:bnicholson)]

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.

Comment 20

[Brian Nicholson (:bnicholson)]

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.

Comment 21

[Wesley Johnston (:wesj)]

(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 22

[Brian Nicholson (:bnicholson)]

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.

Comment 23

[Brian Nicholson (:bnicholson)]

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.

Comment 24

[Wesley Johnston (:wesj)]

I just made the same mistake twice and forgot to fixup some review comments here. :( Will do that real quick in a follow up....

https://hg.mozilla.org/integration/mozilla-inbound/rev/2076903c21fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/7acb6c63fdf4
https://hg.mozilla.org/integration/mozilla-inbound/rev/684ba082942b
https://hg.mozilla.org/integration/mozilla-inbound/rev/416f9cdde0f7

Comment 25

[Wesley Johnston (:wesj)]

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.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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

Comment 27

[Wesley Johnston (:wesj)]

Attachment: Patch 6/5 - Gingerbread fixes

Fixes gingerbread errors. Pushing to try;

https://tbpl.mozilla.org/?tree=Try&rev=95676be93cbe

Comment 28

[Sriram Ramasubramanian [:sriram]]

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 29

[Brian Nicholson (:bnicholson)]

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.

Comment 30

[Brian Nicholson (:bnicholson)]

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).

Comment 31

[Brian Nicholson (:bnicholson)]

(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 32

[Brian Nicholson (:bnicholson)]

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.

Comment 33

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1fb1dd02790
https://hg.mozilla.org/integration/mozilla-inbound/rev/d43f62520151
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2d2ee487ac4

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f1fb1dd02790
https://hg.mozilla.org/mozilla-central/rev/d43f62520151
https://hg.mozilla.org/mozilla-central/rev/f2d2ee487ac4

Comment 35

[M Schulze]

isn't this done by now? Upon creating a bookmark, you have a small notification with 'settings' that allows you to add the bookmark.

Comment 36

[Aaron Train [:aaronmt]]

Yes.