669510 - Create Home screen shortcuts from bookmarks using a shell shortcut launcher

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, enhancement, P2)

Description

[:Atoll]

Problem: Home screen shortcuts cannot be created from Aurora bookmarks. No option is provided in the context menu for bookmarks in the Bookmarks tab of Aurora. No option for Aurora Bookmarks is shown in the Home screen "Add to Home Screen" Shortcuts list (which does include items such as "Bookmark", "ConnectBot", and "Dropbox Folder").

Workaround: Manually copy bookmarks to the built-in Browser application bookmarks list and then add shortcuts to those.

Parity: The stock Browser can create home screen shortcuts from the bookmarks list, and lists Bookmark as an available shortcut in the "Select shortcut" dialog.

Device: Verizon Droid 2 (stock, unrooted)
Version: Mozilla/5.0 (Android; Linux armv7l; rv:6.0a2) Gecko/20110704 Firefox/6.0a2 Fennec/6.0a2

Comment 1

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

We recently landed code to add WebApp shortcuts on the homescreen. We can use the same type of code for bookmarks.

We do want a slightly different UI for picking the bookmarks. We'll work with UX on that.

Comment 2

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

This bug is about adding support to the Android shell. Bug 676293 is about adding an action within Fennec itself.

Comment 3

[Wesley Johnston (:wesj)]

You can now do this through the main Fennec UI. Do we still want further integration into the system?

Comment 4

[:Atoll]

Yes, please. Users are accustomed to adding widgets and shortcuts to the home screen from the home screen, I have at least six apps offering services through the Android shell.  We absolutely should present the Firefox bookmarks through that selection process as well.

Comment 5

[Jeff Vier [:jvier]]

