Merge lp:~mardy/unity-2d/grid-flow into lp:unity-2d

Proposed by Alberto Mardegan
Status: Merged
Approved by: Gerry Boland
Approved revision: 696
Merged at revision: 738
Proposed branch: lp:~mardy/unity-2d/grid-flow
Merge into: lp:unity-2d
Diff against target: 70 lines (+31/-6)
2 files modified
places/CenteredGridView.qml (+9/-0)
places/ListViewWithHeaders.qml (+22/-6)
To merge this branch: bzr merge lp:~mardy/unity-2d/grid-flow
Reviewer Review Type Date Requested Status
Michał Sawicz functional Approve
Gerry Boland (community) Approve
Florian Boucault (community) functional Approve
Alberto Mardegan (community) Abstain
Review via email: mp+74232@code.launchpad.net

Commit message

[dash] Set height on lenses results containers

Setting the proper height prevents drawing outside the container area.
This patch also renames a few variables, and hopefully improves code
readability.

Description of the change

  [dash] Set height on lenses results containers

  Setting the proper height prevents drawing outside the container area.
  This patch also renames a few variables, and hopefully improves code
  readability.

In order to ease the testing and understanding of what this patch does, you can add the following code

==========
                    Rectangle {
                        anchors.fill: parent
                        color: "red"
                        Rectangle {
                            anchors.fill: parent
                            anchors.margins: 10
                            color: "green"
                        }
                    }
==========

inside the "bodyLoader" item, right after this line:
                    Binding { target: bodyLoader; property: "model"; value: model }

If you run the dash with this code, before and after applying the MR, you'll see how the bodyLoader element size is computed.

To post a comment you must log in.
Revision history for this message
Gerry Boland (gerboland) wrote :

Hey Alberto,
I'm testing this now (note: on Qt4.7.4 which we'll be using) and I am still seeing layout problems. Try this:

1. Open Dash, select the Applications lens. The Filter pane is open.
2. All 3 groups are collapsed. I click on the "Installed" header to show all the installed apps.
3. Scroll down to the very bottom of the list.
4. Click "Filter results" to hide the filter pane.
5. Click it again to show the filter pane. The list pops up a bit.
6. Try scrolling down to the bottom of the list again.

I see a big blank area for a while, and then the list just re-appears correctly. I can make you a video of this if you like.

review: Needs Fixing
Revision history for this message
Alberto Mardegan (mardy) wrote :

> Hey Alberto,
> I'm testing this now (note: on Qt4.7.4 which we'll be using) and I am still
> seeing layout problems. Try this:
[...]

It works fine here. I'll try upgrading to Qt 4.7.4, maybe the problem will become evident to me too.

lp:~mardy/unity-2d/grid-flow updated
696. By Alberto Mardegan

Funny hack

Revision history for this message
Alberto Mardegan (mardy) wrote :

I could reproduce the same weird behaviour that Gerry observed with Qt 4.7.4. Despite spending a lot of time debugging it, I couldn't find where the problem is. The values of the contentY property, as well as all other properties I inspected, seem to be correct throughout the application execution.

I filed https://bugreports.qt.nokia.com/browse/QTBUG-21452 about it, and here's a silly workaround.

review: Needs Resubmitting
Revision history for this message
Alberto Mardegan (mardy) :
review: Abstain
Revision history for this message
Gerry Boland (gerboland) wrote :

