Merge into trunk : fixBookmarkDesign : Code : webbrowser-app

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	continuous-integration		Needs Fixing on 2015-02-24
Olivier Tilloy		2015-01-24	Approve on 2015-02-04
Review via email: mp+247512@code.launchpad.net

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-01-24:

#

FAILED: Continuous integration, rev:875
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1389/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/971
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-vivid/446
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/147
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/147
        deb: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/147/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/147
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/860
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/969
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/969/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17414
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-vivid/367
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-amd64/538
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-amd64/538/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1389/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-01-26:

#

After applying the change, this is how it looks on my desktop (utopic): http://people.canonical.com/~osomon/multi-select.png.

There are a few things that need adjusting still, wrt the visual spec:
- icons in the top toolbar are scaled up, they shouldn’t be
- icons in the top toolbar need to have a left and right margin to be aligned with the rest of the items on screen
- the horizontal line that underlines each section header should be aligned to the left with the checkboxes, and to the right it should have a 2GU margin
- the text of the section headers should also be aligned to the left with the checkboxes (i.e. a 2GU left margin)
- there should be 2GUs between the horizontal line and the first item in a section (there currently is only one)
- there should be 2.5GUs between the last item in a section and the text of the following header

review: Needs Fixing

Revision history for this message

Riccardo Padovani (rpadovani) wrote on 2015-01-26:

#

> After applying the change, this is how it looks on my desktop (utopic):
> http://people.canonical.com/~osomon/multi-select.png.
>
> There are a few things that need adjusting still, wrt the visual spec:
> - icons in the top toolbar are scaled up, they shouldn’t be

Sorry about this, I'm on vivid and it works well, I think it's due this commit[0] in the sdk, probably hasn't been backported yet. Anyway, I reworked the Icon, so now should be ok.

> - icons in the top toolbar need to have a left and right margin to be aligned
> with the rest of the items on screen

Fixed

> - the horizontal line that underlines each section header should be aligned
> to the left with the checkboxes, and to the right it should have a 2GU margin

fixed

> - the text of the section headers should also be aligned to the left with the
> checkboxes (i.e. a 2GU left margin)

fixed

> - there should be 2GUs between the horizontal line and the first item in a
> section (there currently is only one)

Fixed

> - there should be 2.5GUs between the last item in a section and the text of
> the following header

About this, I didn't find any way to find if a section it's the first, so I set a negative margin to the listview to compensate the topmargin the first section has. There is a better way to implement that?

[0]https://bazaar.launchpad.net/~ubuntu-sdk-team/ubuntu-ui-toolkit/trunk/revision/1115

lp:~rpadovani/webbrowser-app/fixBookmarkDesign updated on 2015-01-26

876. By Riccardo Padovani on 2015-01-26: addressed osomon's comments

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-01-26:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1393/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Francis Ginther (fginther) wrote on 2015-01-26:

#

The jenkins node for the amd64 build failed, I've restarted a new ci run.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-01-27:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1399/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Riccardo Padovani (rpadovani) wrote on 2015-01-28:

#

The failure seems unrelated to the branch (maybe is due [0]?).
Anyway, if [1] lands the test shouldn't fail again.

[0] https://bugs.launchpad.net/webbrowser-app/+bug/1413194
[1] https://code.launchpad.net/~rpadovani/webbrowser-app/addressBarFullWidth/+merge/239039

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-01-28:

#

The test failure is unrelated indeed.

This looks much better now. A bunch of additional minor remarks:

- When in multi-selection mode, the checkbox is shifted a bit to the right (between 0.5 and 1 GU), and is not aligned with the section header

- According to the visual spec, the background of the toolbar seems to be white

- When in multi-selection mode, scrolling the list up makes it overlap with the toolbar. Can the toolbar be made to appear on top of the list?

- When an entry is selected, it’s highlighted, but it’s not in the visual spec

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-01-28:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1405/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Riccardo Padovani (rpadovani) wrote on 2015-01-28:

