667530 - Add ability to add application/bookmark shortcuts to Launcher screens

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Attachment: WiP Patch

Talked to mfinkle about this briefly the other day and wanted to put a WIP up. Bug 631717 is about creating a widget for us to show/launch currently installed "webapps" through. Alternative, we can use the ability to put shortcuts in the launcher itself. This WIP starts that, but is a lot of copy and paste from around the world (i.e. needs cleanup). Also doesn't set icons correctly. I'm looking into that problem. We could potentially make this a little more robust and also use it to add desktop bookmarks.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Screenshots

This is just some screenshots of the flow, for UX to look at.

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Updated WIP

This adds icons in the app listview, as well as fixing the icons I'm pushing to desktop. Needs testing and sanity checks all over the place (i.e. don't fail if there's no sqlite db yet).

Comment 3

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

Attachment: patch

This patch builds on Wes' patches. The favicon is used in the shortcut list and on the home screen. Wes' previous WIP wasn't quite right. I also removed the unneeded activity-alias from AndroidManifest.xml.

Strings were added. The dpiDensity is used to specify the shortcut favicon size.

Comment 4

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

Attachment: in shortcut list

The item in the Android shortcut list

Comment 5

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

Attachment: list of web apps

The list of web apps

Comment 6

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

Attachment: shortcuts ot webapps on a homescreen

Favicons and text used to create the shortcuts on the homescreen

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

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

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

::: embedding/android/LauncherShortcuts.java.in
@@ +64,5 @@
> +        private Cursor _cursor;
> +        private Context _context;
> +    
> +        public LauncherCursorAdapter(Context context, int layout, Cursor c, String[] from, int[] to) {
> +            super(context, layout, c, from, to);

this constructor is deprecated, please use the non-deprecated version

@@ +94,5 @@
> +                if (files[i].endsWith(".default"))
> +                    profilePath = files[i];
> +            }
> +        }
> +        Log.i("LauncherShortcuts", "Opening: " + basePath + "/" + profilePath + "/webapps.sqlite");

why are you going through this effort to get the profile path? It might be better to just put this in the files dir
Is it possible for this code to be called before the app is launched for the first time (i.e. there is no profile)? What is the behavior in that case?

Also, this sqlite db is only called from java code, right?

@@ +95,5 @@
> +                    profilePath = files[i];
> +            }
> +        }
> +        Log.i("LauncherShortcuts", "Opening: " + basePath + "/" + profilePath + "/webapps.sqlite");
> +        mDb = SQLiteDatabase.openDatabase(basePath + "/" + profilePath + "/webapps.sqlite", null, SQLiteDatabase.OPEN_READONLY | SQLiteDatabase.NO_LOCALIZED_COLLATORS);

use the File object to construct the path with new File(parent, node) rather than this string manipulation (note that you can use File.listFiles above).

::: embedding/android/Makefile.in
@@ +122,5 @@
>    $(NULL)
>  
>  RES_VALUES = res/values/colors.xml res/values/themes.xml
>  
> +AB_CD = $(shell echo $(AB_CD) | sed -e s/-/-r/)

why are you changing this?

::: embedding/android/locales/en-US/android_strings.dtd
@@ +16,5 @@
>  <!ENTITY  sending_crash_report "Sending crash report\u2026">
>  <!ENTITY  exit_label "Exit">
>  <!ENTITY  continue_label "Continue">
> +
> +<!ENTITY  launcher_shortcuts_title "&brandShortName; Web Apps">

don't need the blank space

::: embedding/android/strings.xml.in
@@ +21,5 @@
>    <string name="sending_crash_report">&sending_crash_report;</string>
>    <string name="exit_label">&exit_label;</string>
>    <string name="continue_label">&continue_label;</string>
> +
> +  <string name="launcher_shortcuts_title">&launcher_shortcuts_title;</string>

don't need the blank space

Comment 8

[[:fabrice] Fabrice Desré]

> 
> why are you going through this effort to get the profile path? It might be
> better to just put this in the files dir
> Is it possible for this code to be called before the app is launched for the
> first time (i.e. there is no profile)? What is the behavior in that case?

We should gracefully fail in this case.

> Also, this sqlite db is only called from java code, right?

No, it's created and managed from within fennec (see bug 663571), and is not android specific