Hey,
still height & positioning problems:( Try this:

1. Open dash, type something (I use "d", to get a good 25+ results of installed apps).
2. Expand the "Installed Apps".
3. Scroll down to the bottom of the list.
4. Maximise the dash (using button at bottom right corner).
5. Now try scrolling up.

Contents of the first list jitter up & down. If you need, I can make a video.
-G

review: Needs Fixing
Revision history for this message
Alberto Mardegan (mardy) wrote :

[...]
> Contents of the first list jitter up & down. If you need, I can make a video.

Yep, that would be very appreciated! Unfortunately I cannot reproduce it, I tried with having the filters pane on and off as well, but no luck.

Revision history for this message
Gerry Boland (gerboland) wrote :

Video attached

On 15 September 2011 08:19, Alberto Mardegan <<email address hidden>
> wrote:

> [...]
> > Contents of the first list jitter up & down. If you need, I can make a
> video.
>
> Yep, that would be very appreciated! Unfortunately I cannot reproduce it, I
> tried with having the filters pane on and off as well, but no luck.
> --
> https://code.launchpad.net/~mardy/unity-2d/grid-flow/+merge/74232<https://code.launchpad.net/%7Emardy/unity-2d/grid-flow/+merge/74232>
> You are reviewing the proposed merge of lp:~mardy/unity-2d/grid-flow into
> lp:unity-2d.
>

Revision history for this message
Gerry Boland (gerboland) wrote :

Hmmm, well I emailed you the video directly. Looks like you can't attach files to merge request comments in any way.

Revision history for this message
Alberto Mardegan (mardy) wrote :

Hi Gerry! I tried again, merging this branch on top of the trunk branch, but still I cannot reproduce it. What happens here, is that the view scrolls back towards the top when maximizing -- which also might not be the desired behaviour, but it's not an obvious bug either.

Could you please re-test it? BTW, I'm using Qt 4:4.7.4-0ubuntu2~ppa1

Revision history for this message
Florian Boucault (fboucault) wrote :

The overall behaviour is better with the patch rather than without. I do see the jitter that Gerry is referring to though.
Let's have a look at the code now.

review: Approve (functional)
Revision history for this message
Florian Boucault (fboucault) wrote :

Interestingly if only the change to places/CenteredGridView.qml is kept, the behaviour is vastly improved in a different way.

Revision history for this message
Florian Boucault (fboucault) wrote :

I have a few ideas I would like to test before we merge it. Thanks for the patience.

Revision history for this message
Alberto Mardegan (mardy) wrote :

I pushed one more silly commit to another branch: lp:~mardy/unity-2d/test
Does it make the behaviour any better to you?

Revision history for this message
Gerry Boland (gerboland) wrote :

I cannot reproduce the earlier issue. It looks fine. I'm happy to merge this.

review: Approve
Revision history for this message
Michał Sawicz (saviq) wrote :

Works fine here!

review: Approve (functional)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'places/CenteredGridView.qml'
2--- places/CenteredGridView.qml 2011-07-27 17:36:25 +0000
3+++ places/CenteredGridView.qml 2011-09-14 12:59:52 +0000
4@@ -35,6 +35,15 @@
5 return true
6 }
7
8+ /* FIXME: HACK to workaround GridView's layout issues in Qt 4.7.4.
9+ Ref.: https://bugreports.qt.nokia.com/browse/QTBUG-21452
10+ */
11+ onWidthChanged: {
12+ var y = contentY
13+ contentY = 0
14+ contentY = y
15+ }
16+
17 Keys.onPressed: if (handleKeyPress(event.key)) event.accepted = true
18 function handleKeyPress(key) {
19 switch (key) {
20
21=== modified file 'places/ListViewWithHeaders.qml'
22--- places/ListViewWithHeaders.qml 2011-09-07 12:12:01 +0000
23+++ places/ListViewWithHeaders.qml 2011-09-14 12:59:52 +0000
24@@ -211,11 +211,27 @@
25 height: headerLoader.height + bodyLoader.item.totalHeight
26 property bool focusable: bodyLoader.item.focusable
27
28- property int pmin: pmax - (ymax - ymin)
29- property int pmax: items.heightFirstChildren(index) - ymin
30- property int ymin: list.accordion ? items.heightFirstHeaders(index) : -headerLoader.height
31+ property int heightFirstHeaders: items.heightFirstHeaders(index)
32+ property int heightFirstChildren: items.heightFirstChildren(index)
33+ /* Minimum Y coordinate in viewport space where the item header
34+ * can appear; in the non-accordion case, this is
35+ * -headerLoader.height to allow the header to be placed
36+ * offscreen; the body is anchored to the header on top, so the
37+ * body will appear at viewport Y coordinate 0.
38+ */
39+ property int ymin: list.accordion ? heightFirstHeaders : -headerLoader.height
40 property int ymax: list.accordion ? ymin + items.availableHeight : list.height
41- y: items.clamp(-items.value + ymax + pmin, ymin, ymax)
42+ /* Physical space available for drawing the item body; in the
43+ * non-accordion case we need to compensate the
44+ * -headerLoader.height which we added to ymin.
45+ */
46+ property int bodyAvailableHeight: ymax - y + (list.accordion ? 0 : -headerLoader.height)
47+ /* Y coordinate where the body of this item is located, relative to "list" */
48+ property int bodyVirtualY: heightFirstChildren - items.value
49+ /* Y coordinate where the body becomes visible (in body coordinates) */
50+ property int bodyVisibleY: ymin - bodyVirtualY
51+ y: items.clamp(bodyVirtualY, ymin, ymax)
52+
53
54 Loader {
55 id: headerLoader
56@@ -248,12 +264,12 @@
57 onLoaded: item.focus = true
58 width: parent.width
59 anchors.top: headerLoader.bottom
60- height: items.clamp(parent.ymax - parent.y, 0, item.totalHeight)
61+ height: Math.min(items.clamp(item.totalHeight - bodyVisibleY, 0, item.totalHeight), bodyAvailableHeight)
62
63 Binding {
64 target: bodyLoader.item
65 property: "contentY"
66- value: Math.max(items.value - pmax, 0)
67+ value: Math.max(bodyVisibleY, 0)
68 }
69
70 /* Workaround Qt bug http://bugreports.qt.nokia.com/browse/QTBUG-18857

Subscribers

People subscribed via source and target branches