Merge lp:~zsombi/ubuntu-ui-toolkit/columnLayout into lp:ubuntu-ui-toolkit/staging
| Status: | Merged |
|---|---|
| Approved by: | Zsombor Egri on 2016-08-26 |
| Approved revision: | 2057 |
| Merged at revision: | 2081 |
| Proposed branch: | lp:~zsombi/ubuntu-ui-toolkit/columnLayout |
| Merge into: | lp:ubuntu-ui-toolkit/staging |
| Diff against target: |
2250 lines (+2108/-7) 16 files modified
debian/control (+1/-0) debian/control.gles (+1/-0) documentation/overview.qdoc (+16/-0) src/Ubuntu/Test/UbuntuTestCase13.qml (+1/-1) src/Ubuntu/UbuntuToolkit/UbuntuToolkit.pro (+8/-2) src/Ubuntu/UbuntuToolkit/privates/splitviewhandler.cpp (+137/-0) src/Ubuntu/UbuntuToolkit/privates/splitviewhandler_p_p.h (+52/-0) src/Ubuntu/UbuntuToolkit/splitview.cpp (+675/-0) src/Ubuntu/UbuntuToolkit/splitview_p.h (+160/-0) src/Ubuntu/UbuntuToolkit/splitview_p_p.h (+154/-0) src/Ubuntu/UbuntuToolkit/splitviewlayout.cpp (+252/-0) src/Ubuntu/UbuntuToolkit/ubuntutoolkitmodule.cpp (+5/-0) src/Ubuntu/UbuntuToolkit/ucmathutils_p.h (+4/-4) tests/unit/visual/tst_splitview.13.qml (+383/-0) tests/unit/visual/tst_splitview_page.13.qml (+143/-0) tests/unit/visual/tst_splitview_repeater.13.qml (+116/-0) |
| To merge this branch: | bzr merge lp:~zsombi/ubuntu-ui-toolkit/columnLayout |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| ubuntu-sdk-build-bot | continuous-integration | Approve on 2016-08-26 | |
| Christian Dywan | 2016-07-11 | Approve on 2016-08-18 | |
|
Review via email:
|
|||
Commit Message
SplitView to Ubuntu.
Description of the Change
TODO:
- make Page to work with SplitView.
FAILED: Continuous integration, rev:2046
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:2046
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:2046
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:2046
https:/
Executed test runs:
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
None: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:2047
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:2047
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:2047
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:2047
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:2047
https:/
Executed test runs:
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
None: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:2048
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:2048
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:2048
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:2048
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:2048
https:/
Executed test runs:
SUCCESS: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
None: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
| Christian Dywan (kalikiana) wrote : | # |
> + * In order a SplitView to show some content it must have at least one active layout
In order for a SplitView to show some content it must have at least one active layout
> + * shown differs on the space available), make sure the content of your view does take
shown differs depending on the available space), make sure the content of your view takes
+ * Page {
367 + * id: column1
368 + * height: parent.height
369 + * }
The height: requirement is not being explained and it's different to APL. I would expect the Page to fill the column by default. Is height: needed in that case? What if I do anchors.fill?
> + * can be used with a Repeater in case the content of the view columns
> + * doesn't need to be preserved between layout changes.
That's asking for trouble. It's basically saying "This will probably not do what you want, but try this anyway". We need to have an example that is safe to copy and doesn't lead to surprises like losing all your data when the columns change.
+ * Spacing between the view columns, also drives the ability to resize view columns.
> + * If the value is zero, the view cannot resize columns. If the value is > 0, the
> + * view can resize those columns which have their \l{ViewColumn:
> + * is less than the \l {ViewColumn:
Spacing between view columns.
A value > 0 enables resizing of columns with a \l {ViewColumn:
If spacing is 0 the columns can't be resized.
> 1256 + QList<SplitView
columnLayouts
> + // Q: should we reset the sizes of the previous layout?
> + // at least it feels right to preserve the last state of the layout...
We need to define how sizes are restored. Since the user of SplitView is not supposed to set width: it's probably up to the component. But that would mean the component also has to save and restore, as the app isn't expected to touch that. We don't know where and how a particular app saves its state so we can't really do that.
> +static qreal clamp(qreal min, qreal max, qreal v)
Why not use clamp() from our MathUtils? It's identical except for using the real type versus Qt's alias.
| Zsombor Egri (zsombi) wrote : | # |
> > + * In order a SplitView to show some content it must have at least one
> active layout
> In order for a SplitView to show some content it must have at least one active
> layout
>
> > + * shown differs on the space available), make sure the content of your
> view does take
> shown differs depending on the available space), make sure the content of your
> view takes
>
> + * Page {
> 367 + * id: column1
> 368 + * height: parent.height
> 369 + * }
>
> The height: requirement is not being explained and it's different to APL. I
> would expect the Page to fill the column by default. Is height: needed in that
> case? What if I do anchors.fill?
Actually Page causes trouble, because of its anchoring... I HATE THAT!... I need to figure out something so it works with Page.
The height is needed in a sense that - if you want - you can create a component which would vertically be centered, or on the top/bottom of the column. Not just a Page, but a simple Item. I would not set the height by default, I would avoid any automatic height setting so we don't repeat the Page problem... But definitely anchors.fill should also be avoided! So I will document this! Good catch!
>
> > + * can be used with a Repeater in case the content of the view columns
> > + * doesn't need to be preserved between layout changes.
>
> That's asking for trouble. It's basically saying "This will probably not do
> what you want, but try this anyway". We need to have an example that is safe
> to copy and doesn't lead to surprises like losing all your data when the
> columns change.
If you have a Repeater, that's what you get. Perhaps we don't mention that at all then... Anyways, I have a test for that so in case someone asks us, we can point them to that. And I'll have a test with Pages!!!
>
> + * Spacing between the view columns, also drives the ability to resize view
> columns.
> > + * If the value is zero, the view cannot resize columns. If the value is >
> 0, the
> > + * view can resize those columns which have their
> \l{ViewColumn:
> > + * is less than the \l {ViewColumn:
>
>
> Spacing between view columns.
>
> A value > 0 enables resizing of columns with a \l
> {ViewColumn:
> {ViewColumn:
> If spacing is 0 the columns can't be resized.
Thanks :)
>
> > 1256 + QList<SplitView
> columnLayouts
>
> > + // Q: should we reset the sizes of the previous layout?
> > + // at least it feels right to preserve the last state of the layout...
>
> We need to define how sizes are restored. Since the user of SplitView is not
> supposed to set width: it's probably up to the component. But that would mean
> the component also has to save and restore, as the app isn't expected to touch
> that. We don't know where and how a particular app saves its state so we can't
> really do that.
Actually when I wrote this Q I (almost) immediately answered it myself. So, now any manual (with mouse or touch) resizing of the columns will be preserved in the ViewColumns, and restores when the layout i...
FAILED: Continuous integration, rev:2049
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:2049
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:2049
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:2049
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:2049
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
FAILURE: https:/
None: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:2051
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:2051
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:2051
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:2051
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:2051
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
FAILURE: https:/
None: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
| Christian Dywan (kalikiana) wrote : | # |
Please make tst_splitview{
Also testHandle appears to be untested - it exists but it's never being used.
> "handle delegate not an Item"
Minor nitpick: could we avoid repeating this half a dozen times, like we do for eg. deprecation warnings?
FAILED: Continuous integration, rev:2052
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:2052
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:2052
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:2052
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:2052
https:/
Executed test runs:
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
None: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:2053
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:2053
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:2053
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:2053
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:2053
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
FAILURE: https:/
None: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
| Christian Dywan (kalikiana) wrote : | # |
In file included from privates/
privates/
QPointer<
privates/
privates/
Curiously this only fails to compile with gles.
| Christian Dywan (kalikiana) wrote : | # |
Thanks for making the tests manually testable and incorporating the handle.
FAILED: Continuous integration, rev:2053
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:2053
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:2053
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:2053
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Autolanding.
More details in the following jenkins job:
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
FAILURE: https:/
None: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
FAILED: Continuous integration, rev:2053
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:2053
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:2053
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:2053
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Autolanding.
More details in the following jenkins job:
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
FAILURE: https:/
None: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
PASSED: Continuous integration, rev:2053
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:2053
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:2053
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:2053
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:2053
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Autolanding.
More details in the following jenkins job:
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
FAILURE: https:/
None: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
- 2056. By Zsombor Egri on 2016-08-25
-
staging sync
- 2057. By Zsombor Egri on 2016-08-26
-
fix build issues on gles
PASSED: Continuous integration, rev:2057
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:2057
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:2057
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:2057
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:2057
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
None: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:2057
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:2057
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:2057
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:2057
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:2046
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/