Closed Bug 1031094 Opened 5 years ago Closed 5 years ago
[Collection] Removing an e
.me app from the homescreen also removes it from the top of the collection
46 bytes, text/x-github-pull-request
|Details | Review|
4.37 MB, video/mp4
4.73 MB, video/mp4
114.29 KB, text/plain
STR 1. Open a Smart Collection 2. Add a e.me app to the homescreen 3. Add it to the top of the Smart Collection 4. Go to the homescreen and remove it. 5. Go back to the collection Actual result The app is moved back with the other. Worst case, if the Internet connection is off while removing the bookmark on the homescreen, the icon is no more present.
Bad user experience.
blocking-b2g: --- → 2.0?
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking+]
Did this problem happen in 1.4 with the old e.me search implementation?
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking+] → [VH-FL-blocking-][VH-FC-blocking?]
No, it doesn't. The icon is still at the top of the homescreen in 1.4.
Appears that this is because we call down into unpin() for both mozApp types and bookmark types. We should probably not do this for bookmarks. https://github.com/mozilla-b2g/gaia/blob/master/apps/collection/js/native_info.js#L105 The proper fix should include a test which adds a result the the homescreen, pins it, removes the homescreen bookmark, and verifies that the pin is still there.
Chris - do you have any cycles to look at this one? The hardest part for this is probably writing the marionette test, which is something we definitely want for this.
I'll take a look at this.
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
I have a patch that fixes the issue, I'm working on an integration test to make sure this doesn't regress.
Comment on attachment 8448210 [details] [review] Don't unpin uninstalled bookmarks Looking good, but I'd like to see another version before leaving an R+. Left a few comments on github, thanks for taking the time to write a test!
Attachment #8448210 - Flags: review?(kgrandon) → feedback+
Comment on attachment 8448210 [details] [review] Don't unpin uninstalled bookmarks Ok, attempt 2 is up :) I've answered your comments on github, I couldn't take everything into account, but most of it. We can discuss it later if it isn't quite enough.
Comment on attachment 8448210 [details] [review] Don't unpin uninstalled bookmarks I would still like to assert on the icon identifier being present, but besides that I think we are good to go. Thanks!
Attachment #8448210 - Flags: review?(kgrandon) → review+
Squashed commit with last revision made. Carrying f+/r+
We had some green test runs, and this looks good to me, so landing: https://github.com/mozilla-b2g/gaia/commit/bc215204fb986871f1a26d4c459ba1cfc3f8c5c3 Thanks for working on this test!
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
This issue has been failed verified on Flame 2.1. See attachment: verify_v2.1.mp4. Reproduce rate: 5/6 Repro STR: 1. Open a Smart Collection. 2. Select a e.me app and long tap,select "Add to Home Screen". 3. Long tap the app and select "Add to top of collection". 4. Go to the homescreen and remove it. 5. Go back to the collection. **After removed the e.me app from the homescreen,it will also be removed from the top of the collection. This issue has been successfully verified on Flame 2.0. See attachment: verified_v2.0.mp4. Reproduce rate: 0/5 Flame 2.1 build: Gaia-Rev ccb49abe412c978a4045f0c75abff534372716c4 Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22 Build-ID 20141201001201 Version 34.0 Device-Name flame FW-Release 4.4.2 FW-Incremental eng.cltbld.20141201.034405 FW-Date Mon Dec 1 03:44:15 EST 2014 Bootloader L1TC00011880 Flame 2.0 build: Gaia-Rev 8d1e868864c8a8f1e037685f0656d1da70d08c06 Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3 Build-ID 20141201000201 Version 32.0 Device-Name flame FW-Release 4.4.2 FW-Incremental eng.cltbld.20141201.034308 FW-Date Mon Dec 1 03:43:18 EST 2014 Bootloader L1TC00011880
Add flame v2.1 logcat.
Add NI?whsu to follow up
Flags: needinfo?(hlu) → needinfo?(whsu)
Thanks Shally! :) A bug was submitted. (Bug 1118699)
You need to log in before you can comment on or make changes to this bug.