(In reply to Wesley Johnston (:wesj) from comment #3)
> You can now do this through the main Fennec UI. Do we still want further
> integration into the system?

I agree with atoll.  DO WANT.

Comment 6

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

qawanted:

This should be fixed already (go to awesome page, long tap, select shortcut).
Please verify and close.

Comment 7

[Kevin Brosnan [Ex-Mozilla]]

That is not what Atoll was requesting. The comment 0 request is to provide some sort of Widget providing access to bookmarks.

Comment 8

[James Hugman [:jhugman] [@jhugman]]

This is implementing an activity with an IntentFilter for Intent.ACTION_PICK, so non-fennec callers (including the homescreen) can request a bookmark from fennec.

Comment 9

[Dominique Vincent [:domivinc]]

Attachment: patch_669510_1.diff

I started to work on this one.
I didn’t exactly implement the initial design. The new activity is not limited to the Firefox bookmarks list. All the links in the Firefox Home page can be added as an Android shortcut. The links can come from the Firefox bookmarks, or top sites list, or history or sync list …

Two main technical  benefits of this design change:
- I re-used the Firefox Home Page code,
- the impact in term of package size is limited.

The long press action to create a shortcut is not always available on the Android home screen. It depends of the Android launcher used by the device. 
In order to allow the same functionality for all the users, I also added a new menu “Shortcuts” in Settings -> Customize.

There are probably some UI changes in the header of the new activity. The user could be confused because in this new shortcuts activity your are not in the Firefox Home Page but the look is really the same. Only the header is different (no URL/Search input area). We could probably try to improve that.
In this shortcuts activity, I also deactivated the long press menu available in the Firefox home screen. The long press menu makes the same default action: it creates the Android shortcut.

I made my tests on a 2.3 device (HTC). It seems to work with the HTC Sense launcher.

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 8485991 [details] [diff] [review]
patch_669510_1.diff

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

This is really neat, and I think exposes some interesting coupling in our current DB/HomeProvider code. I'm going to suggest you ask lucasr for review, because he owns the home/ code pretty well and might have better ideas on how to make it more reusable.

::: mobile/android/base/BrowserApp.java
@@ +2944,5 @@
>      }
>  
>      // HomePager.OnUrlOpenListener
>      @Override
> +    public void onUrlOpen(String url, String title, EnumSet<OnUrlOpenListener.Flags> flags) {

Hmm.. Maybe this should be something more generally named. "OnItemClicked"?

::: mobile/android/base/GeckoAppShell.java
@@ +260,5 @@
>          }
>  
>          @Override
>          public void onFaviconLoaded(String pageUrl, String faviconURL, Bitmap favicon) {
> +            if (favicon != null){

Space after.

::: mobile/android/base/Shortcut.java
@@ +26,5 @@
> +import android.view.ViewStub;
> +import android.widget.Toast;
> +
> +public class Shortcut extends FragmentActivity 
> +implements HomePager.OnNewTabCreateShortcutListener,

Keep this on the same line as the extends bit, then you can indent the other two implements interfaces.

@@ +30,5 @@
> +implements HomePager.OnNewTabCreateShortcutListener,
> +            OnUrlOpenListener,
> +            ContextGetter 
> +{
> +    private static final String LOGTAG = "Shortcut";

We preface these classes with "Gecko" so that we can easily grep for all of them.

@@ +33,5 @@
> +{
> +    private static final String LOGTAG = "Shortcut";
> +
> +    private HomePager mHomePager;
> +    private ViewGroup mHomePagerContainer;

I don't think you use or need to keep either of these references.

@@ +42,5 @@
> +        // the app context, because various parts of Fennec (e.g.,
> +        // GeckoScreenOrientation) use GAS to access the Activity in
> +        // the guise of fetching a Context.
> +        // When that's fixed, `this` can change to
> +        // `(GeckoApplication) getApplication()` here.

We let our lines run pretty long. You don't need to wrap this much.

@@ +43,5 @@
> +        // GeckoScreenOrientation) use GAS to access the Activity in
> +        // the guise of fetching a Context.
> +        // When that's fixed, `this` can change to
> +        // `(GeckoApplication) getApplication()` here.
> +        GeckoAppShell.setContextGetter(this);

Do we need this? :(

@@ +68,5 @@
> +        BrowserDB.setSuggestedSites(suggestedSites);
> +        super.onCreate(bundle);
> +        setContentView(R.layout.shortcut);
> +        mHomePagerContainer = (ViewGroup) findViewById(R.id.home_pager_container);
> +        final ViewStub homePagerStub = (ViewStub) findViewById(R.id.home_pager_stub);

I think I would like this better if it looked like a "dialog". i.e. I wonder if we can open this view in an AlertDialog.Builder (or a DialogFragment) instead and leave the main view blank. We can probably reuse the ShareProvider's themeing code.

@@ +82,5 @@
> +        GeckoAppShell.setGeckoInterface(geckoInterface);
> +    }
> +
> +    public static GeckoAppShell.GeckoInterface getGeckoInterface() {
> +        return GeckoAppShell.getGeckoInterface();

You only use this once. I'd just inline it.

::: mobile/android/base/home/HomeFragment.java
@@ +118,5 @@
>          }
>  
> +        if (getActivity() instanceof Shortcut) {
> +            return;
> +        }

I would like to avoid special code like this if we can. We could create this HomePager and override its onCreateContextMenu() method in code. Or just a "setContextMenuEnabled(boolean)" method would work.

::: mobile/android/base/home/HomePager.java
@@ +86,5 @@
>          public void onNewTabs(List<String> urls);
>      }
>  
> +    public interface OnNewTabCreateShortcutListener extends OnNewTabsListener {
> +        public void onNewTab(String url, String title);

I would prefer to not add a new interface here.

::: mobile/android/base/home/RecentTabsPanel.java
@@ +151,5 @@
> +
> +                    final ArrayList<String> urls = new ArrayList<String>();
> +                    urls.add(c.getString(c.getColumnIndexOrThrow(RecentTabs.URL)));
> +
> +                    mNewTabsListener.onNewTabs(urls);

Not adding that interface might mean we have to handle this though.... Alternatively I guess we could add a "Mode" to the HomePager. i.e. if its set to "Picker", that would have to be accessible to views and they could avoid showing things that weren't "pickable" (i.e. this or "Clear History"). Alternatively, we could use something like ListView.ChoiceMode.SINGLE_CHOICE to say this HomePager should only support single choice actions. MUTLI_CHOICE isn't a great definition of the alternative that lets you do extra stuff (clear history, etc).

::: mobile/android/base/resources/layout/shortcut.xml
@@ +35,5 @@
> +	    <ViewStub android:id="@+id/home_banner_stub"
> +	              android:layout="@layout/home_banner"
> +	              android:layout_width="match_parent"
> +	              android:layout_height="@dimen/home_banner_height"
> +	              android:layout_gravity="bottom"/>

Does the HomePager just get mad if you don't have a home_banner on it? Thats sad :(

::: mobile/android/base/resources/xml-v11/preferences_customize.xml
@@ +21,5 @@
> +        <intent android:action="android.intent.action.VIEW"
> +                android:targetPackage="@string/android_package_name"
> +                android:targetClass="org.mozilla.gecko.Shortcut" >
> +        </intent>
> +    </PreferenceScreen>

All of this preference screen stuff I would probably cut, or do in a separate patch. We have ways to do this in the UI right now though, so I don't think we need it. In fact, I (personally) want to expand our current bookmark button using the ShareDialog to expose it a little better.

::: mobile/android/base/util/ThreadUtils.java
@@ +130,5 @@
>      }
>  
>      public static void assertNotOnGeckoThread() {
> +        if (sGeckoThread != null){
> +            assertNotOnThread(sGeckoThread, AssertBehavior.THROW);

I think you could just fix assertNotOnThread here. If the thread is null, there's a good chance you're not on it, so we can just return then.

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 8485991 [details] [diff] [review]
patch_669510_1.diff

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

Nice. I agree with all wesj's points here. Just to underline a couple of things:
- Avoid random 'instanceof' as they're dangerously easy to get wrong.
- We should really try to avoid checks like "Tabs.getInstance() != null". This is one the of most pervasive internal APIs in Fennec.

::: mobile/android/base/Shortcut.java
@@ +55,5 @@
> +        GeckoProfile profile = GeckoProfile.get(this).forceCreate();
> +        BrowserDB.initialize(profile.getName());
> +
> +        try {
> +            Favicons.attachToContext(this);

Can Favicons be attached to multiple contexts?

::: mobile/android/base/home/TopSitesPanel.java
@@ +249,5 @@
>          }
>  
> +        if (getActivity() instanceof Shortcut) {
> +            return;
> +        }

This looking a bit smelly. Maybe find a more general way of figuring out whether the host activity needs to handle context menus?

::: mobile/android/base/home/TwoLinePageRow.java
@@ +166,5 @@
>       * selected tab.
>       */
>      protected void updateDisplayedUrl() {
> +        if (Tabs.getInstance() != null &&
> +                Tabs.getInstance().getSelectedTab() != null){

Hmmm, there are *tons* of places in our codebase that just assumes Tabs.getInstance() is never null. It will be tough to ensure we're doing the right thing in all cases.

::: mobile/android/base/resources/xml-v11/preferences_customize.xml
@@ +21,5 @@
> +        <intent android:action="android.intent.action.VIEW"
> +                android:targetPackage="@string/android_package_name"
> +                android:targetClass="org.mozilla.gecko.Shortcut" >
> +        </intent>
> +    </PreferenceScreen>

I agree with wesj here: leave the pref changes out for now.

Comment 12

[Richard Newman [:rnewman]]

(In reply to Lucas Rocha (:lucasr) from comment #11)

> > +            Favicons.attachToContext(this);
> 
> Can Favicons be attached to multiple contexts?

attachToContext was renamed initializeWithContext in Bug 1064029. Calling it multiple times is safe; Favicons only uses it to get to resources, and no longer even holds a reference to the Context.

Sounds like something needs rebasing here...

Comment 13

[Dominique Vincent [:domivinc]]

Attachment: patch_669510_2.diff

Thanks for the code reviews! I made all the changes except for the followings:

1- Crash without the line:
GeckoAppShell.setContextGetter(this);

2- I don't understand what you want to implement here:

>I think I would like this better if it looked like a "dialog". i.e. I wonder 
>if we can open this view in an AlertDialog.Builder (or a DialogFragment) 
>instead and leave the main view blank. We can probably reuse the 
>ShareProvider's themeing code.

What is the advantage to display a dialog on top of an empty activity view?
What is the link with the Share functionality?


I removed the pref changes, but I really think that it's not a good idea from the UX point of view:

a) if the Android launcher doesn't allow the shortcut creation, the user won't be able to access the new Shortcut activity.
In this case, the Fennec functionality delivered to the end-user won't be the same. With the pref changes, all the users will get the same functionality.

b) if the end-user wants to create 10 shortcuts, with the current "Add to Home Screen" action, he must make 10 long press + 10 clicks on the "Add to Home Screen" menu option. 
To create 10 shortcuts using the new Shortcut activity, he just makes 10 clicks.
Why should we force the end-users to make more clicks than necessary?

Comment 14

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8492461 [details] [diff] [review]
patch_669510_2.diff

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

Flagging myself so I don't forget.  I'm pretty convinced this is not something we should do, even if it's technically possible; but I'll read to understand more.  This needs to land with about a billion tests.

Comment 15

[Wesley Johnston (:wesj)]

Comment on attachment 8492461 [details] [diff] [review]
patch_669510_2.diff

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

If you don't mind, this would be much easier to look at if we split it up a bit. For instance, the onUrlOpen -> onUrlClicked stuff isn't really necessary (except for the new parameter). Putting it in its own patch would make this easier to read, and let us not do it if we don't want. Likewise, I wouldn't mind putting the HomePagerMode stuff in its own patch as well. If you have mercuial's qcrecord extension set up (./mach mercurial-setup), its not very hard. just run:

hg qcref
// Select what you want in the first patch, then hit 'c' for commit
hg qnew newPatch
hg qcref
// Select stuff again, hit 'c'. Rinse and repeat.

I'm a little nervous about the "adding multiple items" stuff, and I still really would like to try putting this in a "dialog" BUT, I'm happy to debate that a bit later. Lets ping UX to at least let them know about this:
http://people.mozilla.org/~wjohnston/shortcuts.apk

I should mention, there's definite nervousness from people around reusing the HomePager like this.

::: mobile/android/base/Shortcut.java
@@ +24,5 @@
> +import android.view.ViewGroup;
> +import android.view.ViewStub;
> +import android.widget.Toast;
> +
> +public class Shortcut extends FragmentActivity implements HomePager.OnNewTabCreateShortcutListener,

Lets name this activity something with Activity in the name. ShortcutActivity?

@@ +37,5 @@
> +        // the app context, because various parts of Fennec (e.g.,
> +        // GeckoScreenOrientation) use GAS to access the Activity in
> +        // the guise of fetching a Context.
> +        // When that's fixed, `this` can change to
> +        // `(GeckoApplication) getApplication()` here.

Don't wrap these comments so short. I'd be fine with anything from 100-120 characters.

@@ +44,5 @@
> +        // has not already been set
> +        if (GeckoAppShell.getGeckoInterface() == null) {
> +            setGeckoInterface(new BaseGeckoInterface(this));
> +        }
> +        ThreadUtils.setUiThread(Thread.currentThread(), new Handler());

This doesn't build anymore. I think we do this automatically now. Yay! I ran into another issue with some recent changes to lightweight themes. i.e. getLightweightTheme returns null here:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/ThemedView.java.frag#35

which causes some failures below. We could protect them, or fix lightweight theme support here (call prepareLightweightTheme like we do here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#532 )

@@ +65,5 @@
> +        setContentView(R.layout.shortcut);
> +
> +        HomePager mHomePager;
> +        ViewGroup mHomePagerContainer;
> +        mHomePagerContainer = (ViewGroup) findViewById(R.id.home_pager_container);

Do the declaration and initialization on one line. Also, make everything final if you can.

@@ +67,5 @@
> +        HomePager mHomePager;
> +        ViewGroup mHomePagerContainer;
> +        mHomePagerContainer = (ViewGroup) findViewById(R.id.home_pager_container);
> +        final ViewStub homePagerStub = (ViewStub) findViewById(R.id.home_pager_stub);
> +        mHomePager = (HomePager) homePagerStub.inflate();

Same for this. move the declaration down here.

@@ +72,5 @@
> +
> +        mHomePagerContainer.setVisibility(View.VISIBLE);
> +        mHomePager.setContextMenuEnabled(HomePager.HOMEPAGER_DISABLE_CONTEXT_MENU |
> +                HomePager.HOMEPAGER_PICKER | HomePager.HOMEPAGER_HIDE_OPEN_ALL_BUTTONS |
> +                HomePager.HOMEPAGER_HIDE_CLEAR_BROWSING_HISTORY_BUTTON);

Lots of flags!

@@ +83,5 @@
> +        GeckoAppShell.setGeckoInterface(geckoInterface);
> +    }
> +
> +    public void createShortcut(String url, String title) {
> +        if (url != null && title != null) {

Early return here.

::: mobile/android/base/home/HistoryPanel.java
@@ +193,5 @@
>      }
>  
>      private void updateUiFromCursor(Cursor c) {
> +        if (c != null && c.getCount() > 0){
> +            if ((getHomePagerMode() & HomePager.HOMEPAGER_HIDE_CLEAR_BROWSING_HISTORY_BUTTON) == 0) {

Hmm.. Interesting. I wonder if we'd just be better to check context instanceof GeckoApp or something.

::: mobile/android/base/home/HomeFragment.java
@@ +61,5 @@
>  
>      // Default value for "can load" hint
>      static final boolean DEFAULT_CAN_LOAD_HINT = false;
>  
> +    static final int DEFAULT_HOME_PAGER_MODE = 0;

Do you need this constant in here? Can use reuse the other?

@@ +68,5 @@
>      // This is used to defer data loading until the editing
>      // mode animation ends.
>      private boolean mCanLoadHint;
>  
> +    private int mHomePagerMode = 0;

If this is a bitmask, I'd rather call it "mFlags". You can default it to DEFAULT_HOME_PAGER_MODE here (or better, reuse the one from HomePager.DEFAULT_MODE;

::: mobile/android/base/home/HomePager.java
@@ +66,5 @@
> +    private int mHomePagerMode = 0;
> +    static public final int HOMEPAGER_DISABLE_CONTEXT_MENU = 0x00000001;
> +    static public final int HOMEPAGER_PICKER = 0x00000010;
> +    public static final int HOMEPAGER_HIDE_OPEN_ALL_BUTTONS = 0x00000100;
> +    public static final int HOMEPAGER_HIDE_CLEAR_BROWSING_HISTORY_BUTTON = 0x00001000;

These are in hex, not binary, so be careful. i.e. I think you want 0x01, 0x02, 0x04, 0x08, etc.

@@ +539,5 @@
>              mCurrentPanelSessionSuffix = null;
>          }
>      }
> +
> +    public void setContextMenuEnabled(int b) {

This should probably take a boolean and have something like:

mHomePagerMode |= ENABLE_CONTEXT_MENUS;

But you should just rename this setFlags() too.

::: mobile/android/base/home/RecentTabsPanel.java
@@ +142,5 @@
>                  if (c == null || !c.moveToPosition(position)) {
>                      return;
>                  }
>  
>                  final int itemType = c.getInt(c.getColumnIndexOrThrow(RecentTabs.TYPE));

This can go in the if

@@ +144,5 @@
>                  }
>  
>                  final int itemType = c.getInt(c.getColumnIndexOrThrow(RecentTabs.TYPE));
>  
> +                if ((mHomePagerMode & HomePager.HOMEPAGER_PICKER) != 0) {

Do you need this if(mHomePagerMode) check?

@@ +165,5 @@
> +
> +                    final ArrayList<String> urls = new ArrayList<String>();
> +                    urls.add(c.getString(c.getColumnIndexOrThrow(RecentTabs.URL)));
> +
> +                    mNewTabsListener.onNewTabs(urls);

Hmm.. This whole interface is just because you can tap a row that includes more than one url... I talked to nalexander and he says he thinks we can change this to just call mUrlClickedListener when its just a single row. Then we don't need this entire HOMEPAGER_PICKER flag that I don't really like.

::: mobile/android/base/home/TwoLinePageRow.java
@@ +195,5 @@
>       * Only looks for tabs that are either private or non-private, depending on the current
>       * selected tab.
>       */
>      protected void updateDisplayedUrl() {
> +        if (Tabs.getInstance().getSelectedTab() != null){

I'd probably just early return here. if (selectedTab == null) return;

::: mobile/android/base/resources/layout/shortcut.xml
@@ +11,5 @@
> +    android:layout_height="match_parent"
> +    android:orientation="vertical" >
> +
> +    <TextView
> +        android:id="@+id/shortcut_header"

Instead of this header (since I'm not getting my dialog :)) I wonder if we can put this in the actionbar. For backwards compat, we'd need to use ActionBarActivity:

http://developer.android.com/reference/android/support/v7/app/ActionBarActivity.html

but I think we ship enough of the v7 library to do that anyway. Then you'd just set the title of the activity to shortcut_header and remove this. It would actually be nice to add a "Done" button up there as well (although I think Android is schizophrenic with whether Done buttons are shown at the top or bottom of the screen).

::: mobile/android/base/util/ThreadUtils.java
@@ +232,5 @@
>       * Resets the priority of a thread whose priority has been reduced
>       * by reduceGeckoPriority.
>       */
>      public static void resetGeckoPriority() {
> +        if (sIsGeckoPriorityReduced && sGeckoThread!= null) {

Heh. Interesting. TopSites shouldn't be doing this I guess. In fact, since it may not be the default, it really shouldn't do this at all. You mind filing a bug on that?

Comment 16

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Wesj pinged me for UX feedback on this. Could someone please film and post a quick video of how the shortcut creation works? It's a little difficult to read just words and picture how things are interacting. Thanks!

Comment 17

[Wesley Johnston (:wesj)]

Sure! A quick run through video. There's a build linked above as well:

http://people.mozilla.org/~wjohnston/demo.mkv

Comment 18

[Dominique Vincent [:domivinc]]

Wesj, thanks for the second code review. 
I don't know what is the next step here: we should probably wait for a decision regarding the reusing of the HomePager? It's useless to make your code changes if the final decision is not to go that way.
How long will it take for a decision to be made on such topic? Who will make the decision?

Comment 19

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8492461 [details] [diff] [review]
patch_669510_2.diff

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

A year ago, I looked at this, and wrote:

"Flagging myself so I don't forget.  I'm pretty convinced this is not something we should do, even if it's technically possible; but I'll read to understand more.  This needs to land with about a billion tests."

I don't think it landed, so I'm just going to unflag myself.

Comment 20

[:Margaret Leibovic]

Reading bug comments, I'm not totally sure what the proposed UX is here. Is this still something we want to do?

Comment 21

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

Given that I only see the "Long press on Android Homescreen to add a Shortcut" feature on Gingerbread, I don't think we want to move ahead with this feature. Dominique did add a way to access from Settings > Customize, but that doesn't seem much different our current "Add to Home Screen" feature.

There might be reasons we want to be able to re-parent the Home Panels in a different Activity, but let's come up with new use cases.