Closed Bug 711905 Opened 8 years ago Closed 8 years ago

Support adding links to homescreen

Categories

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

All
Android
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: wesj, Assigned: wesj)

References

Details

(Whiteboard: [QA+])

Attachments

(1 file, 1 obsolete file)

XUL Fennec supported adding bookmarks and links to your homescreen. Native should too!
Attached patch Patch v1 (obsolete) — Splinter Review
Patch adds "Open in new tab" and a "Add to homescreen" for shortcuts on the awesomescreen.
Assignee: nobody → wjohnston
Attachment #582742 - Flags: review?(mark.finkle)
I guess I can | hg qdel | my half done patch :)
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
Attachment #582742 - Flags: review?(mark.finkle) → review-
Attached patch Patch v2Splinter Review
Fixed up and tweaked in a few bits. Screenshot (arranged in a pretty rainbow) at:

http://dl.dropbox.com/u/72157/icons.png
Attachment #582742 - Attachment is obsolete: true
Attachment #582974 - Flags: review?(mark.finkle)
Priority: -- → P4
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
Attachment #582974 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/d2bb564589fc
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 712370
Depends on: 714092
Flags: in-litmus+
Whiteboard: [QA+]
No longer depends on: 714092
Aurora nom?
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.
Attachment #582974 - Flags: approval-mozilla-aurora?
Comment on attachment 582974 [details] [diff] [review]
Patch v2

Oh wait. This already is on Aurora. Heh.
Attachment #582974 - Flags: approval-mozilla-aurora?
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.)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.