Comment 9

[[:fabrice] Fabrice Desré]

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

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

Waiting for blassey comments to be addressed, and fixing the "use before first run" case.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #8)
> > Also, this sqlite db is only called from java code, right?
> 
> No, it's created and managed from within fennec (see bug 663571), and is not
> android specific
Marco, is it going to be problematic to create and modify the db with our version of sqlite and read it with the system version?

Comment 11

[Marco Bonardo [:mak]]

(In reply to comment #10)
> Marco, is it going to be problematic to create and modify the db with our
> version of sqlite and read it with the system version?

You can check file format compatibility here: http://www.sqlite.org/formatchng.html
If the database is simple and doesn't use fancy features (WAL or foreign keys) it is probably supported by any version > 3.3.0.

Comment 12

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

(In reply to comment #11)
> (In reply to comment #10)
> > Marco, is it going to be problematic to create and modify the db with our
> > version of sqlite and read it with the system version?
> 
> You can check file format compatibility here:
> http://www.sqlite.org/formatchng.html
> If the database is simple and doesn't use fancy features (WAL or foreign
> keys) it is probably supported by any version > 3.3.0.

No fancy features are used. It's a simple table.

Comment 13

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

(In reply to comment #7)

> ::: embedding/android/LauncherShortcuts.java.in
> > +        public LauncherCursorAdapter(Context context, int layout, Cursor c, String[] from, int[] to) {
> > +            super(context, layout, c, from, to);
> 
> this constructor is deprecated, please use the non-deprecated version

Only deprecated for Honeycomb. I added a comment and kept the current code

> > +                if (files[i].endsWith(".default"))
> > +                    profilePath = files[i];
> > +            }
> > +        }
> > +        Log.i("LauncherShortcuts", "Opening: " + basePath + "/" + profilePath + "/webapps.sqlite");
> 
> why are you going through this effort to get the profile path? It might be
> better to just put this in the files dir
> Is it possible for this code to be called before the app is launched for the
> first time (i.e. there is no profile)? What is the behavior in that case?

The sqlite DB is managed from Fennec and lives in the profile. If the profile does not exist or the DB does not exist, we used to crash. Now we show an empty list.

> Also, this sqlite db is only called from java code, right?

Readonly from Java. Managed from Fennec. Based on Marco's comments, we should be fine for compat.

> > +                    profilePath = files[i];
> > +            }
> > +        }
> > +        Log.i("LauncherShortcuts", "Opening: " + basePath + "/" + profilePath + "/webapps.sqlite");
> > +        mDb = SQLiteDatabase.openDatabase(basePath + "/" + profilePath + "/webapps.sqlite", null, SQLiteDatabase.OPEN_READONLY | SQLiteDatabase.NO_LOCALIZED_COLLATORS);
> 
> use the File object to construct the path with new File(parent, node) rather
> than this string manipulation (note that you can use File.listFiles above).

Switched to File objects, but kept File.list since File.listFiles didn't gain me anything.

> ::: embedding/android/Makefile.in
> @@ +122,5 @@
> >    $(NULL)
> >  
> >  RES_VALUES = res/values/colors.xml res/values/themes.xml
> >  
> > +AB_CD = $(shell echo $(AB_CD) | sed -e s/-/-r/)
> 
> why are you changing this?

It was in Wes' original patch. the AB_rCD seemed weird to me as well. Things seem to work with Wes' change.

> ::: embedding/android/locales/en-US/android_strings.dtd
> @@ +16,5 @@
> >  <!ENTITY  sending_crash_report "Sending crash report\u2026">
> >  <!ENTITY  exit_label "Exit">
> >  <!ENTITY  continue_label "Continue">
> > +
> > +<!ENTITY  launcher_shortcuts_title "&brandShortName; Web Apps">
> 
> don't need the blank space
> 
> ::: embedding/android/strings.xml.in
> @@ +21,5 @@
> >    <string name="sending_crash_report">&sending_crash_report;</string>
> >    <string name="exit_label">&exit_label;</string>
> >    <string name="continue_label">&continue_label;</string>
> > +
> > +  <string name="launcher_shortcuts_title">&launcher_shortcuts_title;</string>
> 
> don't need the blank space