#

> This looks much better now. A bunch of additional minor remarks:
>
> - When in multi-selection mode, the checkbox is shifted a bit to the right
> (between 0.5 and 1 GU), and is not aligned with the section header

Wow, I didn't notice this! I spend some time to understand the difference, but I fixed it now

> - According to the visual spec, the background of the toolbar seems to be
> white

Fixed

>
> - When in multi-selection mode, scrolling the list up makes it overlap with
> the toolbar. Can the toolbar be made to appear on top of the list?

Fixed

> - When an entry is selected, it’s highlighted, but it’s not in the visual
> spec

I fixed that, but there is a little color change, because you asked[0] a response when there user click on something, so there is a color change on mouseArea.pressed.

Atm, I don't have an idea to avoid this color change... Suggestions?

[0]https://code.launchpad.net/~rpadovani/webbrowser-app/1351167/+merge/238155/comments/585299

lp:~rpadovani/webbrowser-app/fixBookmarkDesign updated on 2015-01-28

877. By Riccardo Padovani on 2015-01-28: Addressed some issues raised by oSoMoN

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-01-28:

#

That looks great now, very close to the visual design spec.

For the implementation of the buttons in the topBar, couldn’t you use the ToolbarButton standard component (http://developer.ubuntu.com/api/qml/sdk-14.10/Ubuntu.Components.ToolbarButton/) ?
If not, you should probably make them an AbstractButton that contains both Icon and a Label (instead of Text), and have the font size defined by the 'fontSize' property (http://developer.ubuntu.com/api/qml/sdk-14.10/Ubuntu.Components.Label/#fontSize-prop).

lp:~rpadovani/webbrowser-app/fixBookmarkDesign updated on 2015-01-28

878. By Riccardo Padovani on 2015-01-28: refactoring topbar in history page

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-01-28:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1409/rebuild

review: Needs Fixing (continuous-integration)

lp:~rpadovani/webbrowser-app/fixBookmarkDesign updated on 2015-01-28

879. By Riccardo Padovani on 2015-01-28: Fixing actions

Revision history for this message

Riccardo Padovani (rpadovani) wrote on 2015-01-28:

#

You're totally right, now the code is cleaner!

But, for some reasons, onTriggered doesn't work, so I had to use MouseArea... Hope it's good in any case!

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-01-29:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1413/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-01-29:

#

Ah, sorry for suggesting the use of ToolbarButton, it is apparently deprecated (or meant to be anyway, I don’t see any reference to it being obsolete in the online docs, but I clearly remember a conversation with the UITK folks in which they said it shouldn’t be used anymore).

I now remember that I wrote a custom ToolbarAction component (src/app/webbrowser/ToolbarAction.qml) which does exactly what you need, can you use that one instead, for consistency across the code base? When the UITK provides a component we can use, we’ll just remove the custom one.

See how to use it here: http://bazaar.launchpad.net/~phablet-team/webbrowser-app/trunk/view/head:/src/app/webbrowser/HistoryView.qml#L285

lp:~rpadovani/webbrowser-app/fixBookmarkDesign updated on 2015-02-01

880. By Riccardo Padovani on 2015-02-01: Replaced ToolbarButton with ToolbarAction

Revision history for this message

Riccardo Padovani (rpadovani) wrote on 2015-02-01:

#

Updated :-)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-02-01:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1427/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-02-02:

#

Thanks!

Sorry if it looks like I’m picking nits here, but the anchor changes in ListItemWithActions.qml don’t sound quite right to me: this is a generic component that shouldn’t have to know the details of its contents. Instead, I’m pretty sure there is a way to ensure the checkbox is aligned with the rest of the list view in all cases, regardless of what the delegate actually contains.

lp:~rpadovani/webbrowser-app/fixBookmarkDesign updated on 2015-02-03

881. By Riccardo Padovani on 2015-02-03: Removed modifications from ListItemWithActions.qml

Revision history for this message

Riccardo Padovani (rpadovani) wrote on 2015-02-03:

#

No, you're totally right, I shouldn't modify upstream component if isn't strictly needed (as for color).

I removed the anchors edit and implemented it in HistoryView.qml

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-02-03:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1437/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-02-04:

#

That looks great now, thanks!

One last tiny suggestion, and it will be good to merge: in topBar, the item that contains the actions doesn’t need to be a Rectangle, as it has nothing to actually draw, it could be replaced by an Item. And instead of setting its width and height, just use anchors.

lp:~rpadovani/webbrowser-app/fixBookmarkDesign updated on 2015-02-04

882. By Riccardo Padovani on 2015-02-04: Addressed osomon's comment about anchors and rectangle/item

Revision history for this message

Riccardo Padovani (rpadovani) wrote on 2015-02-04:

#

Done :-)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-02-04:

