910768 - Add a link to the marketplace in the "No apps installed for this url" toast

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

We currently show a little "No installed applications can handle this link" toast when you click a link to a uri scheme that Fennec doesn't natively support. We could use button toast to add a market link  to the toast so that the user can search for apps.

The stock browser does something like this already (without prompts):

http://androidxref.com/4.3_r2.1/xref/packages/apps/Browser/src/com/android/browser/UrlHandler.java#129

Maybe we could avoid the prompts here as well, but clicking a link and having the play store come up seems like it might feel confusing.

Comment 1

[Wesley Johnston (:wesj)]

I can mentor someone on this. We'll have to do some jugging to get Java to generate a URI intent, but this message is actually sent from JS. We'll also need some UX feedback.

In JS, we'll need to add a button to the toast we show for these links here:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/ContentDispatchChooser.js

(I need to update our docs to explain how to add buttons to these, but those docs will be at https://developer.mozilla.org/en-US/docs/Extensions/Mobile/API/NativeWindow/toast).

When the button is clicked, I think as a first round, I'd just try launching a different uri to "market://search?q=pname:<My_URI>", but we may need to do something fancier to get Android to generate an intent/packagename for us....

Comment 2

[Errietta Kostala [:errietta]]

Attachment: Proposed patch

Depends on #921756 being fixed

Comment 3

[Errietta Kostala [:errietta]]

Attachment: Screenshot of button

Comment 4

[Wesley Johnston (:wesj)]

Nice. Can you look at this and give some feedback ian?

Comment 5

[Ian Barlow (:ibarlow)]

I'm not sure I understand what we're trying to do here, or what is meant to happen when you click the rocket icon. Can you provide a little more context?

Comment 6

[Ian Barlow (:ibarlow)]

Talked to Wes, I get it now. The rocket was throwing me off, heh. I have a couple tweaks to the copy, and we should be using the Google Play icon here as well.


--------------------------------------------------------
                          |
Couldn't find an app to   |   [icon] Search Google Play
open this link            |
                          |
--------------------------------------------------------

Comment 7

[Wesley Johnston (:wesj)]

We have to be a little careful, as the user might not have Play installed, or if they've installed Yandex AND play, the market:// link may default to Yandex.

Comment 8

[Ian Barlow (:ibarlow)]

Can we tell what they have installed at all? I don't think the rocket is a clear metaphor at all.

Comment 9

[Wesley Johnston (:wesj)]

Yeah, we should be able to query the default handler and show its name (and icon with some patches in my queue).

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 811582 [details] [diff] [review]
Proposed patch

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

Nice. My preference is to just land this without an icon for now. In v2, we can look at showing the app-icon (although we don't have the package name here, so it will be a bit difficult).

::: mobile/android/components/ContentDispatchChooser.js
@@ +58,5 @@
> +
> +        win.NativeWindow.toast.show(failedText, "long", {
> +          button: {
> +            label: searchText,
> +            icon: "drawable://alert_app",

TBH, We don't have to have an icon here (and we don't use it anywhere else we show these). This text doesn't really look like a button, and the more words you add, the less it does. Maybe we just remove the image for the first draft and just show "Search".

Comment 11

[Errietta Kostala [:errietta]]

Attachment: Patch without icon

Comment 12

[Ian Barlow (:ibarlow)]

Wes showed me a screenshot earlier: https://www.dropbox.com/s/hfldnmbyjvwavok/Screenshot_2013-10-10-02-00-26.png

I think we can tweak a few things here: 

Mockup - http://cl.ly/image/453k1I1m3n3Q
1. Change "application" to "app"
2. Split the bar to be 2/3 message, 1/3 action button
3. Place the icon above the action text
4. Use flat, 1 colour icon
5. Make action text smaller (12sp?)

Comment 13

[Wesley Johnston (:wesj)]

Hmm.. the scope creeps. We originally designed this to model the gmail toast, hence the icon placement. I wonder if we want to leave it in place and allow users to pick between the two layouts? Or maybe just have an "iconPosition" property and let you specify left, right, top, bottom.

#4 will be difficult... we're just using the icon that the market gives us. We could try just putting a solid white tint on the imageView: http://developer.android.com/reference/android/widget/ImageView.html#attr_android:tint

Comment 14

[Ian Barlow (:ibarlow)]

(In reply to Wesley Johnston (:wesj) from comment #13)
> Hmm.. the scope creeps. We originally designed this to model the gmail
> toast, hence the icon placement. I wonder if we want to leave it in place
> and allow users to pick between the two layouts? Or maybe just have an
> "iconPosition" property and let you specify left, right, top, bottom.

Well, we could keep the icon on the left, like this maybe http://cl.ly/image/3q442o2p151Z

> #4 will be difficult... we're just using the icon that the market gives us.
> We could try just putting a solid white tint on the imageView:
> http://developer.android.com/reference/android/widget/ImageView.
> html#attr_android:tint

I'd rather just use whatever we get then, but scale it down as per the mockup

Comment 15

[Wesley Johnston (:wesj)]

Comment on attachment 814624 [details] [diff] [review]
Patch without icon

Clearing this. I think we've made progress on the final version, but I'll wait for it :)

Comment 16

[Errietta Kostala [:errietta]]

Attachment: Another patch

Comment 17

[Wesley Johnston (:wesj)]

Comment on attachment 818072 [details] [diff] [review]
Another patch

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

A few little changes and I think this is good! Nice work!

::: mobile/android/base/GeckoApp.java
@@ +694,5 @@
>                      Log.e(LOGTAG, "Received Contact:Add message with no email nor phone number");
>                  }                
>              } else if (event.equals("Intent:GetHandlers")) {
> +                String url = message.optString("url");
> +                Uri uri = url.indexOf(':') >= 0 ? Uri.parse(url) : new Uri.Builder().scheme(url).build();

I think this might be better just included in getOpenURIIntent. It looks like there are two callers of that in GeckoAppShell. One of them already does it. The other doesn't, but wouldn't be hurt (AFAICT).

::: mobile/android/components/ContentDispatchChooser.js
@@ +79,5 @@
> +        let icon = null;
> +        let marketApp = this.getDefaultAppForUri("market://search");
> +
> +        if (marketApp) {
> +           searchText += " " + marketApp.name;

Here, we should use the string bundle to format this:

https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIStringBundle#GetStringFromName%28%29

i.e.
bundle.formatStringFromName(protocol.toast.searchApp, [marketApp.name], 1);

and protocol.toast.searchApp = Search %S

@@ +82,5 @@
> +        if (marketApp) {
> +           searchText += " " + marketApp.name;
> +           icon = "-moz-icon://" + marketApp.packageName;
> +        } else {
> +          searchText = bundle.GetStringFromName("protocol.toast.searchMarket");

We shouldn't need to do this. We assign searchText up above. We can just leave it. I'd rather leave out the word "Market" as well.

@@ +103,5 @@
>        }
>      }
> +  },
> +
> +  getDefaultAppForUri: function getDefaultAppForUri(url) {

Ahh. I see, you're assuming if there's one app, its the default. I could buy that, but we should check the isDefault flags too. i.e. if the list is length == 1, return the item. Otherwise, if we find one with isDefault == true, return that.

@@ +119,5 @@
> +      return found;
> +    }
> +
> +    let apps = this._parseApps(JSON.parse(data));
> +      for (let i = 0; i < apps.length; i++) {

Indentation looks off here :) ?

@@ +122,5 @@
> +    let apps = this._parseApps(JSON.parse(data));
> +      for (let i = 0; i < apps.length; i++) {
> +        let appName = apps[i].name;
> +        if ( appName.length > 0 ) {
> +          found.push (apps[i]);

You might as well just return the app.name here. i.e.

if (apps.length == 1 && apps[0].length)
  return apps[0].length;

for (let i = 0; i < apps.length; i++) {
  if (apps[i].name && apps[i].isDefault)
    return apps[i].name;
}
return "";

Comment 18

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

(In reply to Ian Barlow (:ibarlow) from comment #14)
> (In reply to Wesley Johnston (:wesj) from comment #13)
> > Hmm.. the scope creeps. We originally designed this to model the gmail
> > toast, hence the icon placement. I wonder if we want to leave it in place
> > and allow users to pick between the two layouts? Or maybe just have an
> > "iconPosition" property and let you specify left, right, top, bottom.
> 
> Well, we could keep the icon on the left, like this maybe
> http://cl.ly/image/3q442o2p151Z
> 
> > #4 will be difficult... we're just using the icon that the market gives us.
> > We could try just putting a solid white tint on the imageView:
> > http://developer.android.com/reference/android/widget/ImageView.
> > html#attr_android:tint
> 
> I'd rather just use whatever we get then, but scale it down as per the mockup

Can we just use "Search" and drop the "Play Store" ? The icon does a great job letting me know where we are going. The Android app drawer uses the same style of icon in the top right corner with no text at all.

Comment 19

[Ian Barlow (:ibarlow)]

(In reply to Mark Finkle (:mfinkle) from comment #18)

> Can we just use "Search" and drop the "Play Store" ? The icon does a great
> job letting me know where we are going. The Android app drawer uses the same
> style of icon in the top right corner with no text at all.

I'm fine to drop it back to just say "Search" with the icon. But it absolutely has to say search -- I realize the app drawer only uses the icon, but almost no one I have ever talked to realized that was actually a link to the store ;)

Comment 20

[Errietta Kostala [:errietta]]

Attachment: Patch

Updated patch as discussed here. Wesj said to maybe use HelperApps to get the matching apps but I haven't got it to work yet so we can do that later.

Comment 21

[Errietta Kostala [:errietta]]

Attachment: Updated screenshot

Comment 22

[Wesley Johnston (:wesj)]

Comment on attachment 824193 [details] [diff] [review]
Patch

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

Back from vacation :)

This looks almost right. Just want one loop cleaned up a bit, and a question about what to do if we don't find any apps for market://search urls...

::: mobile/android/components/ContentDispatchChooser.js
@@ +7,5 @@
>  const Cc = Components.classes;
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
> +//Cu.import("resource://gre/modules/HelperApps.jsm");

Remove this if you don't need it. We should use this instead eventually, but we can file a separate bug for it :)

@@ +79,5 @@
> +        let marketApp = this.getDefaultAppForUri("market://search");
> +
> +        if (marketApp) {
> +           icon = "-moz-icon://" + marketApp;
> +        }

Remove the braces on a single line if (in javascript).

@@ +81,5 @@
> +        if (marketApp) {
> +           icon = "-moz-icon://" + marketApp;
> +        }
> +
> +        win.NativeWindow.toast.show(failedText, "long", {

Do we need a fallback if there is no marketApp?

@@ +111,5 @@
> +      className: ""
> +    };
> +    let data = sendMessageToJava(msg);
> +    if (!data) {
> +      return found;

return "";

@@ +116,5 @@
> +    }
> +
> +    let default_app = null;
> +    let apps = this._parseApps(JSON.parse(data));
> +    for (let i = 0; i < apps.length; i++) {

I think you can simplify this loop a bunch. Something like:

let found = null;
for (let app of apps) {
  if (!app.name)
    continue;

  if (app.isDefault)
    return app.packageName;
  else if (!found)
    found = app;
}
return found ? found.packageName : "";

Comment 23

[Errietta Kostala [:errietta]]

Attachment: patch

I think I fixed most details. Not sure what to do if marketApp is null.

Comment 24

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

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

>diff --git a/mobile/android/base/resources/values/styles.xml b/mobile/android/base/resources/values/styles.xml

>     <style name="ToastButton">
>         <item name="android:background">@drawable/toast_button</item>
>         <item name="android:textAppearance">?android:textAppearanceSmall</item>
>-        <item name="android:layout_width">wrap_content</item>
>+        <item name="android:layout_width">160dp</item>

How will this affect any other super toast? Will all "buttons" be 160dp wide even if they can be much smaller? That might make other toast look odd.

>diff --git a/mobile/android/components/ContentDispatchChooser.js b/mobile/android/components/ContentDispatchChooser.js

>   },
>-
>+  _parseApps: function _parseApps(aJSON) {

Don't remove the blank line. We want a blank line between methods.

>+    return apps;
>+  },
>   ask: function ask(aHandler, aWindowContext, aURI, aReason) {

Add a blank line. We want a blank line between methods.

>+  },
>+  getDefaultAppForUri: function getDefaultAppForUri(url) {

Same

Comment 25

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

Comment on attachment 824195 [details]
Updated screenshot

I wish the image was a smaller 1 color image. The toast looks a bit loop-sided. How strongly do we want the image?

I would be satisfied without the image. The "Search" label seems like enough to me.

Comment 26

[Errietta Kostala [:errietta]]

If you don't want the icon, I think https://bug910768.bugzilla.mozilla.org/attachment.cgi?id=814624 may still work.

Comment 27

[Wesley Johnston (:wesj)]

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

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

We've been over this design a few times. I'm fine with it as is for now. I'll r+, but we need a new patch with this one little fix in it :)

::: mobile/android/base/resources/values/styles.xml
@@ +533,5 @@
>  
>      <style name="ToastButton">
>          <item name="android:background">@drawable/toast_button</item>
>          <item name="android:textAppearance">?android:textAppearanceSmall</item>
> +        <item name="android:layout_width">160dp</item>

(In reply to Mark Finkle (:mfinkle) from comment #24)
> How will this affect any other super toast? Will all "buttons" be 160dp wide
> even if they can be much smaller? That might make other toast look odd.

Grr. You're right, I shoulda caught that. I think we probably just want a max-width here. http://developer.android.com/reference/android/widget/TextView.html#attr_android:maxWidth

::: mobile/android/components/ContentDispatchChooser.js
@@ +127,5 @@
> +      if (!found)
> +        found = appName;
> +    }
> +
> +    return found ? found : '';

Use double quotes :)

Comment 28

[Errietta Kostala [:errietta]]

Attachment: patch

Not sure I did the right thing with the max width there. I'm worried that

+        mButton.setMaxWidth(560);

Won't look the same in all devices.

Comment 29

[Wesley Johnston (:wesj)]

Yeah, we need to use device independent units. Does just chaning the layout_width to maxWidth not work?

http://developer.android.com/reference/android/widget/TextView.html#attr_android:maxWidth

Comment 30

[Errietta Kostala [:errietta]]

Attachment: patch

Using maxWidth in styles.xml now, but won't that affect other buttons too?

Comment 31

[Wesley Johnston (:wesj)]

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

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

Great! Thanks!

Comment 32

[Errietta Kostala [:errietta]]

If it's ok can you add checkin-needed or assign it to me? It's not assigned to me so i can't add it

Comment 33

[Wesley Johnston (:wesj)]

After a bunch of irc wrangling, decided to just remove the icon for now. That was pretty easy to do from this though. Landed:

https://hg.mozilla.org/integration/fx-team/rev/f0ccd1e58b7f

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f0ccd1e58b7f

Comment 35

[Francesco Lodolo [:flod]]

(In reply to Ian Barlow (:ibarlow) from comment #12)
> I think we can tweak a few things here: 
> 
> Mockup - http://cl.ly/image/453k1I1m3n3Q
> 1. Change "application" to "app"

When touching strings already landed on mozilla-central (i.e. exposed to the localization process), every string change requires a new ID.
https://developer.mozilla.org/en-US/docs/Making_String_Changes

Thia may sound impractical, but that's the only way to ensure that localizers see the change and take action.

This string change is a bit on the edge, so I'd like to know why you thought necessary to move from "application" to "app". 
If it's important that localizations reflect this change, we'll need a follow-up bug to assign the string a new ID.

Comment 36

[Wesley Johnston (:wesj)]

I missed it :)

But we were just trying to make this shorter I think. Localizations don't really need to update unless their strings are long, which is something they should probably be looking out for anyway.

We can bump the entitiy.

Comment 37

[Francesco Lodolo [:flod]]

(In reply to Wesley Johnston (:wesj) from comment #36)
> But we were just trying to make this shorter I think. Localizations don't
> really need to update unless their strings are long, which is something they
> should probably be looking out for anyway.

Ok, I'd say no need to change the entity name in this case. I'll send out a message to dev-l10n to be sure people are aware of this change.

Comment 38

[Tim Maks van den Broek [:mad_maks]]

In dutch are application and app different translations so the meaning of the entity is changed. i think it is needed to change the entity so all locales pick up this change.

Comment 39

[Francesco Lodolo [:flod]]

(In reply to Tim Maks van den Broek [:mad_maks] from comment #38)
> In dutch are application and app different translations so the meaning of
> the entity is changed. i think it is needed to change the entity so all
> locales pick up this change.

Sorry but I don't follow: is there a difference (meaning, not localization) between "app" and "application" for Dutch? AFAIK they should be synonyms, so if your locale is using "application" it should still be semantically and syntactically correct, it's just a matter of space.

Comment 40

[Tim Maks van den Broek [:mad_maks]]

Translation: 

application = toepassing ( like word or acrobat reader)
app = app (from a app store, like play an marketplace)

But you are right, the first translation was taken fom firefox for desktop and is not right for android (i think) where everything are apps.

forget my comment, we need a discussion in the translation bug for the dutch locale about this.