Merge lp://qastaging/~fboucault/unity-2d/fix_nested_grid_view_positioning into lp://qastaging/unity-2d/0.4

This fix is tricky because it's part of a bigger hack whose rationale is explained in the FIXME above the modified code. Explaining this hack is very tricky and understanding it very difficult as well.

To review that MR I suggest that the reviewer proceeds to functional testing and especially stress testing on scrolling up and down the grid of results and at the same time expanding/folding groups and also switching sections.

As suggested, I stress-tested the grid of results with a lot of scrolling around, expanding/folding groups and switching sections, and I couldn’t observe visible regressions. The performance is good too, no visible slow down.

This fixes bug #703588 too, comment #6 provides a procedure to reliably reproduce.

I’ll now try to understand the fix code-wise; at a first glance it looks ok.

I think I managed to make sense of the "hack", its explanation, and the changes that fix the bug. My only question is: does the condition (flickable.height < 0) ever evaluates to true? If so, when?

Hmm, I cannot remember why that condition was introduced (it's not coming from this patch really but from an earlier revision in that code (see flickable.height > 0):

25 - onCompensateYChanged: {
26 - if (flickable.height > 0 && totalHeight >= flickable.height) {
27 - y = compensateY
28 - contentY = compensateY
29 - }
30 - }

I suppose that in some cases the flickable.height were negative but I would need to investigate to figure out if it's ever the case.

Your change doesn’t seem equivalent to the current version to me.

In the current version, y was set to compensateY only if flickable.height > 0.
In your change, compensateY is set to 0 if flickable.height < 0.

I suspect that flickable.height is either == 0 or > 0, but never < 0 (an item with a negative height doesn’t make sense, does it?).

So in any case I’d change the test in the computation of compensateY to (flickable.height == 0). And I’d keep in mind that we may be able to simplify the computation by removing this test altogether, if we manage to confirm that flickable.height is never null.

Good analysis.

Answers:
"In your change, compensateY is set to 0 if flickable.height < 0."

yes but it mainly is *not* set to compensateY when flickable.height < 0 which is _nearly_ equivalent to (_nearly_ because of the flickable.height == 0 case):

"y was set to compensateY only if flickable.height > 0"

So we need an empirical analysis to determine what values flickable take usually. Though I would argue against spending time on it because:
- we already functionally QAed the current code twice (once me, once you)
- apart from the == 0 case, the test was already there
- this code will disappear in the future for some bigger reasons

Agreed.
Could you please add a tiny comment before the declaration of compensateY to explain that the test (flickable.height < 0) is probably useless, but that removing it would require thorough testing.
This is just in case someone else than you has to read through this code before it’s replaced. As you commented, this piece of code is tricky, so explanatory comments are welcome.

Then let’s have this merged!