Header adds to Flickable topMargin instead of replacing it.

[Alberto Mardegan]

Th attached test application works fine under qml-module-ubuntu-components 1.3.1918+16.04.20160404-0ubuntu3 but breaks with the latest 1.3.1938+16.04.20160416. At a first examination, I believe that the change which cause the regression is this one:

https://code.launchpad.net/~tpeeters/ubuntu-ui-toolkit/invisible-header-topmargin/+merge/290659

Run the attached test case with QML scene. You can scroll the view horizontally to see the other model items. Under the old version of the toolkit, all items have their page headers correctly aligned; with the new version, an extra spacing is added below the header.

I found this bug while testing my ttrss app in rc-proposed; please don't let this UITK version reach our users, as more apps might be affected.

[Andrea Bernabei (faenil)]

I'm not the developer of PageHeader, but I had a look at the code, and it seems that's the expected outcome.

When PageHeader.flickable is set, PageHeader adds its height to the topMargin to ensure all the content is visible.

In your case, when PageHeader.flickable is set both PageHeader and you are setting topMargin. As a consequence, you get a double top margin.

DISCLAIMER: I just had a quick look at the code, so take my words with a pinch of salt :)

If, instead, you get the double height also when PageHeader.flickable is null, then my words are void.

[Alberto Mardegan (mardy)]

I understand it's the expected outcome, but why was it changed? None of the bugs which the MP claimed to fix require this change.
Actually, if you look at line 209 of the diff (at https://code.launchpad.net/~tpeeters/ubuntu-ui-toolkit/invisible-header-topmargin/+merge/290659) you'll see that a unit test was changed to accommodate for this behavioural change. Which proves that there has been a behavioural change in the first place. :-)

Why do we need to *sum* the header's height to the flickable.topMargin? I think that the correct solution should be to *replace* the flickable.topMargin with the header height, while the header is active on the flickable. I don't see any reason why the two values should be summed.

[Andrea Bernabei (faenil)]

because topMargin should be available for the dev to use it for whatever he needs it to. The developer should not have to mind about setting it to accomodate a behaviour that is handled by PageHeader.

So, on one side, you have to make space for the header, on the other side, you should (must, I'd say) still let the developer decide a custom topMargin to implement whatever usecase he needs it for

[Alberto Mardegan (mardy)]

On 20/04/2016 16:29, Andrea Bernabei wrote:
> because topMargin should be available for the dev to use it for whatever
> he needs it to. The developer should not have to mind about setting it
> to accomodate a behaviour that is handled by PageHeader.
>
> So, on one side, you have to make space for the header, on the other
> side, you should (must, I'd say) still let the developer decide a custom
> topMargin to implement whatever usecase he needs it for

But the documentation clearly states (and it has always been like this):
"The topMargin of the flickable will automatically be updated to always
match the height of the header."

[Andrea Bernabei (faenil)]

I will stop here as, as I said, I'm not the maintainer of that component, nor am I in the SDK team :)

I've just been involved in the discussion so I wanted to provide some info about why that works the way it does.

The documentation, to me, looks outdated :)

[Tim Peeters (tpeeters)]

As Andrea said, this is the expected behavior.

The PageHeader will update the topMargin of the flickable that you set in order to ensure enough space for the header when it is visible. In addition to that, you set another topMargin inside the Flickable. We had to make a choice to either let the app developer always set the topMargin in the apps, or do it automatically when you set PageHeader.flickable. We chose the latter. There were some bugs related to that which were fixed in the MR you linked which probably caused the issues for you, but changing the behavior back will re-introduce those bugs.

I'm not sure what the use case of this construction is, but perhaps this would work as a solution for you: http://paste.ubuntu.com/15948932/
There I add the PageHeader inside the ListView delegate so that each of the Items in the ListView has its own header instead of sharing one header that shows/hides depending on what is the currently selected item in the ListView.

[Alberto Mardegan (mardy)]

Hi Tim, I don't think that this behavioural change is related to the bugs fixed by that MP. I believe that those bugs can be fixed, while retaining the old (and documented) behaviour of setting the flickable margin to the height of the header. All what those bugs say, is about the margin behaviour when then flickable is *not* bound to the header (or is bound to an invisible header); but in fact, in fixing those bugs you also changed the behaviour of the flickable margin when it's attached to a visible header. I think that this part of the change could and should be left out.

Do we actually have any bugs filed by developers complaining that the flickable's topMargin is being overwritten when the flickable is attached to a header? If we don't let's not fix it. Besides, it's documented to behave like that, so I really see no point in changing it now.

[Tim Peeters (tpeeters)]

Those bugs could not be fixed without changing the behavior. The problem was that we stored the old topMargin and set that back when a flickable was unset from the Header. However, there are cases (even in our UITK gallery, outside of our control depending on the QML engine) when Header instances are destroyed and/or (un)set the flickable property, there may be different headers setting conflicting values for the same Flickable.topMargin.

Also it was requested (Andrea initially brought it up I think) to not undo an existing flickable.topMargin that an app sets, but instead simply add the Header height to it. We don't have a bug report for that though.

[Zsombor Egri (zsombi)]

Alberto, may I ask what is the reason you are settjng the ListView delegate's flickable the one to control the header movement? That seems to be a bit of a bad idea tbh...

[Tim Peeters (tpeeters)]

The attached MR will clarify the behavior by updating the Header docs.

[Alberto Mardegan (mardy)]

Hi guys! I have to say, that I completely disagree with the proposed solution and would like us to stop for a moment and think about the issue again. I'm attaching a video which shows how the application is working on the current OTA and how I think it should continue working.

The thing I want to stress is that I'm using the UITK in a proper way, I'm not exploiting any undefined behaviour, but actually relying and the *documented* behaviour.

Now, if you think that reverting to the old behaviour is not possible without also reopening the three bugs which were fixed with that MP, I would like to ask you to give me a chance to try and see if it's possible to get the best of both worlds: fix those three bugs, while still retaining the old documented behaviour. I believe it should be possible, and I'm willing to use my free time to work on that.

The other question is how wrong the old behaviour is, and how many people were affected by it. First of all, we have no bug about it, which leads me to think that nobody found it wrong. If I correctly understood Andrea's concerns he wrote in IRC, his use case is very different: he was saying that when the header is hidden, the original topMargin of the flickable should be retained. I fully agree with him, and I see no contradiction: I believe that both this issue and the issue I'm having could be fixed.

In short, the behaviour I'm proposing is:
1) When the flickable is not bound to a header, or is bound to an invisible header, don't mess with its topMargin
2) When the flickable is bound to a visible header, its topMargin is set to the header's height

