← Back to team overview

ubuntu-touch-coreapps-reviewers team mailing list archive

Re: [Merge] lp:~mzanetti/reminders-app/improve-viewer into lp:reminders-app

 

Review: Needs Fixing

[Tablet mode] Choose a note, then choose another note, the note preview goes over the header.

You forgot to remove a reference to a componenent you delete: src/app/qml/ui/EditNoteView.qml:35: ReferenceError: notebookSelector is not defined

[Tablet mode] Cannot create a reminders: qml: pushing reminderspage false
file:///home/rpadovani/Ubuntu/touch/core-apps/reminders/reminders-app/builddir/src/app/qml/ui/NoteView.qml:69: TypeError: Cannot call method 'push' of null

[Tablet mode] If the first note in the list isn't of the default notebook and you choose it, in the preview there is the notebook selector with the default notebook selected (I think is related to a diff comment I left)

Phone mode works as expected, and it's a nice improvement :-)

Diff comments:

> === modified file 'src/app/qml/components/EditTagsDialog.qml'
> --- src/app/qml/components/EditTagsDialog.qml	2015-06-11 22:45:32 +0000
> +++ src/app/qml/components/EditTagsDialog.qml	2015-06-17 21:22:37 +0000
> @@ -174,6 +174,7 @@
>                      else {
>                          root.note.tagGuids.push(model.guid);
>                      }
> +                    NotesStore.saveNote(root.note.guid);
>                  }
>              }
>          }
> 
> === added file 'src/app/qml/components/Header.qml'
> --- src/app/qml/components/Header.qml	1970-01-01 00:00:00 +0000
> +++ src/app/qml/components/Header.qml	2015-06-17 21:22:37 +0000
> @@ -0,0 +1,131 @@
> +import QtQuick 2.2
> +import QtQuick.Layouts 1.1
> +import Ubuntu.Components 1.1
> +import Ubuntu.Components.ListItems 1.0
> +import Ubuntu.Components.Themes.Ambiance 1.1
> +import Evernote 0.1
> +
> +
> +Column {
> +    id: root
> +    width: parent.width
> +    height: childrenRect.height
> +
> +    property var note: null
> +
> +    property bool editingEnabled: true
> +
> +    property alias title: titleTextField.text
> +
> +    signal editReminders();
> +    signal editTags();
> +
> +    TextField {
> +        id: titleTextField
> +        height: units.gu(6)
> +        width: parent.width
> +        text: root.note ? root.note.title : ""
> +        placeholderText: i18n.tr("Untitled")
> +        font.pixelSize: units.gu(4)
> +        visible: root.editingEnabled
> +        style: TextFieldStyle {
> +            background: null
> +        }
> +    }
> +
> +    Label {
> +        height: units.gu(6)
> +        anchors.left: parent.left
> +        anchors.right: parent.right
> +        anchors.margins: units.gu(1)
> +        text: root.note ? root.note.title : ""
> +        visible: !root.editingEnabled
> +        font.pixelSize: units.gu(4)
> +        verticalAlignment: Text.AlignVCenter
> +    }
> +
> +    ThinDivider {}
> +

Nitpick: could you please remove the second newline? :-)