#

Perfect now :)

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-02-04:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1441/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-02-24:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1486/rebuild

review: Needs Fixing (continuous-integration)

webbrowser-app

Merge lp:~rpadovani/webbrowser-app/fixBookmarkDesign into lp:webbrowser-app

Commit message

Description of the change

Preview Diff

Subscribers

 === modified file 'src/app/webbrowser/HistorySectionDelegate.qml'
 --- src/app/webbrowser/HistorySectionDelegate.qml	2014-10-15 19:14:09 +0000
 +++ src/app/webbrowser/HistorySectionDelegate.qml	2015-02-25 07:53:54 +0000
@@ -21,13 +21,14 @@
  import Ubuntu.Components.ListItems 1.0 as ListItem
  Item {
--    height: units.gu(3)
++    height: units.gu(6.5)
      Label {
          anchors {
              left: parent.left
              right: parent.right
              top: parent.top
++            topMargin: units.gu(2.5)
+         }
          height: units.gu(2)
@@ -57,6 +58,7 @@
              left: parent.left
              right: parent.right
              bottom: parent.bottom
++            bottomMargin: units.gu(1)
+         }
+     }
+ }
 === modified file 'src/app/webbrowser/HistoryView.qml'
 --- src/app/webbrowser/HistoryView.qml	2014-11-12 11:04:34 +0000
 +++ src/app/webbrowser/HistoryView.qml	2015-02-25 07:53:54 +0000
@@ -35,68 +35,6 @@
          color: "#f6f6f6"