If some people (do we actually have any?) think that #2 is wrong, let's introduce an additional property retainFlickableMargins which if enabled will toggle the new behaviour (summing the initial flickable topMargin and the header height). But I seriously doubt that it's a needed thing.

[Alberto Mardegan (mardy)]

For a comparison, here's how the application would work with the new UITK and Tim's workaround. I don't think it's an improvement at all, especially because the back page action repeated on all items is rather confusing (am I scrolling through a page stack here? Will I go to the previous element if I press it, or what?).

[Tim Peeters (tpeeters)]

The desired behavior in your video can easily be accomplished by keeping your old code, and NOT setting the flickable of the PageHeader. Actually I think that is the best solution for the users too because otherwise the header may be showing/hiding while you swipe left/right.

About your proposal,
1) I think nobody disagrees with that
2) I think the use case you mention where the topMargin is maintained when the header is hidden conflicts with this. In this case, I interpret the hidden header as a header that goes away by scrolling, so its exposed property becomes false. The header should *not* change the topMargin when exposed changes, because changes in topMargin may change exposed again, and it will cause the page contents to jump around when the header hides.

Another issue with 2) is the implementation. It means that when you unset a header.flickable, it will restore the previous topMargin. So what happens when you have this case (which happens in practice when apps have multiple headers on a Page (for example an edit header and a regular header, of which only one is visible).

Say, we have this QML code:
Page {
   id: page
  PageHeader { id: header1; visible: page.header==header1; flickable: pageFlickable }
  PageHeader { id: header2; visible: page.header==header2; flickable: pageFlickable }
  header: editMode ? header2 : header1
  property bool editMode: false

  Flickable { id: pageFlickable }
}

Internally, header1 and header2 have a property "previousHeaderHeight" that they use to restore the previous header height when flickable is unset or the header becomes invisible.
Let's assume that header1 is always updated first by the qml engine, and then header2 (that happened in my case).

1. editMode = true
// header 1 first reverts the previous header height
// header1: visible = false; pageFlickable.topMargin = 0;
// header2: visible = true; previousHeaderHeight = 0; pageFlickable.topMargin = header2.height;

2. editMode = false
// header1: visible = true; previousHeaderHeight = header2.height; pageFlickable.topMargin = header1.height;
// header2: visible = false; pageFlickable.topMargin = 0 (previousHeaderHeight);

So now we have the wrong topMargin because we don't know in which order the properties of the different headers will be updated. The solution to this is to make the headers not store previousHeaderHeight, or to set the topMargin to header.height but to add to the topMargin when a header is enabled with a flickable, and subtract the header height from it when the header is invisible or the flickable is unset.

[Alberto Mardegan (mardy)]

Hi Tim, I attached a branch which should give you an idea of how I think the problem can be solved. Beware, I didn't test it and it's not complete (it doesn't adjust contentY yet), it's just to be used as a starting point.

In that branch I'm attaching some extra data to the flickable (using setProperty; but other methods, such a static QHash<QQuickFlickable*,HeaderData> are also possible -- I used setProperty because then I don't have to care about memory management):
- The original value of topMargin, before any header was bound to the flickable
- The number of headers currently bound to the flickable

This makes it easy to restore the flickable's margin once the last header detaches from it.

Please let me know if you'd like me to continue working on this branch, or if you'd like to take it over yourself, or if you completely disagree with this solution. I don't see any reason why something like this wouldn't work but, as I wrote, I didn't test it, so maybe I'm missing some big point.

[Tim Peeters (tpeeters)]

Alberto, I disagree with your solution. I think the behavior that we have now is correct because it keeps the original topMargin of the flickable for the part of the flickable that will be visible below the header (in y-direction).

Your video shows a very special case: A Page with a header, and a horizontal ListView in which there are vertical Flickables that are attached to the Page.header (depending on which Flickable is the current item in the ListView). Personally in that case I would use a locked header (don't link it to a flickable), but I think you should talk to design about it. If this were to become a common pattern, I would like to see design guidelines on how to deal with this, and then we can discuss the technical aspects of the proposal.

[Alberto Mardegan (mardy)]

Tim, I understand that you are a bit of a perfectionist, and that's very good :-) But here you want to "fix" a bug that no one reported, and that breaks a completely weird use-case that never actually worked (why would want to have additional margin below the header, that could not be implemented with a margin in the content item?), and by doing so you are changing a well documented behaviour that was working, that at least one developer is using, and for which there isn't a perfect workaround.

That's all for the sake of API perfection?

That's not how a toolkit should work.

[Tim Peeters (tpeeters)]

@Alberto, I proposed before that you lock your header so that it does not auto-hide, because if your header is not locked, and you change the header.flickable while the user is scrolling horizontally in the ListView, you will have weird header behavior where it jumps in and out of the view while scrolling horizontally. That seems bad UX to me, and that's why I recommended you discuss it with design. Can you tell me if/why this is a problem for your use-case?

[Alberto Mardegan (mardy)]

On 04/26/2016 06:18 PM, Tim Peeters wrote:
> @Alberto, I proposed before that you lock your header so that it does
> not auto-hide, because if your header is not locked, and you change the
[...]

I don't think that a meeting with the designers is necessary, and I can
certainly live with the fixed header; it's more a matter of principle,
but I don't intend to have a fight over it :-)

[Tim Peeters (tpeeters)]

After some more discussion on IRC, for Alberto's use case he will lock the header. I will close this bug. In UITK2 we probably will not try to automatically set the topMargin of the flickable, and leave it to the app developer so there will be full freedom (at the cost of a few extra lines of code in the apps).

[Alberto Mardegan (mardy)]

Could it be that this change has a bad effect when combined with the PullToRefresh item? I'm experiencing some weird behaviour (maybe the same as bug 1404884) on rc-proposed which I don't see on the OTA 10.
I hope to get some time tomorrow to investigate this better and produce a test case, but this is just a heads up.

[Launchpad Janitor (janitor)]

This bug was fixed in the package ubuntu-ui-toolkit - 1.3.1984+16.10.20160527.2

---------------
ubuntu-ui-toolkit (1.3.1984+16.10.20160527.2) yakkety; urgency=medium

  [ Christian Dywan ]
  * Slimmer frame for TextFields: 0.5dp. Fixes LP: #1578190.

  [ Albert Astals Cid ]
  * Add override
    The override specifier (since C++11) specifies that a virtual function
    overrides another virtual function. In a member function declaration or
    definition, override ensures that the function is virtual and is overriding
    a virtual function from the base class.

  [ Tim Peeters ]
  * Fix reference error in PullToRefreshStyle. Fixes LP: #1582843
  * Mark Tab, Tabs, TabBar, PageHeadConfiguration, PageHeadSections,
    PageHeadState, ToolbarButton, ToolbarItems as deprecated in the
    documentation. Fixes LP: #1566735, LP: #1566741.

  [ CI Train Bot ]
  * Resync trunk.

 -- Zoltan Balogh <email address hidden> Fri, 27 May 2016 07:08:44 +0000