Closed Bug 1091007 Opened 6 years ago Closed 6 years ago
Installing an app with last group collapsed is broken
46 bytes, text/x-github-pull-request
|Details | Review|
STR: - Have app grouping enabled and last group on home screen collapsed. - Install an app from marketplace. - Formatting of last group will be messed up.
Will fix this tomorrow.
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
We no longer use placeholders past the last divider to create new sections, so this simplifies things a bit and removes removeUntilDivider.
Comment on attachment 8515034 [details] [review] Don't append apps to a collapsed group when installing Code looks fine, but I have one concern about what happens when the home screen gets killed. Left a comment on github, re-flag me when ready. Thanks!
Comment on attachment 8515034 [details] [review] Don't append apps to a collapsed group when installing So you're right, I think we can just take the add-to-expanded-group path every time, the initial load in works via synchronising first, then any apps that weren't in the verticalhome store will go via the addIconToGrid path (and any apps that aren't in the store are ones that were installed while the homescreen was killed) It bears out in testing locally, and if it's incorrect, I really hope automated testing catches it :p
Comment on attachment 8515034 [details] [review] Don't append apps to a collapsed group when installing Hey Chris - did you push a new version? When I install an app after manually triggering an OOM kill, I still see them getting appended to the collapsed groups. The same goes for bookmarks - I guess they should have the same logic during an OOM kill? I believe this is going into the synchronize path here: https://github.com/Cwiiis/gaia/blob/bug1091007-install-collapsed-group/apps/verticalhome/js/sources/application.js#L191 Perhaps we should consider removing the second argument to addIconToGrid()?
Comment on attachment 8515034 [details] [review] Don't append apps to a collapsed group when installing damnit, forgot to actually add the changed files...
Comment on attachment 8515034 [details] [review] Don't append apps to a collapsed group when installing Looks great now that the changes are in. Thanks!
Attachment #8515034 - Flags: review?(kgrandon) → review+
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
I'm not sure if this is related but as of today's master, when you install an app with the last group collapsed, it will create a new group and place the newly installed app in it. If the last group is opened, the newly app will go in there instead. This behavior continues if the last group is collapsed so you could end up with new group every time you install a new app (with the last group collapsed). I think this approach is a bit unpredictable and was wondering if we would consider a simpler design? Maybe have a pseudo "downloads" group that appears at the last position when something is downloaded/bookmarked? I'm not sure what the solution is but I would like to get Katie's feedback on it.
hey Hung, I just reviewed Maria's spec (pages 13-14). The spec'd behaviour: >If the last group is collapsed a new group is created and the icon is placed as the first icon in the new group. >If the last group is expanded the new icon is placed in the next available slot in the unnamed group. As for the scenario you mention Hung, only if the user collapses the last group, then adds a new app from marketplace, will a new group will be created. That said, I agree, this could create a lot of unnecessary new groups, if every time the user downloads an app from Marketplace (or stars a web app or bookmarks a site). I'll mock something up to discuss in our next sfe sync. Chris - Is it possible to detect if the last group has been made by the system (downloads/bookmarks) or made by the user (in edit mode)?
Flags: needinfo?(kcaldwell) → needinfo?(chrislord.net)
(In reply to katieC from comment #10) > Chris - Is it possible to detect if the last group has been made by the > system (downloads/bookmarks) or made by the user (in edit mode)? I'm not sure what this would consist of? If an app install creates a new group, when does that group switch over to being a user-created group - when you collapse it? Or when you add new apps to it? Never? What if the user moves the group between other groups? Or moves it, then moves it back? We could put a marker on groups to give them a particular designation, but I don't know what set of circumstances would be sensible to do this in. As to the original comment, I don't think there's really a problem here. You'd only end up with lots of groups if you installed lots of apps one-by-one, collapsing the newly-created group between each install. Why would you do that? I don't think this could happen by accident...
Based on comment 9 + some UX review... New general rule for adding new apps to homescreen: New apps (hosted/packaged/bookmarks/web apps) are always added to the last group in the next available slot. If the last group is collapsed, expand group to show the user where new app has been added. Ni'ing Chris to make the update. Thanks!
Filed bug 1102220 to track this change.
Issue verified fixed on Flame 3.0 When user installs app from Marketplace, it behaves correctly in regards to app grouping on homescreen regardless of whether the group it is placed (the last group available on homescreen) in is collapsed or expanded. If the group is collapsed, the group expands and the new icon is added to the previously collapsed group on return to homescreen. If the last available group was already expanded, it stays expanded. If all app groups are collapsed, only the app grouping where the app was added to is expanded, the rest of the groups remain collapsed. All of these were checked on LTR as well as RTL languages. Device: Flame 3.0 Master Build ID: 20150209010211 Gaia: 0d7b35f23402c4cb29bca6b98280fec48a196dec Gecko: 3436787a82d0 Gonk: e7c90613521145db090dd24147afd5ceb5703190 Version: 38.0a1 (3.0) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
You need to log in before you can comment on or make changes to this bug.