1022713 - Optimise Gaia home-screen edit mode

Status: RESOLVED WORKSFORME

(Firefox OS Graveyard :: Gaia::Homescreen, defect)

Description

[Chris Lord [:cwiiis]]

While working on bug 1009530, there were a few performance improvements I made that weren't necessarily related to the feature itself. It would be good to get those in the tree to provide a (slightly) more fluid editing experience :)

Comment 1

[Chris Lord [:cwiiis]]

Attachment: Improve performance of vertical home-screen in edit mode

These are most of the performance improvements I made while working on bug 1009530, split out into several separate commits (that should all work correctly when broken up).

I haven't included the split render path as I think it complicates the code more than is necessary (it's only a performance improvement when animating the icons, so we may want to reconsider it at a later date).

Comment 2

[Kevin Grandon :kgrandon]

Comment on attachment 8437007 [details] [review]
Improve performance of vertical home-screen in edit mode

This is looking pretty good. I'm going to go through this patch now, but it looks like we have problems with both jshint and testing. If you could rebase and get a green travis build I will take a look again. Thanks!

Comment 3

[Kevin Grandon :kgrandon]

Hey Chris - A fairy told me that you might have some cycles soon to work on this. Is there any chance you'd be willing to work on it this week with us? If you can, that would be amazing, if not no worries just let me know and I'll assign this to myself.

I was playing around with the patch before, and noticed a few issues. The main one being icon movement over the last divider, I could not seem to drag over it to create a new group. As things go this one got bitrotted unfortunately. If there are some smaller safe patches that you would want to rip out and land first, we can certainly do so. Thanks!

Comment 4

[Chris Lord [:cwiiis]]

(In reply to Kevin Grandon :kgrandon from comment #3)
> Hey Chris - A fairy told me that you might have some cycles soon to work on
> this. Is there any chance you'd be willing to work on it this week with us?
> If you can, that would be amazing, if not no worries just let me know and
> I'll assign this to myself.

That fairy hasn't spoken to me yet, but I've taken a little time to check up on this anyway :)

> I was playing around with the patch before, and noticed a few issues. The
> main one being icon movement over the last divider, I could not seem to drag
> over it to create a new group. As things go this one got bitrotted
> unfortunately. If there are some smaller safe patches that you would want to
> rip out and land first, we can certainly do so. Thanks!

I've rebased on master and removed the patch that separates the icon into a separate element - I think that patch was causing most/all of the test failures, and isn't much of an optimisation on its own, so I removed it. Waiting on travis now, but I can confirm that dragging an icon over a divider worked fine testing it locally.

Comment 5

[Jason Smith [:jsmith]]

Kevin - Is this required to ship vertical homescreen? Or if push came to shove, could we live without this?

Comment 6

[Chris Lord [:cwiiis]]

I just got all tests passing on this, so there's a good chance it could be merged today if it's deemed urgent.

Comment 7

[Kevin Grandon :kgrandon]

This is going to land today, and the improvements are nice. (I don't think we'd ship without them, so please block I guess) - Toggling the flags for you if that's ok.

Comment 8

[Jason Smith [:jsmith]]

Okay - Candice - Can you add the feature-b2g flag then, since we're claiming this is feature work & a blocker for ship?

Comment 9

[Kevin Grandon :kgrandon]

Attachment: Github pull request

Rebased PR with a commit that disables the test for now.

Comment 10

[Kevin Grandon :kgrandon]

I noticed a problem where dragging dividers outside of a group will cause the group not to collapse. I think it's fairly important to fix this, so I'm going to hold off on this patch for now and work on a test that covers this.

Comment 11

[Kevin Grandon :kgrandon]

I would like to start landing these patches to get the improvements in and not cause rebasing issues. I'm going to start blocking this bug with sub-bugs, and landing the pieces that we can. Thanks!

Comment 12

[Kevin Grandon :kgrandon]

Hey Chris - currently working on getting the first patch landed and looking at the second one here: https://github.com/KevinGrandon/gaia/commit/4d695ffa96ff3d7cceb9ef437475965a2aa0d0a7

