ubuntu-system-settings

Merge lp://staging/~seb128/ubuntu-system-settings/updates-column-layout into lp://staging/ubuntu-system-settings

updates-column-layout
Merge into trunk

Proposed by Sebastien Bacher on 2014-08-20

Status:

Merged

Approved by:

Sebastien Bacher on 2014-08-25

Approved revision:

929

Merged at revision:

941

Proposed branch:

lp://staging/~seb128/ubuntu-system-settings/updates-column-layout

Merge into:

lp://staging/ubuntu-system-settings

Diff against target:

720 lines (+320/-333)

2 files modified

plugins/system-update/PageComponent.qml (+320/-329)
tests/autopilot/ubuntu_system_settings/tests/test_system_updates.py (+0/-4)

To merge this branch:

bzr merge lp://staging/~seb128/ubuntu-system-settings/updates-column-layout

Low

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Sebastien Bacher (community)			Approve on 2014-08-25
PS Jenkins bot	continuous-integration		Needs Fixing on 2014-08-25
Diego Sarmentero (community)		2014-08-20	Approve on 2014-08-25
Ken VanDine		2014-08-20	Pending
Review via email: mp+231590@code.staging.launchpad.net

Commit message

[system-update] rework the ui to use a column rather than anchors,
the layout is quite dynamic and it should be easier to position and
stack widgets this way

Description of the change

[system-update] rework the ui to use a column rather than anchors,
the layout is quite dynamic and it should be easier to position and
stack widgets this way

Revision history for this message

Sebastien Bacher (seb128) wrote on 2014-08-20:

Sending for review but there is an issue with the listview height computation (it's visible when there is only a system image available for example), still I would welcome comments on the approach/suggestions

The layout is a bit dynamic, not sure that the column is the best way to handle it.

There are basically several "groups"
- one with the top "actions" (the "checking for update" spinner and the "install" button)
- the list of updates
- an "error group" (when e.g there is no u1 account configured)
- the bottom item to select the download mode

some of the those elements might, or not, be on screen, which makes tricky to computer the listview height (the current code should work but seems a bit off, if somebody has a clue why, that would be nice)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-08-20:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/1260/rebuild

review: Approve (continuous-integration)

Revision history for this message

Ken VanDine (ken-vandine) wrote on 2014-08-21:

> Sending for review but there is an issue with the listview height computation
> (it's visible when there is only a system image available for example), still
> I would welcome comments on the approach/suggestions
>
> The layout is a bit dynamic, not sure that the column is the best way to
> handle it.
>
> There are basically several "groups"
> - one with the top "actions" (the "checking for update" spinner and the
> "install" button)
> - the list of updates
> - an "error group" (when e.g there is no u1 account configured)
> - the bottom item to select the download mode
>
> some of the those elements might, or not, be on screen, which makes tricky to
> computer the listview height (the current code should work but seems a bit
> off, if somebody has a clue why, that would be nice)

I'd suggest getting rid of the ListView and just using a Column with a Repeater. You only want a ListView if you want it to be a separate flickable. In this case we probably don't want that, we want the whole page to be the flickable.

Revision history for this message

Sebastien Bacher (seb128) wrote on 2014-08-22:

> In this case we probably don't want that, we want the whole page to be the flickable.

Not sure, we at least want the bottom item to stay in place, but we can probably make the column flickable in that case?

Revision history for this message

Ken VanDine (ken-vandine) wrote on 2014-08-22:

If you do want to keep the bottom item in place, I guess a ListView is fine or you could put a Column inside of a Flickable. I guess if you reliably know the height, a ListView works well. So you can just anchor the ListView on to the bottom item.

lp://staging/~seb128/ubuntu-system-settings/updates-column-layout updated on 2014-08-22

926. By Sebastien Bacher on 2014-08-22: Extra tweaks to the layout

Revision history for this message

Sebastien Bacher (seb128) wrote on 2014-08-22:

Ok, another update that seems to work fine now, without having to compute height values.
I went back to use a listview because it makes easier to keep the "install <n> updates" button callback (which iterates through the list and active the items)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-08-22:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/1285/rebuild

review: Needs Fixing (continuous-integration)

lp://staging/~seb128/ubuntu-system-settings/updates-column-layout updated on 2014-08-22

927. By Sebastien Bacher on 2014-08-22: Restore line cleared which is still needed
928. By Sebastien Bacher on 2014-08-22: Restore other line

Revision history for this message

Sebastien Bacher (seb128) wrote on 2014-08-22:

(the recent commits should fix the CI, waiting for another run)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-08-22:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/1287/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Sebastien Bacher (seb128) wrote on 2014-08-25:

CI failed on a system settling "regression", which seems weird, I did a retry

Revision history for this message

Diego Sarmentero (diegosarmentero) wrote on 2014-08-25:

I think for the singular should be:
i18n.tr("Install %1 update", "Install %1 update", root.updatesAvailable).arg(root.updatesAvailable)
or just:
i18n.tr("Install 1 update", "Install 1 update", root.updatesAvailable).arg(root.updatesAvailable)

Except that, everything looks good! Great work

review: Needs Fixing

Revision history for this message

Diego Sarmentero (diegosarmentero) wrote on 2014-08-25:

review: Approve

lp://staging/~seb128/ubuntu-system-settings/updates-column-layout updated on 2014-08-25

929. By Sebastien Bacher on 2014-08-25: use the correct singular, even if it's not displayed

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-08-25:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/1292/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-08-25:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/1294/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Sebastien Bacher (seb128) wrote on 2014-08-25:

the CI issue is in the SIM code, nothing to do with the changes here, not blocking the approval on those

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes:

Sebastien Bacher

Ubuntu Touch System Settings