711905 - Support adding links to homescreen

Status: VERIFIED FIXED

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

Description

[Wesley Johnston (:wesj)]

XUL Fennec supported adding bookmarks and links to your homescreen. Native should too!

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Patch adds "Open in new tab" and a "Add to homescreen" for shortcuts on the awesomescreen.

Comment 2

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

I guess I can | hg qdel | my half done patch :)

Comment 3

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

Comment on attachment 582742 [details] [diff] [review]
Patch v1

> 
>+    private Cursor mContextMenuCursor = null;
>+    @Override
>+    public void onCreateContextMenu(ContextMenu menu, View view, ContextMenuInfo menuInfo) {

Add a blank line to separate mmContextMenuCursor from @Override

>+        super.onCreateContextMenu(menu, view, menuInfo);
>+
>+        AdapterView.AdapterContextMenuInfo info = (AdapterView.AdapterContextMenuInfo) menuInfo;
>+        ListView list = (ListView) view;
>+        Object selecteditem = list.getItemAtPosition(info.position);
>+
>+        if (!(selecteditem instanceof Cursor)) {

Can we reset mContextMenuCursor to null here? In case we return early.

>+    @Override
>+    public boolean onContextItemSelected(MenuItem item) {
>+        AdapterContextMenuInfo info = (AdapterContextMenuInfo) item.getMenuInfo();
>+

Check mContextMenuCursor and if it's null, bail

>+        switch (item.getItemId()) {
>+            case R.id.open: {

R.id.contextmenu_open_new_tab

>+                String url = mContextMenuCursor.getString(mContextMenuCursor.getColumnIndexOrThrow(URLColumns.URL));
>+                GeckoApp.mAppContext.loadUrl(url, AwesomeBar.Type.ADD);

Should we be opening in the background? Can we do that yet? Matt might have a patch to do it. (just thinking out loud)

>+            case R.id.addtolauncher: {

R.id.contextmenu_add_to_launcher

>+                Bitmap bitmap = null;
>+                if (b != null) {
>+                    bitmap = BitmapFactory.decodeByteArray(b, 0, b.length);
>+                }

No {} needed in this file (I think)


Madhava would also like to add "Share" (R.id.contextmenu_share) which will send the title and URL to GeckoAppShell helper for sharing.

>diff --git a/mobile/android/base/GeckoAppShell.java b/mobile/android/base/GeckoAppShell.java

>+    public static void createShortcut(String aTitle, String aURI, Bitmap aIcon, String aType) {
>+        new CreateShortcutTask().execute(new Shortcut(aTitle, aURI, aIcon, aType));
>+    }
>+
>+    private static class CreateShortcutTask extends AsyncTask<Shortcut, Void, Void> {

AsyncTask is probably wrong for two reasons:
* Since you don't need to impl the onPostExecute, you could probably just use GeckoAppShell.mHandler.post( Runnable );
* We use GeckoAsyncTask to make sure everything is on the right thread. If you do need AsyncTask, use GeckoAsyncTask instead.

>+    static private Bitmap getIcon(Bitmap aSource) {

>+        paint.setColor(Color.HSVToColor(hsv));
>+        canvas.drawRoundRect(new RectF(kOffset,
>+                                       kOffset,
>+                                       kIconSize-kOffset,
>+                                       kIconSize-kOffset), 5, 5, paint);

kIconSize - kOffset

What are the | 5 | about? Margins? Add a constant?

>+        // draw the bitmap
>+        if (aSource == null) {
>+            aSource = BitmapFactory.decodeResource(GeckoApp.mAppContext.getResources(), R.drawable.home_star);
>+        }

Drop the {} in this file (I think)

>diff --git a/mobile/android/base/gfx/BitmapUtils.java b/mobile/android/base/gfx/BitmapUtils.java

>+            if (colors[h] > colors[maxH]) {
>+              maxH = h;
>+            }
>+            if (sat[s] > sat[maxS]) {
>+              maxS = s;
>+            }
>+            if (val[v] > val[maxV]) {
>+              maxV = v;
>+            }

Drop {}

>+            Log.i("BitmapUtils", "h = " + maxH + ", s = " + maxS + ", v = " + maxV);

Add a LOGTAG constant to the class (See other java files for example)

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd

>+<!ENTITY contextmenu_open "Open in new tab">

contextmenu_open_new_tab "Open in New Tab"

>+<!ENTITY contextmenu_addtolauncher "Add to homescreen">

contextmenu_add_to_launcher "Add to Home Screen" (what we used in XUL)

>diff --git a/mobile/android/base/resources/menu/awesomebar_contextmenu.xml b/mobile/android/base/resources/menu/awesomebar_contextmenu.xml

>+    <item android:id="@+id/open"
>+          android:title="@string/contextmenu_open"/>
>+
>+    <item android:id="@+id/addtolauncher"
>+          android:title="@string/contextmenu_addtolauncher"/>
>+

* Add "Share" (Don't reuse the existing id and string entity since l10n might have a different context)
* Use new id and string entity names

>diff --git a/mobile/android/base/strings.xml.in b/mobile/android/base/strings.xml.in

>+  <string name="contextmenu_open">&contextmenu_open;</string>
>+  <string name="contextmenu_addtolauncher">&contextmenu_addtolauncher;</string>

Here too

r-, but close

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Fixed up and tweaked in a few bits. Screenshot (arranged in a pretty rainbow) at:

http://dl.dropbox.com/u/72157/icons.png

Comment 5

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

Comment on attachment 582974 [details] [diff] [review]
Patch v2

>diff --git a/mobile/android/base/AwesomeBar.java b/mobile/android/base/AwesomeBar.java
>+    public boolean onContextItemSelected(MenuItem item) {
>+        if (mContextMenuCursor == null) {
>+            return false;
>+        }
>+        AdapterContextMenuInfo info = (AdapterContextMenuInfo) item.getMenuInfo();

Drop {} but leave a blank line after | return false; |

>+            case R.id.share: {
>+                String url = mContextMenuCursor.getString(mContextMenuCursor.getColumnIndexOrThrow(URLColumns.URL));
>+                String title = mContextMenuCursor.getString(mContextMenuCursor.getColumnIndexOrThrow(URLColumns.TITLE));
>+                GeckoAppShell.openUriExternal(url, "text/plain", "", "",
>+                                              Intent.ACTION_SEND, title);

Does this need wrapped?

>diff --git a/mobile/android/base/GeckoAppShell.java b/mobile/android/base/GeckoAppShell.java
>+    static private Bitmap getIcon(Bitmap aSource) {

Since this is a method on GeckoAppShell, getIcon sounds too gneral - how about getLauncherIcon ?

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd

>+<!ENTITY contextmenu_open_new_tab "Open in new tab">

"Open in New Tab"

r+ with the nits fixed

Comment 6

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

Made nits and pushed
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2bb564589fc

Comment 7

[Ed Morley [:emorley]]

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

Comment 8

[Aaron Train [:aaronmt]]

Aurora nom?

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 582974 [details] [diff] [review]
Patch v2

I think we should probably take this on Aurora. It a feature regression from XUL Fennc. Has been on nightlies for awhile now. All known regression bugs have been cleaned up (will have to also land the patch in bug 712627 and bug 714975, I'll nom them as well).
Fennec only. Low risk.

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 582974 [details] [diff] [review]
Patch v2

Oh wait. This already is on Aurora. Heh.

Comment 11

[Carla Nadastean]

Verified with:
12.0a1 (2012-01-11) Device: LG Optimus 2X Android(2.2)
11.0a2 (2012-01-11) Device: LG Optimus 2X Android(2.2)

Links can be added to home screen from Awesome menu. (After long tap on the link, a context menu with "Add to Home Screen" option is displayed.)