ubuntu-touch-coreapps-reviewers team mailing list archive
-
ubuntu-touch-coreapps-reviewers team
-
Mailing list archive
-
Message #08431
Re: [Merge] lp:~nik90/ubuntu-calendar-app/migrate-listitemlayout into lp:ubuntu-calendar-app
Review: Needs Fixing
Looking good, just a couple of comments inline :-)
Diff comments:
> === modified file 'CalendarChoicePopup.qml'
> --- CalendarChoicePopup.qml 2016-01-29 14:35:14 +0000
> +++ CalendarChoicePopup.qml 2016-02-26 12:57:09 +0000
> @@ -15,125 +15,131 @@
> * You should have received a copy of the GNU General Public License
> * along with this program. If not, see <http://www.gnu.org/licenses/>.
> */
> +
> import QtQuick 2.4
> +import QtOrganizer 5.0
> import Ubuntu.Components 1.3
> -import Ubuntu.Components.Popups 1.0
> -import Ubuntu.Components.ListItems 1.0 as ListItem
> -import QtOrganizer 5.0
> import Ubuntu.SyncMonitor 0.1
> +import Ubuntu.Components.Popups 1.3
>
> Page {
> - id: root
> + id: calendarChoicePage
> objectName: "calendarchoicepopup"
> - property var model;
>
> - signal collectionUpdated();
> + property var model
> + signal collectionUpdated()
>
> visible: false
> - title: i18n.tr("Calendars")
> -
> - head {
> - backAction: Action {
> - text: i18n.tr("Back")
> - iconName: "back"
> - onTriggered: {
> - root.collectionUpdated();
> - pop();
> - }
> - }
> - }
> -
> - head.actions: Action {
> - objectName: "syncbutton"
> - iconName: "reload"
> - // TRANSLATORS: Please translate this string to 15 characters only.
> - // Currently ,there is no way we can increase width of action menu currently.
> - text: enabled ? i18n.tr("Sync") : i18n.tr("Syncing")
> - onTriggered: syncMonitor.sync(["calendar"])
> - enabled: (syncMonitor.state !== "syncing")
> - visible: syncMonitor.enabledServices ? syncMonitor.serviceIsEnabled("calendar") : false
> + header: PageHeader {
> + title: i18n.tr("Calendars")
> + leadingActionBar.actions: [
> + Action {
> + text: i18n.tr("Back")
> + iconName: "back"
> + onTriggered: {
> + calendarChoicePage.collectionUpdated();
> + pop();
> + }
> + }
> + ]
NitPick: According to the QML coding conventions if a list contains only one element then omit the square brackets.
> + trailingActionBar.actions: [
> + Action {
> + objectName: "syncbutton"
> + iconName: "reload"
> + // TRANSLATORS: Please translate this string to 15 characters only.
> + // Currently ,there is no way we can increase width of action menu currently.
> + text: enabled ? i18n.tr("Sync") : i18n.tr("Syncing")
> + onTriggered: syncMonitor.sync(["calendar"])
> + enabled: (syncMonitor.state !== "syncing")
> + visible: syncMonitor.enabledServices ? syncMonitor.serviceIsEnabled("calendar") : false
> + }
> + ]
> }
>
> SyncMonitor {
> id: syncMonitor
> }
>
> - ListView {
> + UbuntuListView {
Why the change to UbuntuListView? Afaik the UbuntuListView is geared towards handling expandable delegates, which i don't see that happening in this view. UbuntuListView just extends upon ListView, so this is probably bringing some additional unneeded bindings/memory usage with no real gain.
> id: calendarsList
> - anchors.fill: parent
> -
> - footer: CalendarListButtonDelegate {
> +
> + anchors { top: calendarChoicePage.header.bottom; left: parent.left; right: parent.right; bottom: parent.bottom }
> +
> + header: ListItem {
> id: importFromGoogleButton
>
> visible: (onlineAccountHelper.status === Loader.Ready)
> - iconSource: "image://theme/google"
> - labelText: i18n.tr("Add online Calendar")
> + height: onlineCalendarLayout.height + divider.height
> +
> + ListItemLayout {
> + id: onlineCalendarLayout
> + title.text: i18n.tr("Add online Calendar")
> +
> + Image {
> + SlotsLayout.position: SlotsLayout.First
> + source: "image://theme/google"
> + width: units.gu(5)
> + height: width
> + }
> + }
> +
> onClicked: {
> onlineAccountHelper.item.setupExec()
> }
> }
>
> - model : root.model.getCollections();
> - delegate: ListItem.Standard {
> + model : calendarChoicePage.model.getCollections()
> + currentIndex: -1
> +
> + delegate: ListItem {
> id: delegateComp
> objectName: "calendarItem"
>
> - Rectangle {
> - id: calendarColorCode
> - objectName: "calendarColorCode"
> -
> - width: parent.height - units.gu(2)
> - height: width
> -
> - anchors {
> - left: parent.left
> - leftMargin: units.gu(2)
> - verticalCenter: parent.verticalCenter
> - }
> -
> - color: modelData.color
> - opacity: checkBox.checked ? 1.0 : 0.8
> -
> - MouseArea{
> - anchors.fill: parent
> - onClicked: {
> - //popup dialog
> - var dialog = PopupUtils.open(Qt.resolvedUrl("ColorPickerDialog.qml"),root);
> - dialog.accepted.connect(function(color) {
> - var collection = root.model.collection(modelData.collectionId);
> - collection.color = color;
> - root.model.saveCollection(collection);
> - })
> - }
> - }
> - }
> -
> - Label{
> - objectName: "calendarName"
> - text: modelData.name
> - elide: Text.ElideRight
> - opacity: checkBox.checked ? 1.0 : 0.8
> - color: UbuntuColors.midAubergine
> - width: parent.width - calendarColorCode.width - checkBox.width - units.gu(6) /*margins*/
> - anchors {
> - left: calendarColorCode.right
> - margins: units.gu(2)
> - verticalCenter: parent.verticalCenter
> - }
> - }
> -
> - control: CheckBox {
> - id: checkBox
> - objectName: "checkBox"
> - checked: modelData.extendedMetaData("collection-selected")
> - enabled: !root.isInEditMode
> - onCheckedChanged: {
> - modelData.setExtendedMetaData("collection-selected",checkBox.checked)
> - var collection = root.model.collection(modelData.collectionId);
> - root.model.saveCollection(collection);
> - }
> - }
> -
> + height: calendarsListLayout.height + divider.height
> +
> + ListItemLayout {
> + id: calendarsListLayout
> +
> + title.text: modelData.name
> + title.objectName: "calendarName"
> +
> + CheckBox {
> + id: checkBox
> + objectName: "checkBox"
> + SlotsLayout.position: SlotsLayout.Last
> + checked: modelData.extendedMetaData("collection-selected")
> + enabled: !calendarChoicePage.isInEditMode
> + onCheckedChanged: {
> + modelData.setExtendedMetaData("collection-selected",checkBox.checked)
> + var collection = calendarChoicePage.model.collection(modelData.collectionId);
> + calendarChoicePage.model.saveCollection(collection);
> + }
> + }
> +
> + Rectangle {
> + id: calendarColorCode
> + objectName: "calendarColorCode"
> +
> + SlotsLayout.position: SlotsLayout.First
> + width: units.gu(5)
> + height: width
> + color: modelData.color
> + opacity: checkBox.checked ? 1.0 : 0.8
> +
> + MouseArea{
> + anchors.fill: parent
> + onClicked: {
> + //popup dialog
> + var dialog = PopupUtils.open(Qt.resolvedUrl("ColorPickerDialog.qml"),calendarChoicePage);
> + dialog.accepted.connect(function(color) {
> + var collection = calendarChoicePage.model.collection(modelData.collectionId);
> + collection.color = color;
> + calendarChoicePage.model.saveCollection(collection);
> + })
> + }
> + }
> + }
> + }
> }
> }
>
>
> === modified file 'Settings.qml'
> --- Settings.qml 2016-02-03 08:06:25 +0000
> +++ Settings.qml 2016-02-26 12:57:09 +0000
> @@ -15,106 +15,71 @@
> * You should have received a copy of the GNU General Public License
> * along with this program. If not, see <http://www.gnu.org/licenses/>.
> */
> +
> import QtQuick 2.4
> import Ubuntu.Components 1.3
> -import Ubuntu.Components.Popups 1.0
> -import Ubuntu.Components.ListItems 1.0 as ListItem
>
> Page {
> - id: root
> + id: settingsPage
> objectName: "settings"
>
> visible: false
>
> - head {
> + header: PageHeader {
> title: i18n.tr("Settings")
> - backAction: Action {
> - text: i18n.tr("Back")
> - iconName: "back"
> - onTriggered: {
> - pop();
> + leadingActionBar.actions: [
Same nitpick as above :-D
> + Action {
> + text: i18n.tr("Back")
> + iconName: "back"
> + onTriggered: {
> + pop()
> + }
> }
> - }
> + ]
> }
>
> - ListModel{
> - id: model;
> + Component.onCompleted: {
> + weekCheckBox.checked = mainView.displayWeekNumber
> + lunarCalCheckBox.checked = mainView.displayLunarCalendar
> }
>
> Column {
> id: settingsColumn
> objectName: "settingsColumn"
> +
> spacing: units.gu(0.5)
> - anchors {
> - margins: units.gu(2)
> - fill: parent
> - }
> -
> - Item{
> - width: parent.width;
> - height: Math.max(weekNumber.height, weekCheckbox.height)
> -
> - Label{
> - id: weekNumber;
> - objectName: "weekNumber"
> - text: i18n.tr("Show week numbers");
> - elide: Text.ElideRight
> - opacity: weekCheckbox.checked ? 1.0 : 0.8
> - color: UbuntuColors.midAubergine
> - anchors {
> - left: parent.left
> - right: weekCheckbox.left;
> - margins: units.gu(2)
> - verticalCenter: parent.verticalCenter
> - }
> - }
> -
> - CheckBox {
> - id: weekCheckbox
> - objectName: "weekCheckBox"
> - anchors.right:parent.right;
> - onCheckedChanged: {
> - mainView.displayWeekNumber = weekCheckbox.checked;
> - }
> - }
> - }
> -
> - ListItem.ThinDivider {}
> -
> - Item{
> - width: parent.width;
> - height: Math.max(lunarCal.height, lunarCalCheckbox.height)
> -
> - Label{
> - id: lunarCal;
> - objectName: "lunarCalendar"
> - text: i18n.tr("Show lunar calendar");
> - elide: Text.ElideRight
> - opacity: lunarCalCheckbox.checked ? 1.0 : 0.8
> - color: UbuntuColors.midAubergine
> - anchors {
> - left: parent.left
> - right: lunarCalCheckbox.left;
> - margins: units.gu(2)
> - verticalCenter: parent.verticalCenter
> - }
> - }
> -
> - CheckBox {
> - id: lunarCalCheckbox
> - objectName: "lunarCalCheckbox"
> - anchors.right:parent.right;
> - onCheckedChanged: {
> - mainView.displayLunarCalendar = lunarCalCheckbox.checked
> - }
> - }
> - }
> -
> - ListItem.ThinDivider {}
> - }
> -
> - Component.onCompleted: {
> - weekCheckbox.checked = mainView.displayWeekNumber
> - lunarCalCheckbox.checked = mainView.displayLunarCalendar
> + anchors { top: settingsPage.header.bottom; left: parent.left; right: parent.right; bottom: parent.bottom }
> +
> + ListItem {
> + height: weekNumberLayout.height + divider.height
> + ListItemLayout {
> + id: weekNumberLayout
> + title.text: i18n.tr("Show week numbers")
> + CheckBox {
> + id: weekCheckBox
> + objectName: "weekCheckBox"
> + SlotsLayout.position: SlotsLayout.Last
> + onCheckedChanged: {
> + mainView.displayWeekNumber = weekCheckBox.checked
> + }
> + }
> + }
> + }
> +
> + ListItem {
> + height: lunarCalLayout.height + divider.height
> + ListItemLayout {
> + id: lunarCalLayout
> + title.text: i18n.tr("Show lunar calendar")
> + CheckBox {
> + id: lunarCalCheckBox
> + objectName: "lunarCalCheckbox"
> + SlotsLayout.position: SlotsLayout.Last
> + onCheckedChanged: {
> + mainView.displayLunarCalendar = lunarCalCheckBox.checked
> + }
> + }
> + }
> + }
> }
> }
--
https://code.launchpad.net/~nik90/ubuntu-calendar-app/migrate-listitemlayout/+merge/287309
Your team Ubuntu Calendar Developers is subscribed to branch lp:ubuntu-calendar-app.
References