The file already has blank lines to separate the splash screen and the crash reporter strings, so I kept the lines to separate the launcher strings

Comment 14

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

Attachment: patch 2

Updated based on comments

Comment 15

[[:fabrice] Fabrice Desré]

Comment on attachment 544405 [details] [diff] [review]
patch 2

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

r+ if we're sure about the AB_CD change

::: embedding/android/Makefile.in
@@ +122,5 @@
>    $(NULL)
>  
>  RES_VALUES = res/values/colors.xml res/values/themes.xml
>  
> +AB_CD = $(shell echo $(AB_CD) | sed -e s/-/-r/)

AB_rCD is used for non en-US builds, so I'm not so sure this change is safe.

See http://mxr.mozilla.org/mozilla-central/search?string=ab_rcd

Comment 16

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

(In reply to comment #15)
> Comment on attachment 544405 [details] [diff] [review] [review]
> patch 2
> 
> Review of attachment 544405 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> r+ if we're sure about the AB_CD change
> 
> ::: embedding/android/Makefile.in
> @@ +122,5 @@
> >    $(NULL)
> >  
> >  RES_VALUES = res/values/colors.xml res/values/themes.xml
> >  
> > +AB_CD = $(shell echo $(AB_CD) | sed -e s/-/-r/)
> 
> AB_rCD is used for non en-US builds, so I'm not so sure this change is safe.
> 
> See http://mxr.mozilla.org/mozilla-central/search?string=ab_rcd

Yeah, I am backing the AB_CD change out.

Comment 17

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

Attachment: patch 3

Found out that I had to update other occurrences of "org.mozilla.fennec.WEBAPP" to "org.mozilla.gecko.WEBAPP" for this patch to work. I also backed out the AB_CD change to AB_rCD.

Those the only changes from the one Fabrice r+, so only asking Brad for review on this one.

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 544493 [details] [diff] [review]
patch 3

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

::: embedding/android/locales/en-US/android_strings.dtd
@@ +17,5 @@
>  <!ENTITY  exit_label "Exit">
>  <!ENTITY  continue_label "Continue">
> +
> +<!ENTITY  launcher_shortcuts_title "&brandShortName; Web Apps">
> +<!ENTITY  launcher_shortcuts_empty "No web apps were found">

nit remove the blank line

::: embedding/android/strings.xml.in
@@ +22,5 @@
>    <string name="exit_label">&exit_label;</string>
>    <string name="continue_label">&continue_label;</string>
> +
> +  <string name="launcher_shortcuts_title">&launcher_shortcuts_title;</string>
> +  <string name="launcher_shortcuts_empty">&launcher_shortcuts_empty;</string>

nit, remove the blank line

Comment 19

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/89ef5bf1e3d2

Comment 20

[Daniel Holbert [:dholbert]]

Backed out because this push turned Android talos perma-orange (in a way that can be caused by bugs in code, according to aki):
http://hg.mozilla.org/integration/mozilla-inbound/rev/bc91728e5a7c

Comment 21

[Daniel Holbert [:dholbert]]

(looks like the push turned all unittests orange, actually - talos was just the first to finish. http://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=89ef5bf1e3d2 )

Comment 22

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

The failures were caused by the way the test automation tries to start Fennec. It attempts to launch the first <activity> in the AndroidManifest.xml file. The new LauncherShortcuts activity was the first activity. Things failed.

I moved the activity to the end of the list. This passed locale Android testing, after we reproduced the failures.

http://hg.mozilla.org/integration/mozilla-inbound/rev/22d8bb992dc2

Comment 23

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/22d8bb992dc2

Comment 24

[Anna (Waverley)]

Steps to reproduce:

1. Open fennec
2. Visit www.cnn.com
3. Tap on the favicon 
4. Choose Install as App
5. Press home button to bring fennec in the background.
6. Long tap on the home screen
7. From the context menu choose Shortcut -> Nightly Web Apps -> Cnn

The shortcut will appear on the home screen. Tapping on it will load the cnn webpage.

I will this bug as VERIFIED FIXED on:

Build ID: Mozilla /5.0 (Android;Linux armv7l;rv:8.0a1) Gecko/20110713 Firefox/8.0a1 Fennec/8.0a1  

Devices: HTC Desire Z (Android 2.2), Google Nexus S (Android 2.3)