+     }
--    Item {
--        id: topBar
--        visible: domainsListView.isInSelectionMode
--
--        onVisibleChanged: visible ? topBarOpenAnimation.start() : topBarCloseAnimation.start()
--
--        anchors { left: parent.left; right: parent.right; top: parent.top }
--
--        Icon {
--            name: "close"
--            anchors {
--                top: parent.top;
--                bottom: parent.bottom;
--                left: parent.left;
--                margins: units.gu(1)
--            }
--            width: height
--            MouseArea {
--                anchors.fill: parent
--                onClicked: domainsListView.cancelSelection()
--            }
--        }
--        Icon {
--            name: "select"
--            anchors {
--                top: parent.top;
--                bottom: parent.bottom;
--                right: deleteIcon.left;
--                margins: units.gu(1)
--            }
--            width: height
--            MouseArea {
--                anchors.fill: parent
--                onClicked: {
--                    if (domainsListView.selectedItems.count === domainsListView.count) {
--                        domainsListView.clearSelection()
--                    } else {
--                        domainsListView.selectAll()
--                    }
--                }
--            }
--        }
--        Icon {
--            id: deleteIcon
--            enabled: domainsListView.selectedItems.count > 0
--            name: "delete"
--            anchors {
--                top: parent.top;
--                bottom: parent.bottom;
--                right: parent.right;
--                margins: units.gu(1)
--            }
--            width: height
--            MouseArea {
--                anchors.fill: parent
--                onClicked: {
--                    domainsListView.endSelection()
--                }
--            }
--        }
--    }
--
      UbuntuNumberAnimation {
          id: topBarOpenAnimation
          target: topBar
@@ -123,7 +61,8 @@
              left: parent.left
              right: parent.right
              bottom: toolbar.top
--            margins: units.gu(2)
++            topMargin: units.gu(-0.5) // topMargin 2 - firstSection.topMargin 2.5
++            rightMargin: units.gu(2)
+         }
          listModel: HistoryDomainListChronologicalModel {
@@ -136,7 +75,9 @@
          section.property: "lastVisitDate"
          section.delegate: HistorySectionDelegate {
--            width: parent.width
++            width: parent.width - units.gu(2)
++            anchors.left: parent.left
++            anchors.leftMargin: units.gu(2)
+         }
          onSelectionDone: {
@@ -163,6 +104,16 @@
                  removalAnimation.start()
+             }
++            anchors {
++                left: parent.left
++                // we need to move left the favicon to align the favicon to
++                // other elements. Favicon has a container bigger than it.
++                // units.gu(3) it's the size of the favicon container
++                // units.dp(16) it's the size of the favicon
++                // the favicon is hCentered in the container
++                leftMargin: selectionMode ? - (units.gu(3) - units.dp(16)) / 2 : 0
++            }
++
              selectionMode: domainsListView.isInSelectionMode
              selected: domainsListView.isSelected(urlDelegate)
@@ -299,4 +250,87 @@
+             }
+         }
+     }
++
++    Item {
++        id: topBar
++        visible: domainsListView.isInSelectionMode
++
++        onVisibleChanged: visible ? topBarOpenAnimation.start() : topBarCloseAnimation.start()
++
++        anchors { left: parent.left; right: parent.right; top: parent.top }
++
++        Rectangle {
++            width: parent.width
++            height: parent.height + units.gu(1.5)
++            color: "white"
++        }
++
++        Item {
++            anchors {
++                top: parent.top
++                left: parent.left
++                leftMargin: units.gu(2)
++                bottom: parent.bottom
++                right: parent.right
++                rightMargin: units.gu(2)
++            }
++
++            ToolbarAction {
++                iconName: "back"
++                text: i18n.tr("Cancel")
++
++                MouseArea {
++                    anchors.fill: parent
++                    onClicked: domainsListView.cancelSelection()
++                }
++
++                anchors.left: parent.left
++
++                height: parent.height
++                width: height
++            }
++
++            ToolbarAction {
++                iconName: "select"
++                text: i18n.tr("Select all")
++
++                MouseArea {
++                    anchors.fill: parent
++                    onClicked: {
++                        if (domainsListView.selectedItems.count === domainsListView.count) {
++                            domainsListView.clearSelection()
++                        } else {
++                            domainsListView.selectAll()
++                        }
++                    }
++                }
++
++                anchors {
++                    right: deleteButton.left
++                    rightMargin: units.gu(2)
++                }
++
++                height: parent.height
++                width: height
++            }
++
++            ToolbarAction {
++                id: deleteButton
++
++                iconName: "delete"
++                text: i18n.tr("Delete")
++                enabled: domainsListView.selectedItems.count > 0
++
++                MouseArea {
++                    anchors.fill: parent
++                    onClicked: domainsListView.endSelection()
++                }
++
++                anchors.right: parent.right
++
++                height: parent.height
++                width: height
++            }
++        }
++    }
+ }
 === modified file 'src/app/webbrowser/upstreamcomponents/ListItemWithActions.qml'
 --- src/app/webbrowser/upstreamcomponents/ListItemWithActions.qml	2014-10-17 10:30:33 +0000
 +++ src/app/webbrowser/upstreamcomponents/ListItemWithActions.qml	2015-02-25 07:53:54 +0000
@@ -219,7 +219,7 @@
+         }
          width: parent.width
--        color: root.selected || (mouseArea.pressed && swipeState === "Normal" )? root.selectedColor : root.color
++        color: mouseArea.pressed && swipeState === "Normal" ? root.selectedColor : root.color
          Loader {
              id: selectionIcon