Add ability to remove shortcuts

REOPENED
Assigned to

Status

()

REOPENED
6 years ago
2 years ago

People

(Reporter: wesj, Assigned: vlad)

Tracking

Trunk
Firefox 16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 629368 [details]
Patch

I was digging through some code the other day and saw that there's an Intent for removing homescreen short cuts. We should use it! It basically matches in url (or the intent the shortcut is associated with) and name of the shortcut.

I tried to hook this up, but apps don't provide us with the name of the app after it is uninstalled. So I'm hoping to loop some of them in to see if they can help?
http://code.google.com/p/android-launcher-plus/source/browse/trunk/src/mobi/intuitit/android/p/launcher/UninstallShortcutReceiver.java?r=4#40

Looks like it will remove all duplicates by default, which is a good thing.

From the docs & posts I have seen online, the shortcutIntent needs to match the one used to create the shortcut. Looks like you missed adding the setAction part:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShell.java#785

Not sure f this is really needed or not.
(Reporter)

Updated

6 years ago
Blocks: 761048
(In reply to Wesley Johnston (:wesj) from comment #0)
> Created attachment 629368 [details]
> Patch
> 
> I was digging through some code the other day and saw that there's an Intent
> for removing homescreen short cuts. We should use it! It basically matches
> in url (or the intent the shortcut is associated with) and name of the
> shortcut.
> 
> I tried to hook this up, but apps don't provide us with the name of the app
> after it is uninstalled. So I'm hoping to loop some of them in to see if
> they can help?

During an uninstall of a specific app, wouldn't you already be able to figure that information out (the name of the app and the origin + launch path of the app)? I would think you could get that information off the app object.

https://developer.mozilla.org/en/DOM/App
(Reporter)

Comment 3

6 years ago
We have info when WE are uninstalling the app (i.e. about:apps). Is that the only place where un-installation can happen? I looked through http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js briefly to try and find a notification we could receive when apps were uninstalled, but I may have missed something.
Wes - Can't you listen for "webapps-sync-uninstall"? We already listen for "webapps-sync-install".
(Reporter)

Comment 5

6 years ago
Yeah. That's what I tried. For install we have to get the manifest by using:

DOMApplicationRegistry.getManifestFor(data.origin, callback);

For uninstall this will return null to the callback (i.e. the manifest is gone by this point).
(Reporter)

Comment 6

6 years ago
Created attachment 630387 [details] [diff] [review]
Patch

Might as well review these bits for now. Added back in the type stuff.
Assignee: nobody → wjohnston
Attachment #629368 - Attachment is obsolete: true
Attachment #630387 - Flags: review?(mark.finkle)
Comment on attachment 630387 [details] [diff] [review]
Patch

This looks OK, but we should hook it up to about:apps so we can actually test it out.

We can worry about Apps In The Cloud uninstalling apps later. And we can make sure a pre-uninstall notification is fired too.
Attachment #630387 - Flags: review?(mark.finkle) → review+
Also note that Brubeck is killing nsIShellService. I think he'll be able to unbitrot his patch without too much trouble.
(In reply to Mark Finkle (:mfinkle) from comment #8)
> Also note that Brubeck is killing nsIShellService. I think he'll be able to
> unbitrot his patch without too much trouble.

If you're curious, that change is in attachment 622564 [details] [diff] [review] from bug 753574.  It replaces the nsIShellService method with a JSON message.
(In reply to Mark Finkle (:mfinkle) from comment #7)
> Comment on attachment 630387 [details] [diff] [review]
> Patch
> 
> This looks OK, but we should hook it up to about:apps so we can actually
> test it out.

Could you file a bug for this?
(Reporter)

Comment 11

6 years ago
Created attachment 630648 [details] [diff] [review]
Without shell service

Oh. Didn't notice this was r+ and refactored to remove the ShellService stuff.
Attachment #630387 - Attachment is obsolete: true
Attachment #630648 - Flags: review?(mark.finkle)
(Reporter)

Comment 12

6 years ago
Created attachment 630649 [details] [diff] [review]
Use in about:apps

This uses this in about:apps. in onuninstall, we also don't have the manifest anymore. i.e. the third argument here is null http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#366 . So instead, I had to do this in the context menu handler.
Attachment #630649 - Flags: review?(mark.finkle)

Updated

6 years ago
Whiteboard: [qa+]

Updated

6 years ago
Whiteboard: [qa+] → [qa+:AaronMT]
Attachment #630648 - Flags: review?(mark.finkle) → review+
Attachment #630649 - Flags: review?(mark.finkle) → review+
(Reporter)

Comment 14

6 years ago
Dang it. Multiple patches are not my forte. This got lost in the pushing:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d529eceaccfa
I just realized that we do cache the "app" object and the "manifest" object in the DOM element:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutApps.js#102

So we might be able to move this to "onUninstall"
https://hg.mozilla.org/mozilla-central/rev/762be4801739
https://hg.mozilla.org/mozilla-central/rev/eb7306640e5f
https://hg.mozilla.org/mozilla-central/rev/d529eceaccfa
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
So, I',m running an inbound from this afternoon (06/07) I've added a few apps via (testmanifest.com), and added home-screen shortcuts, on uninstallation they are not removed on my home-screen. Am I missing something?
(In reply to Aaron Train [:aaronmt] from comment #17)
> So, I',m running an inbound from this afternoon (06/07) I've added a few
> apps via (testmanifest.com), and added home-screen shortcuts, on
> uninstallation they are not removed on my home-screen. Am I missing
> something?

Agreed - same behavior on my galaxy nexus phone - I get a message saying the shortcut was removed, but the shortcut is still on my phone. This patch also causes a regression in about:apps - doing an uninstall does not remove the app visually from about:apps until you reload the page. Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

6 years ago
Whiteboard: [qa+:AaronMT]
Logcat below. Looks like something is indeed failing out here:

06-11 09:57:48.049: I/GeckoApp(23074): Got message: Shortcut:Remove
06-11 09:57:48.073: W/GeckoAppShell(23074): removeShortcut for http://mozilla.clock6.com/ [World Clocks] > webapp
06-11 09:57:48.073: W/InputManagerService(181): Window already focused, ignoring focus gain of: com.android.internal.view.IInputMethodClient$Stub$Proxy@41a7bf10
06-11 09:57:48.128: E/GeckoConsole(23074): [JavaScript Error: "NS_ERROR_FAILURE: Failure arg 0 [nsIDOMEventListener.handleEvent]" {file: "jar:jar:file:///data/app/org.mozilla.fennec-2.apk!/omni.ja!/components/Webapps.js" line: 367}]
06-11 09:57:48.471: I/Launcher(469): setLoadOnResume
06-11 09:57:48.495: I/Launcher(469): setLoadOnResume
06-11 09:57:48.526: I/Launcher(469): setLoadOnResume
06-11 09:57:48.526: I/Launcher(469): setLoadOnResume
06-11 09:57:48.573: I/Launcher(469): setLoadOnResume
06-11 09:57:48.588: I/Launcher(469): setLoadOnResume
06-11 09:57:48.596: I/Launcher(469): setLoadOnResume
06-11 09:57:48.612: I/Launcher(469): setLoadOnResume
06-11 09:57:48.635: I/Launcher(469): setLoadOnResume
06-11 09:57:48.651: I/Launcher(469): setLoadOnResume
06-11 09:57:48.659: I/Launcher(469): setLoadOnResume
06-11 09:57:48.682: I/Launcher(469): setLoadOnResume
06-11 09:57:48.729: I/Launcher(469): setLoadOnResume
06-11 09:57:48.799: D/dalvikvm(469): GC_FOR_ALLOC freed 4147K, 36% free 22283K/34567K, paused 37ms
06-11 09:57:48.854: I/Launcher(469): setLoadOnResume
(Reporter)

Comment 21

6 years ago
Can you list devices? This works fine for me on at least two.
(Reporter)

Comment 22

6 years ago
Also, are the particular apps you're installing and seeing problems. I've tried with BarFight installed from https://apps.mozillalabs.com/appdir/ and World Clock from https://marketplace.mozilla.org/.

The other error seems unrelated, but I can't reproduce it either.
(In reply to Wesley Johnston (:wesj) from comment #21)
> Can you list devices? This works fine for me on at least two.

Still getting a reproduction on a Samsung Galaxy Nexus. Aaron - Can you still reproduce and on what devices?
Grabbing this from Wes -- I've got some changes/fixes here that should resolve this and a few other bits.
Assignee: wjohnston → vladimir
So with my patch in bug 753102, we remove the shortcut (I believe the failure before had to do with a different launchURL vs. origin; fixed in the patch to be consistent), but something still fails:

E/GeckoConsole(15777): [JavaScript Error: "NS_ERROR_FAILURE: Failure arg 0 [nsIDOMEventListener.handleEvent]" {file: "jar:jar:file:///data/app/org.mozilla.fennec_vladimir-2.apk!/omni.ja!/components/Webapps.js" line: 367}]

that's the handleEvent call in:

      case "Webapps:Uninstall:Return:OK":
        if (this._onuninstall) {
          let event = new WebappsApplicationEvent(new WebappsApplication(this._window, msg.origin, null, null, null, null, 0));
          this._onuninstall.handleEvent(event);
        }
        break;

This is causing the app to not be removed from about:apps.  I'm not sure yet why this is happening.
From some reading -- it seems that UNINSTALL_SHORTCUT may be broken on ICS, though I've definitely seen it work.  Still trying to figure out what the conditions for breakage are... I get the toast notification that it uninstalled a shortcut, but the shortcut doesn't go away.  (If I use the wrong title or something I get no notification.)
You need to log in before you can comment on or make changes to this bug.