If I understand this correctly, this allows us to choose a lower resolution icon than the actual size, correct? Are we sure that upscaling from a lower resolution will produce as good results as downscaling?

Comment 13

[Chris Lord [:cwiiis]]

Thanks for helping getting these landed! I think the most difficult one is going to be the from/to patch, which is probably the most helpful one... But I'm sure we'll get there :)

(In reply to Kevin Grandon :kgrandon from comment #12)
> Hey Chris - currently working on getting the first patch landed and looking
> at the second one here:
> https://github.com/KevinGrandon/gaia/commit/
> 4d695ffa96ff3d7cceb9ef437475965a2aa0d0a7
> 
> If I understand this correctly, this allows us to choose a lower resolution
> icon than the actual size, correct? Are we sure that upscaling from a lower
> resolution will produce as good results as downscaling?

The original code will pick the nearest size that is smaller than the desired resolution (even if there's one that's an exact match); the new code will pick the nearest size irrespective of whether it's larger or smaller.

We could always pick the nearest larger size of course too, but I think given our icon size guidelines, it makes sense to just pick the nearest. If we did pick the nearest larger size, we'd definitely want to allow exact matches and to disregard the activeScale, otherwise we'd always pick the next biggest rather than an exact match for what the user will 99% of the time see.

We may want to disregard activeScale anyway, really - but I don't think the size of the activeScale will (commonly) cause a jump away from an exact match.

Comment 14

[Chris Lord [:cwiiis]]

(In reply to Chris Lord [:cwiiis] from comment #13)
> Thanks for helping getting these landed! I think the most difficult one is
> going to be the from/to patch, which is probably the most helpful one... But
> I'm sure we'll get there :)
> 
> (In reply to Kevin Grandon :kgrandon from comment #12)
> > Hey Chris - currently working on getting the first patch landed and looking
> > at the second one here:
> > https://github.com/KevinGrandon/gaia/commit/
> > 4d695ffa96ff3d7cceb9ef437475965a2aa0d0a7
> > 
> > If I understand this correctly, this allows us to choose a lower resolution
> > icon than the actual size, correct? Are we sure that upscaling from a lower
> > resolution will produce as good results as downscaling?
> 
> The original code will pick the nearest size that is smaller than the
> desired resolution (even if there's one that's an exact match); the new code
> will pick the nearest size irrespective of whether it's larger or smaller.
> 
> We could always pick the nearest larger size of course too, but I think
> given our icon size guidelines, it makes sense to just pick the nearest. If
> we did pick the nearest larger size, we'd definitely want to allow exact
> matches and to disregard the activeScale, otherwise we'd always pick the
> next biggest rather than an exact match for what the user will 99% of the
> time see.
> 
> We may want to disregard activeScale anyway, really - but I don't think the
> size of the activeScale will (commonly) cause a jump away from an exact
> match.

ugh, actually no, ignore this entire comment - reading over the code again, I misread it when I first encountered it (conflating it with other issues caused by rebasing) - this patch is unnecessary.

Comment 15

[Kevin Grandon :kgrandon]

Thanks for the info. Let's drop the lower-res icon one then, and focus on getting the from/to landed. Thanks!

Comment 16

[Kevin Grandon :kgrandon]

If we get a patch here that works well, we will ask for uplift into 2.0, but I don't think this should necessarily block. For now there will be a few slight improvements landing in bug 1022414, but more are always welcome.

Comment 17

[Chris Lord [:cwiiis]]

Attachment: Only call render on the necessary items when editing

So I looked over this properly and I made some bad assumptions in the original patch. Unfortunately, for the most part, if we drag over a divider, we need to render to the end of the container - there are some situations where we don't need to, but I think the complexity of working out what those situations are would introduce a larger maintenance burden than the benefit it would give.

Also, I realise that although this lets you avoid a lot of string manipulation, this shouldn't be a performance improvement except with the later patches I wrote for the animation bug.

I'm starting to think that the added complexity of having from/to isn't really worth it, but I think this request fixes a potential bug with the placeholders that are added at the end of the grid in edit mode.

It's up to you whether you want to merge this or not - an alternative would be to separate the fix and strip out from/to altogether, but it might be nice to keep it as it could potentially be useful later.

Comment 18

[Kevin Grandon :kgrandon]

Comment on attachment 8440645 [details] [review]
Only call render on the necessary items when editing

Ok, it looks like gaia-try has a green build, and I've been stress testing this on a device with nothing bad happening. Some of it feels like overkill, but I'm sure we'll probably need it for the future. Dragging over a divider still doesn't feel quite right for example. It should probably append to the end when you drag up over a divider into a previous section. I think we'll need the logic in your patch to handle that.

Thanks so much for sticking this, will land shortly.

Comment 19

[Kevin Grandon :kgrandon]

I saw you had an empty R?, so I just filled and landed:

https://github.com/mozilla-b2g/gaia/commit/1c2db4082e7c7c5bdf18beea429d4b57b617fb87

Comment 20

[Kevin Grandon :kgrandon]

Comment on attachment 8440645 [details] [review]
Only call render on the necessary items when editing

This is required for the vertical homescreen. We've done extensive manual testing and have tests for basic functionality of this feature. It should be safe to uplift. Thanks!

Comment 21

[Kevin Grandon :kgrandon]

Comment on attachment 8440645 [details] [review]
Only call render on the necessary items when editing

Clearing approval as we may need a few tweaks first.

Comment 22

[Kevin Grandon :kgrandon]

So I unfortunately had to back this out for breaking the collections view. Still not entirely sure what's wrong, but they refuse to render with this. 

(I did unfortunately have another patch which adds some testing for this stuff, but it hasn't landed yet). I'll try to look into this to see if I can spot the problem.

Comment 23

[Chris Lord [:cwiiis]]

Attachment: Only call render on the necessary items when editing v2

Sorry for not being thorough enough with this before - this fixes the issue with collections not rendering;

With collections, the first item is a divider which ends up getting removed - I don't know why that divider is there, but this could happen if the from points to a divider or placeholder and there are no items before it.

I've fixed that case by only setting the from/to items when they're found and setting from to zero when a from item isn't found.

Comment 24

[Kevin Grandon :kgrandon]

Comment on attachment 8441384 [details] [review]
Only call render on the necessary items when editing v2

Nice work, haven't found any problems yet. I made a few comments on github.

Travis is red due to some linter issues, go ahead and fix those and feel free to land. Thanks!

Comment 25

[Kevin Grandon :kgrandon]

Careful about landing this, we're seeing some errors on TBPL now, and I've re-triggered a few jobs to check on them.. =/

https://tbpl.mozilla.org/php/getParsedLog.php?id=41889855&tree=Gaia-Try

Comment 26

[Kevin Grandon :kgrandon]

As discussed in our standup we landed will-change support in bug 1022713 yesterday. This is good enough for now, but if we get a fix in we can uplift.

Candice - can you remove the FL and FC blocking status? Thanks!

Comment 27

[Jason Smith [:jsmith]]

missed kevin's comment when I switched flags here

Comment 28

[Kevin Grandon :kgrandon]

Oh, my bad. I actually didn't mean to switch in the first place - meant for Candice to do it because I'm not sure of the process. Sorry about that.

Comment 29

[Kevin Grandon :kgrandon]

We can still land this and request approval if we get a fix here.

Comment 30

[Gregor Wagner [:gwagner]]

Should we land this on trunk?

Comment 31

[Chris Lord [:cwiiis]]

(In reply to Gregor Wagner PTO until 6/30 from comment #30)
> Should we land this on trunk?

So this change doesn't bring much of a performance improvement on its own, though it opens up some possibilities (it helped with animating the icons in edit mode).

I think we could live without it, as it does complicate some of the code, perhaps unnecessarily. We can always revisit the idea if we find we have need.

Comment 32

[Chris Lord [:cwiiis]]

I'm going to mark this as worksforme - I don't think the last patch is worth landing anymore. We can work on something similar if we find we have need for it and it'd be better to open more specific bugs.