> +
> +    ItemSelector {
> +        id: notebookSelector
> +        width: parent.width
> +        model: Notebooks {}
> +
> +        Component.onCompleted: {
> +            for (var i = 0; i < model.count; i++) {
> +                if (model.notebook(i).guid == root.note.notebookGuid) {

It happens I've this error, I think you should add a check

file:///home/rpadovani/Ubuntu/touch/core-apps/reminders/reminders-app/builddir/src/app/qml/components/Header.qml:57: TypeError: Cannot read property 'notebookGuid' of null

> +                    selectedIndex = i;
> +                }
> +            }
> +        }
> +
> +        onDelegateClicked: {
> +            var newNotebookGuid = model.notebook(index).guid;
> +            if (newNotebookGuid != root.note.notebookGuid) {
> +                root.note.notebookGuid = newNotebookGuid;
> +                NotesStore.saveNote(root.note.guid)
> +            }
> +        }
> +
> +        delegate: OptionSelectorDelegate {
> +            Rectangle {
> +                anchors.fill: parent
> +                color: "white"
> +
> +                RowLayout {
> +                    anchors {
> +                        fill: parent
> +                        leftMargin: units.gu(1)
> +                        rightMargin: units.gu(1)
> +                        topMargin: units.gu(0.5)
> +                        bottomMargin: units.gu(0.5)
> +                    }
> +
> +                    Item {
> +                        height: parent.height
> +                        width: height
> +                        Icon {
> +                            anchors.fill: parent
> +                            anchors.margins: units.gu(0.5)
> +                            name: "notebook"
> +                            color: preferences.colorForNotebook(model.guid)
> +                        }
> +                    }
> +
> +                    Label {
> +                        text: model.name
> +                        Layout.fillWidth: true
> +                        color: preferences.colorForNotebook(model.guid)
> +                    }
> +                    RtfButton {
> +                        iconName: root.note && root.note.reminder ? "reminder" : "reminder-new"
> +                        height: parent.height
> +                        width: height
> +                        iconColor: root.note && note.reminder ? UbuntuColors.blue : Qt.rgba(0.0, 0.0, 0.0, 0.0)
> +                        visible: index == notebookSelector.selectedIndex
> +                        onClicked: {
> +                            Qt.inputMethod.hide();
> +                            root.editReminders();
> +                        }
> +                    }
> +                    RtfButton {
> +                        id: tagsButton
> +                        iconSource: "../images/tags.svg"
> +                        height: parent.height
> +                        width: height
> +                        visible: index == notebookSelector.selectedIndex
> +                        onClicked: {
> +                            Qt.inputMethod.hide();
> +                            root.editTags();
> +                        }
> +                    }
> +                }
> +            }
> +        }
> +    }
> +
> +    ThinDivider {}
> +
> +}
> +
> 
> === modified file 'src/app/qml/reminders.qml'
> --- src/app/qml/reminders.qml	2015-06-12 09:48:22 +0000
> +++ src/app/qml/reminders.qml	2015-06-17 21:22:37 +0000
> @@ -1,4 +1,4 @@
> -/*
> +/*
>   * Copyright: 2013 - 2014 Canonical, Ltd
>   *
>   * This file is part of reminders
> @@ -125,7 +125,6 @@
>              } else {
>                  var page = pagestack.push(Qt.createComponent(Qt.resolvedUrl("ui/NotePage.qml")), {readOnly: conflictMode, note: note })
>                  page.editNote.connect(function(note) {root.switchToEditMode(note)})
> -                page.openTaggedNotes.connect(function(title, tagGuid) {pagestack.pop();root.openTaggedNotes(title, tagGuid, true)})
>              }
>          } else {
>              var view;
> @@ -143,7 +142,7 @@
>              } else {
>                  notesPage.conflictMode = conflictMode;
>                  view = sideViewLoader.embed(Qt.resolvedUrl("ui/NoteView.qml"))
> -                view.openTaggedNotes.connect(function(title, tagGuid) {root.openTaggedNotes(title, tagGuid, false)})
> +                view.editNote.connect(function() {root.switchToEditMode(view.note)})
>              }
>              view.note = note;
>          }
> @@ -302,7 +301,7 @@
>      }
>  
>      function openTaggedNotes(title, tagGuid, narrowMode) {
> -        print("opening note page for tag", tagGuid)
> +        print("!!!opening note page for tag", tagGuid)
>          var page = pagestack.push(Qt.createComponent(Qt.resolvedUrl("ui/NotesPage.qml")), {title: title, filterTagGuid: tagGuid, narrowMode: narrowMode});
>          page.selectedNoteChanged.connect(function() {
>              if (page.selectedNote) {
> @@ -555,20 +554,7 @@
>  
>                      onOpenTaggedNotes: {
>                          var tag = NotesStore.tag(tagGuid);
> -                        print("opening note page for tag", tagGuid)
> -                        var page = pagestack.push(Qt.createComponent(Qt.resolvedUrl("ui/NotesPage.qml")), {title: tag.name, filterTagGuid: tagGuid, narrowMode: root.narrowMode});
> -                        page.selectedNoteChanged.connect(function() {
> -                            if (page.selectedNote) {
> -                                root.displayNote(page.selectedNote);
> -                                if (root.narrowMode) {
> -                                    page.selectedNote = null;
> -                                }
> -                            }
> -                        })
> -                        page.editNote.connect(function(note) {
> -                            root.switchToEditMode(note)
> -                        })
> -                        NotesStore.refreshNotes();
> +                        root.openTaggedNotes(tag.name, tag.guid, root.narrowMode)
>                      }
>  
>                      onOpenSearch: root.openSearch();
> 
> === modified file 'src/app/qml/ui/EditNoteView.qml'
> --- src/app/qml/ui/EditNoteView.qml	2015-06-11 19:32:31 +0000
> +++ src/app/qml/ui/EditNoteView.qml	2015-06-17 21:22:37 +0000
> @@ -43,13 +43,12 @@
>      signal exitEditMode(var note)
>  
>      function saveNote() {
> -        var title = titleTextField.text ? titleTextField.text : i18n.tr("Untitled");
> -        var notebookGuid = notebookSelector.selectedGuid;
> +        var title = header.title ? header.title : i18n.tr("Untitled");
> +        var notebookGuid = header.notebookGuid;
>          var text = noteTextArea.text;
>  
>          if (note) {
>              note.title = title;
> -            note.notebookGuid = notebookGuid;
>              note.richTextContent = text;
>              NotesStore.saveNote(note.guid);
>          } else {
> @@ -120,11 +119,11 @@
>  
>                   function ensureVisible(r)
>                   {
> -                     var staticHeight = titleTextField.height + notebookSelector.height
> +                     var staticHeight = header.height
>                       if (contentX >= r.x)
>                           contentX = r.x;
> -                     else if (contentX +width <= r.x+r.width)
> -                         contentX = r.x+r.width-width;
> +                     else if (contentX + width <= r.x + r.width)
> +                         contentX = r.x + r.width-width;
>                       if (contentY >= r.y)
>                           contentY = r.y;
>                       else if (contentY + height <= r.y + staticHeight + r.height) {
> @@ -137,84 +136,22 @@
>                       width: parent.width
>                       height: childrenRect.height
>  
> -                     TextField {
> -                         id: titleTextField
> -                         height: units.gu(6)
> -                         width: parent.width
> -                         text: root.note ? root.note.title : ""
> -                         placeholderText: i18n.tr("Untitled")
> -                         font.pixelSize: units.gu(4)
> -                         style: TextFieldStyle {
> -                             background: null
> -                         }
> -                     }
> -
> -                     ThinDivider {}
> -
> -                     ValueSelector {
> -                         id: notebookSelector
> -                         width: parent.width
> -                         text: values.notebook(selectedIndex).name
> -                         property string selectedGuid: values.notebook(selectedIndex) ? values.notebook(selectedIndex).guid : ""
> -                         values: Notebooks {}
> -
> -                         // The ValueSelector is not customizable enough, yet we wanna use the expanstion it provides. Let's just paint on top of it
> -
> -                         Rectangle {
> -                             anchors { left: parent.left; right: parent.right; top: parent.top }
> -                             height: units.gu(6)
> -                             color: "white"
> -
> -                             RowLayout {
> -                                 anchors.fill: parent
> -                                 anchors.margins: units.gu(1)
> -
> -                                 Item {
> -                                     height: parent.height
> -                                     width: height
> -                                     Icon {
> -                                         anchors.fill: parent
> -                                         anchors.margins: units.gu(0.5)
> -                                         name: "notebook"
> -                                         color: preferences.colorForNotebook(notebookSelector.values.notebook(notebookSelector.selectedIndex).guid)
> -                                     }
> -                                 }
> -
> -                                 Label {
> -                                     text: notebookSelector.values.notebook(notebookSelector.selectedIndex).name
> -                                     Layout.fillWidth: true
> -                                     color: preferences.colorForNotebook(notebookSelector.values.notebook(notebookSelector.selectedIndex).guid)
> -                                 }
> -                                 RtfButton {
> -                                     iconName: root.note && root.note.reminder ? "reminder" : "reminder-new"
> -                                     height: parent.height
> -                                     width: height
> -                                     iconColor: root.note && note.reminder ? UbuntuColors.blue : Qt.rgba(0.0, 0.0, 0.0, 0.0)
> -                                     onClicked: {
> -                                         Qt.inputMethod.hide();
> -                                         pageStack.push(Qt.resolvedUrl("SetReminderPage.qml"), { note: root.note});
> -                                     }
> -                                 }
> -                                 RtfButton {
> -                                     id: tagsButton
> -                                     iconSource: "../images/tags.svg"
> -                                     height: parent.height
> -                                     width: height
> -                                     onClicked: {
> -                                         Qt.inputMethod.hide();
> -                                         PopupUtils.open(tagsDialog)
> -                                     }
> -                                 }
> -                             }
> -                         }
> -                     }
> -
> -                     ThinDivider {}
> +                     Header {
> +                        id: header
> +                        note: root.note
> +
> +                        onEditReminders: {
> +                            pageStack.push(Qt.resolvedUrl("SetReminderPage.qml"), { note: root.note});
> +                        }
> +                        onEditTags: {
> +                            PopupUtils.open(Qt.resolvedUrl("../components/EditTagsDialog.qml"), root, { note: root.note, pageHeight: root.height});
> +                        }
> +                     }
>  
>                       TextEdit {
>                           id: noteTextArea
>                           width: flick.width
> -                         height: Math.max(flick.height - notebookSelector.height - titleTextField.height, paintedHeight)
> +                         height: Math.max(flick.height - header.height, paintedHeight)
>                           focus: true
>                           wrapMode: TextEdit.Wrap
>                           textFormat: TextEdit.RichText
> @@ -415,11 +352,6 @@
>          }
>      }
>  
> -    Component {
> -        id: tagsDialog
> -        EditTagsDialog { note: root.note; pageHeight: parent.height }
> -    }
> -
>      Rectangle {
>          anchors.fill: toolbox
>          color: "#efefef"
> 
> === modified file 'src/app/qml/ui/NotePage.qml'
> --- src/app/qml/ui/NotePage.qml	2015-05-27 22:45:53 +0000
> +++ src/app/qml/ui/NotePage.qml	2015-06-17 21:22:37 +0000
> @@ -28,42 +28,15 @@
>      property bool readOnly: false
>  
>      signal editNote(var note)
> -    signal openTaggedNotes(string title, string tagGuid)
>  
> -    head {
> -        actions: !root.readOnly ? normalActions : []
> -        property list<Action> normalActions: [
> -            Action {
> -                text: i18n.tr("Edit")
> -                iconName: "edit"
> -                onTriggered: {
> -                    root.editNote(root.note)
> -                }
> -            },
> -            Action {
> -                text: note.reminder ? i18n.tr("Edit reminder") : i18n.tr("Set reminder")
> -                iconName: note.reminder ? "reminder" : "reminder-new"
> -                onTriggered: {
> -                    pageStack.push(Qt.resolvedUrl("SetReminderPage.qml"), { note: root.note});
> -                }
> -            },
> -            Action {
> -                text: i18n.tr("Delete")
> -                iconName: "delete"
> -                onTriggered: {
> -                    NotesStore.deleteNote(note.guid);
> -                    pagestack.pop();
> -                }
> -            }
> -        ]
> -    }
>  
>      NoteView {
>          id: noteView
>          anchors.fill: parent
> +        canClose: true
>  
> -        onOpenTaggedNotes: {
> -            root.openTaggedNotes(title, tagGuid);
> +        onEditNote: {
> +            root.editNote(root.note)
>          }
>      }
>  }
> 
> === modified file 'src/app/qml/ui/NoteView.qml'
> --- src/app/qml/ui/NoteView.qml	2015-05-27 21:04:57 +0000
> +++ src/app/qml/ui/NoteView.qml	2015-06-17 21:22:37 +0000
> @@ -17,19 +17,21 @@
>   */
>  
>  import QtQuick 2.3
> +import QtQuick.Layouts 1.1
>  import Ubuntu.Components 1.1
> -import com.canonical.Oxide 1.0
> +import Ubuntu.Components.ListItems 1.0
> +import Ubuntu.Components.Popups 1.0
> +import com.canonical.Oxide 1.5
>  import Ubuntu.Content 1.0
>  import Evernote 0.1
>  import "../components"
>  
>  Item {
>      id: root
> -    property string title: contentPeerPicker.visible ? ""
> -                            : (note && note.title) ? note.title : i18n.tr("Untitled")
>      property var note: null
> +    property bool canClose: false
>  
> -    signal openTaggedNotes(string title, string tagGuid)
> +    signal editNote()
>  
>      BouncingProgressBar {
>          anchors.top: parent.top
> @@ -48,10 +50,39 @@
>          ]
>      }
>  
> +    Rectangle {
> +        id: locationBar
> +        y: noteTextArea.locationBarController.offset
> +        anchors.left: parent.left
> +        anchors.right: parent.right
> +        height: headerContent.height
> +        color: "white"
> +        z: 2
> +
> +        Header {
> +            id: headerContent
> +            note: root.note
> +            editingEnabled: false
> +
> +            onEditReminders: {
> +                print("pushing reminderspage", root.note.reminder)
> +                pageStack.push(Qt.resolvedUrl("SetReminderPage.qml"), { note: root.note});
> +            }
> +            onEditTags: {
> +                PopupUtils.open(Qt.resolvedUrl("../components/EditTagsDialog.qml"), root, { note: root.note, pageHeight: root.height });
> +            }
> +        }
> +    }
> +
>      WebView {
>          id: noteTextArea
> -        width: parent.width
> -        height: parent.height - tagsRow.height - (tagsRow.height > 0 ? units.gu(2) : 0)
> +        anchors.fill: parent
> +        anchors.bottomMargin: buttonPanel.height
> +
> +        locationBarController {
> +            height: locationBar.height
> +            mode: Oxide.LocationBarController.ModeAuto

src/app/qml/ui/NoteView.qml:84: TypeError: Cannot read property 'ModeAuto' of undefined

> +        }
>  
>          property string html: root.note ? note.htmlContent : ""
>  
> @@ -104,38 +135,46 @@
>          ]
>      }
>  
> -    ListView {
> -        id: tagsRow
> -        anchors { left: parent.left; right: parent.right; bottom: parent.bottom; margins: units.gu(1) }
> -        model: root.note ? root.note.tagGuids.length : undefined
> -        orientation: ListView.Horizontal
> -        spacing: units.gu(1)
> -        height: visible ? units.gu(3) : 0
> -        visible: root.note ? root.note.tagGuids.length > 0 ? true : false : false
> -
> -        delegate: Rectangle {
> -            id: rectangle
> -            radius: units.gu(1)
> -            color: "white"
> -            border.color: preferences.colorForNotebook(root.note.notebookGuid)
> -
> -            Text {
> -                text: NotesStore.tag(root.note.tagGuids[index]).name
> -                color: preferences.colorForNotebook(root.note.notebookGuid)
> -                Component.onCompleted: {
> -                    rectangle.width = width + units.gu(2)
> -                    rectangle.height = height + units.gu(1)
> -                    anchors.centerIn = parent
> +    Item {
> +        id: buttonPanel
> +        anchors { left: parent.left; right: parent.right; bottom: parent.bottom }
> +        height: units.gu(6)
> +
> +        RowLayout {
> +            anchors { left: parent.left; right: parent.right; fill: parent }
> +            anchors.margins: units.gu(1)
> +            height: units.gu(4)
> +
> +            RtfButton {
> +                iconName: "tick"
> +                // TRANSLATORS: Button to close the note viewer
> +                text: i18n.tr("Close")
> +                height: parent.height
> +                iconColor: UbuntuColors.green
> +                visible: root.canClose
> +                onClicked: {
> +                    pageStack.pop()
>                  }
>              }
>  
> -            MouseArea {
> -                anchors.fill: parent
> +            RtfSeparator {
> +                visible: root.canClose
> +            }
> +
> +            Item {
> +                Layout.fillWidth: true
> +            }
> +
> +            RtfSeparator {}
> +
> +            RtfButton {
> +                iconName: "edit"
> +                // TRANSLATORS: Button to go from note viewer to note editor
> +                text: i18n.tr("Edit")
> +                height: parent.height
> +                iconColor: UbuntuColors.green
>                  onClicked: {
> -                    if (!narrowMode) {
> -                        sideViewLoader.clear();
> -                    }
> -                    root.openTaggedNotes(NotesStore.tag(root.note.tagGuids[index]).name, NotesStore.tag(root.note.tagGuids[index]).guid)
> +                    root.editNote()
>                  }
>              }
>          }
> 
> === modified file 'src/app/qml/ui/SetReminderPage.qml'
> --- src/app/qml/ui/SetReminderPage.qml	2014-09-23 12:39:27 +0000
> +++ src/app/qml/ui/SetReminderPage.qml	2015-06-17 21:22:37 +0000
> @@ -26,8 +26,6 @@
>      title: setReminderView.title
>      property alias note: setReminderView.note
>  
> -    signal editNote(var note)
> -
>      SetReminderView {
>          id: setReminderView
>          anchors.fill: parent
> 
> === modified file 'src/app/qml/ui/TagsPage.qml'
> --- src/app/qml/ui/TagsPage.qml	2015-03-15 20:00:21 +0000
> +++ src/app/qml/ui/TagsPage.qml	2015-06-17 21:22:37 +0000
> @@ -85,7 +85,6 @@
>                  height: units.gu(10)
>  
>                  onItemClicked: {
> -                    print("selected tag:", model.guid)
>                      root.openTaggedNotes(model.guid)
>                  }
>                  onDeleteTag: {
> 


-- 
https://code.launchpad.net/~mzanetti/reminders-app/improve-viewer/+merge/262162
Your team Ubuntu Reminders app developers is subscribed to branch lp:reminders-app.


Follow ups

References