Closed
Bug 1029891
Opened 10 years ago
Closed 10 years ago
Deleting smartCollections and bookmarks is very slow
Categories
(Firefox OS Graveyard :: Gaia::Homescreen, defect, P1)
Tracking
(blocking-b2g:2.0+, b2g-v2.0 verified, b2g-v2.1 verified)
People
(Reporter: ladamski, Assigned: jlal)
References
Details
(Keywords: perf, uiwanted, Whiteboard: [c=progress p=3 s= u=2.0][systemsfe])
Attachments
(2 files)
When tapping on the X icon to delete smartCollections and bookmarks, the deletion dialog often takes a few seconds to be displayed. X to delete for normal apps is fast.
Reporter | ||
Updated•10 years ago
|
blocking-b2g: --- → 2.0?
Comment 1•10 years ago
|
||
QA Wanted for a video.
Comment 2•10 years ago
|
||
It is not ideal but it is normal due to the implementation. To delete apps the confirm message is displayed in the homescreen app but smartCollections and bookmarks are controlled by other apps via a MozActivity. Nothing to do here from homescreen point of view
Reporter | ||
Comment 3•10 years ago
|
||
I think its problematic for the UI to be unresponsive for several seconds, so we need to find some UX trick to provide feedback to the user while that loads.
Keywords: uiwanted
Updated•10 years ago
|
Severity: normal → blocker
Priority: -- → P1
Whiteboard: [c=progress p=3 s= u=]
Updated•10 years ago
|
blocking-b2g: 2.0? → 2.0+
Whiteboard: [c=progress p=3 s= u=] → [c=progress p=3 s= u=2.0]
Comment 4•10 years ago
|
||
According to our goals for meeting responsiveness [1], we want to show this interaction within 140ms. If we cannot, we need to display some kind of indicator that there is some action in progress within 1000ms, e.g. with a spinner, etc. [1] https://developer.mozilla.org/en-US/Apps/Build/Performance/Firefox_OS_app_responsiveness_guidelines#User_stories
Assignee | ||
Comment 5•10 years ago
|
||
IMO this can be very simple... we just delete the icon after the confirm (without really deleting it) and delete the thing asynchronously.
Comment 6•10 years ago
|
||
The problem is that currently we surface the confirm() inside of the collection or bookmark app, and surface that using a MozActivity. Should we move this confirm into the homescreen app instead?
Comment 7•10 years ago
|
||
Gregor, do you have anyone that can take ownership of this issue? (FxOS Performance Triage)
Flags: needinfo?(anygregor)
Comment 8•10 years ago
|
||
(In reply to Eli Perelman, :Eli from comment #7) > Gregor, do you have anyone that can take ownership of this issue? (FxOS > Performance Triage) Gregor is on PTO, so I'm redirecting this to Candice.
Flags: needinfo?(anygregor) → needinfo?(cserran)
Assignee | ||
Comment 9•10 years ago
|
||
A reasonable approach to me seems to: - Move the dialogs to the homescreen (or gaia_grid that is another issue) - Use datastore to remove the collections, bookmarks asynchronously - Live with the very-very-very edge case of unsuccessful downloads
Assignee | ||
Comment 10•10 years ago
|
||
I can provide a patch today to fix this.
Flags: needinfo?(cserran)
QA Contact: jlal
Assignee | ||
Comment 11•10 years ago
|
||
For best performance we need to move strings around... there is no way to workaround this and get the performance we need (with the same strings).... The ids will move between apps but the strings themselves should remain the same.
Keywords: late-l10n
Assignee | ||
Comment 12•10 years ago
|
||
Hrm- on second thought we can do this without l10n changes if we simply reuse the same string for apps/collections which should not be the end of the world for near-instant downloads. <name> and all of its data will be deleted.
Keywords: late-l10n
Assignee | ||
Comment 13•10 years ago
|
||
Needs tests for bookmark/collection removal (we already have some for app installs).
Attachment #8446240 -
Flags: feedback?(kgrandon)
Comment 14•10 years ago
|
||
Comment on attachment 8446240 [details] [review] https://github.com/mozilla-b2g/gaia/pull/21000 Makes sense to me, thanks.
Attachment #8446240 -
Flags: feedback?(kgrandon) → feedback+
Updated•10 years ago
|
Assignee: nobody → jlal
Status: NEW → ASSIGNED
QA Contact: jlal
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 8446240 [details] [review] https://github.com/mozilla-b2g/gaia/pull/21000 Ready for review... I fixed the test failures (in one case removing the unit test in favor of the better integration test) and adding tests for bookmark removal / collection removal.
Attachment #8446240 -
Flags: feedback+ → review?(kgrandon)
Comment 16•10 years ago
|
||
Please be careful because the message is different when you are removing an app or deleting a bookmark and you are doing the same thing in your pull request. See http://i.imgur.com/itHxCCe.png Bookmarks: Remove <name> Remove <name> form homescreen? Cancel | Remove With your implementation here you could close this bug 1029477 because I think that you are fixing it too seeing your code in github
Assignee | ||
Comment 17•10 years ago
|
||
I chatted with UX today about this (the bookmark case at least) changing the strings is less then ideal at this point but we can try something. @jsavory Are you okay with the above screenshot ? (Thanks Cristian)
Flags: needinfo?(jsavory)
Comment 18•10 years ago
|
||
Comment on attachment 8446240 [details] [review] https://github.com/mozilla-b2g/gaia/pull/21000 Hey, you basically have an R+ here, but I do think there is a legit failing test for pinning items to smart collections. I haven't looked into why yet, but can help dig in tomorrow. Nice job.
Attachment #8446240 -
Flags: review?(kgrandon) → review+
Updated•10 years ago
|
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking?] → [VH-FL-blocking-][VH-FC-blocking+]
Whiteboard: [c=progress p=3 s= u=2.0] → [c=progress p=3 s= u=2.0][systemsfe]
Assignee | ||
Comment 19•10 years ago
|
||
Ugh - should not attempt to read images at night ... the image above is the current images not what this patch does.
Assignee | ||
Comment 20•10 years ago
|
||
:D I will fix this right now since I have an hour... So happy we have those tests ++
Assignee | ||
Comment 21•10 years ago
|
||
Should be fixed now... There was a typo in collection additions and a missing name change in the collections app.
Updated•10 years ago
|
Assignee: jlal → kgrandon
Target Milestone: --- → 2.0 S5 (4july)
Assignee | ||
Updated•10 years ago
|
Assignee: kgrandon → jlal
Comment 22•10 years ago
|
||
Landed: https://github.com/mozilla-b2g/gaia/commit/a62db8f1a3c52546d30fbd08b5e539676c5fc6c0 There is some UX discussion happening in bug 1029477, but because it appears there are no l10n changes here I think this is a win and we should take it for 2.0. I will also close bug 1029477, but UX can follow-up here or there. Thanks!
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: needinfo?(jsavory)
Resolution: --- → FIXED
Comment 24•10 years ago
|
||
v2.0: https://github.com/mozilla-b2g/gaia/commit/f19ca18d48804278964771d8ee4e447d2784d8b1
status-b2g-v2.0:
--- → fixed
status-b2g-v2.1:
--- → fixed
Comment 25•9 years ago
|
||
This issue has been successfully verified on Flame 2.0: Gaia-Rev 8d1e868864c8a8f1e037685f0656d1da70d08c06 Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3 Build-ID 20141202000201 Version 32.0 Device-Name flame FW-Release 4.4.2 This issue has been successfully verified on Flame 2.1: Gaia-Rev ccb49abe412c978a4045f0c75abff534372716c4 Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22 Build-ID 20141202001201 Version 34.0 Device-Name flame FW-Release 4.4.2
You need to log in
before you can comment on or make changes to this bug.
Description
•