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.

-- 
Mailing list: https://launchpad.net/~ubuntu-touch-coreapps-reviewers
Post to     : [email protected]
Unsubscribe : https://launchpad.net/~ubuntu-touch-coreapps-reviewers
More help   : https://help.launchpad.net/ListHelp

Reply via email to