Ubuntu Notes app

Merge lp:~mzanetti/reminders-app/loading-property into lp:reminders-app

  1. loading-property
  2. Merge into trunk
Proposed by Michael Zanetti
Status: Merged
Approved by: David Planella
Approved revision: 95
Merged at revision: 113
Proposed branch: lp:~mzanetti/reminders-app/loading-property
Merge into: lp:reminders-app
Diff against target: 402 lines (+116/-4)
11 files modified
src/app/qml/ui/NotebooksPage.qml (+7/-0)
src/app/qml/ui/NotesPage.qml (+10/-1)
src/app/qml/ui/RemindersPage.qml (+7/-1)
src/plugin/Evernote/note.cpp (+15/-1)
src/plugin/Evernote/note.h (+15/-0)
src/plugin/Evernote/notebooks.cpp (+6/-0)
src/plugin/Evernote/notebooks.h (+7/-0)
src/plugin/Evernote/notes.cpp (+6/-0)
src/plugin/Evernote/notes.h (+4/-0)
src/plugin/Evernote/notesstore.cpp (+29/-1)
src/plugin/Evernote/notesstore.h (+10/-0)
To merge this branch: bzr merge lp:~mzanetti/reminders-app/loading-property
Reviewer Review Type Date Requested Status
David Planella Approve
Ubuntu Phone Apps Jenkins Bot continuous-integration Approve
Riccardo Padovani Approve
Review via email: mp+217792@code.launchpad.net

Commit message

Added a loading property to Notes, Notebooks and Note in order to display busyindicators.

Description of the change

This is based on top of rpadovani's branch: https://code.launchpad.net/~rpadovani/reminders-app/18Mar14/+merge/211545

To post a comment you must log in.
lp:~mzanetti/reminders-app/loading-property updated
93. By Michael Zanetti

merge trunk

94. By Michael Zanetti

revert po changes

Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :

PASSED: Continuous integration, rev:94
http://91.189.93.70:8080/job/reminders-app-ci/276/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2321
    SUCCESS: http://91.189.93.70:8080/job/reminders-app-saucy-amd64-ci/276
    SUCCESS: http://91.189.93.70:8080/job/reminders-app-trusty-amd64-ci/276

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/reminders-app-ci/276/rebuild

review: Approve (continuous-integration)
Revision history for this message
Riccardo Padovani (rpadovani) wrote :

Good job, it works for me.
I like to have the spinner everytime something works in the background, but I prefer to hear dpm's opinion before top-approve it.

review: Approve
lp:~mzanetti/reminders-app/loading-property updated
95. By Michael Zanetti

constify

Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :

PASSED: Continuous integration, rev:95
http://91.189.93.70:8080/job/reminders-app-ci/279/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2329
    SUCCESS: http://91.189.93.70:8080/job/reminders-app-saucy-amd64-ci/279
    SUCCESS: http://91.189.93.70:8080/job/reminders-app-trusty-amd64-ci/279

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/reminders-app-ci/279/rebuild

review: Approve (continuous-integration)
Revision history for this message
David Planella (dpm) wrote :

Looks good to me, thanks to both Michael and Riccardo to get this one done. It's a huge improvement in terms of feedback to the user and the feel for responsiveness of the app.

Adding some context re: the discussion on when to show the activity indicator for the future.

<dpm> mzanetti, I think I agree with rpadovani that I'd hide the spinner on refreshes and perhaps just show it on the initial loading of the notes/notebooks lists (when they're empty). However, given the fact that the branch is a huge improvement already, and that the placement of the spinner might change with the new designs, I think we should go with your implementation, so top-approving, thanks!
<mzanetti> dpm: its close to no efforts to hide it when there's already something in the list
 dpm: if you want I can quickly do that before you top approve
<dpm> mzanetti, that'd mean that if I lose network connectivity once the list has been fetched, I won't get any spinner and thus no indication of flaky/no-network connection, correct?
<mzanetti> dpm: yep...
 dpm: again, I would keep it to inform the user when something is loading... but also not really in the center of the page
<mzanetti> dpm: lets keep it as is, gather some experience with it and ask Lucas to find a nice place where tho show it in the future
<dpm> mzanetti, ok, I agree, thanks!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/app/qml/ui/NotebooksPage.qml'
2--- src/app/qml/ui/NotebooksPage.qml 2014-03-19 16:26:36 +0000
3+++ src/app/qml/ui/NotebooksPage.qml 2014-04-30 21:18:05 +0000
4@@ -105,6 +105,7 @@
5 }
6
7 ListView {
8+ id: notebooksListView
9 model: notebooks
10 anchors { left: parent.left; right: parent.right }
11 height: parent.height - y - buttonRow.height - keyboardRect.height
12@@ -119,6 +120,12 @@
13 root.openNotebook(name, model.guid)
14 }
15 }
16+
17+ ActivityIndicator {
18+ anchors.centerIn: parent
19+ running: notebooks.loading
20+ visible: running
21+ }
22 }
23
24 Item {
25
26=== modified file 'src/app/qml/ui/NotesPage.qml'
27--- src/app/qml/ui/NotesPage.qml 2014-03-13 18:11:32 +0000
28+++ src/app/qml/ui/NotesPage.qml 2014-04-30 21:18:05 +0000
29@@ -99,6 +99,7 @@
30 }
31
32 ListView {
33+ id: notesListView
34 objectName: "notespageListview"
35 anchors { left: parent.left; right: parent.right }
36 height: parent.height - y
37@@ -111,11 +112,19 @@
38 content: model.plaintextContent
39 resource: model.resourceUrls.length > 0 ? model.resourceUrls[0] : ""
40
41- Component.onCompleted: NotesStore.refreshNoteContent(model.guid)
42+ Component.onCompleted: {
43+ NotesStore.refreshNoteContent(model.guid)
44+ }
45
46 onClicked: {
47 root.selectedNote = NotesStore.note(guid);
48 }
49 }
50+
51+ ActivityIndicator {
52+ anchors.centerIn: parent
53+ running: notes.loading
54+ visible: running
55+ }
56 }
57 }
58
59=== modified file 'src/app/qml/ui/RemindersPage.qml'
60--- src/app/qml/ui/RemindersPage.qml 2014-02-17 22:50:10 +0000
61+++ src/app/qml/ui/RemindersPage.qml 2014-04-30 21:18:05 +0000
62@@ -59,7 +59,7 @@
63 }
64
65 ListView {
66-
67+ id: remindersListView
68 anchors.fill: parent
69
70 delegate: RemindersDelegate {
71@@ -74,6 +74,12 @@
72 height: units.gu(3)
73 text: section
74 }
75+
76+ ActivityIndicator {
77+ anchors.centerIn: parent
78+ running: notes.loading
79+ visible: running
80+ }
81 }
82
83 }
84
85=== modified file 'src/plugin/Evernote/note.cpp'
86--- src/plugin/Evernote/note.cpp 2014-02-19 12:19:29 +0000
87+++ src/plugin/Evernote/note.cpp 2014-04-30 21:18:05 +0000
88@@ -34,7 +34,8 @@
89 QObject(parent),
90 m_guid(guid),
91 m_created(created),
92- m_isSearchResult(false)
93+ m_isSearchResult(false),
94+ m_loading(false)
95 {
96 }
97
98@@ -43,6 +44,11 @@
99 qDeleteAll(m_resources.values());
100 }
101
102+bool Note::loading() const
103+{
104+ return m_loading;
105+}
106+
107 QString Note::guid() const
108 {
109 return m_guid;
110@@ -334,3 +340,11 @@
111 {
112 NotesStore::instance()->deleteNote(m_guid);
113 }
114+
115+void Note::setLoading(bool loading)
116+{
117+ if (m_loading != loading) {
118+ m_loading = loading;
119+ emit loadingChanged();
120+ }
121+}
122
123=== modified file 'src/plugin/Evernote/note.h'
124--- src/plugin/Evernote/note.h 2014-02-17 22:50:10 +0000
125+++ src/plugin/Evernote/note.h 2014-04-30 21:18:05 +0000
126@@ -53,10 +53,15 @@
127 Q_PROPERTY(bool isSearchResult READ isSearchResult NOTIFY isSearchResultChanged)
128 // Don't forget to update clone() if you add properties!
129
130+ // Don't clone() "loading" property as results of any current loading operation won't affect the clone.
131+ Q_PROPERTY(bool loading READ loading NOTIFY loadingChanged)
132+
133 public:
134 explicit Note(const QString &guid, const QDateTime &created, QObject *parent = 0);
135 ~Note();
136
137+ bool loading() const;
138+
139 QString guid() const;
140
141 QString notebookGuid() const;
142@@ -132,6 +137,11 @@
143 void reminderDoneChanged();
144 void isSearchResultChanged();
145
146+ void loadingChanged();
147+
148+private:
149+ void setLoading(bool loading);
150+
151 private:
152 QString m_guid;
153 QString m_notebookGuid;
154@@ -143,6 +153,11 @@
155 QDateTime m_reminderDoneTime;
156 bool m_isSearchResult;
157 QHash<QString, Resource*> m_resources;
158+
159+ bool m_loading;
160+
161+ // Needed to be able to call private setLoading (we don't want to have that set by anyone except the NotesStore)
162+ friend class NotesStore;
163 };
164
165 #endif // NOTE_H
166
167=== modified file 'src/plugin/Evernote/notebooks.cpp'
168--- src/plugin/Evernote/notebooks.cpp 2014-02-18 14:04:33 +0000
169+++ src/plugin/Evernote/notebooks.cpp 2014-04-30 21:18:05 +0000
170@@ -31,10 +31,16 @@
171 connect(notebook, &Notebook::noteCountChanged, this, &Notebooks::noteCountChanged);
172 }
173
174+ connect(NotesStore::instance(), &NotesStore::notebooksLoadingChanged, this, &Notebooks::loadingChanged);
175 connect(NotesStore::instance(), SIGNAL(notebookAdded(const QString &)), SLOT(notebookAdded(const QString &)));
176 connect(NotesStore::instance(), SIGNAL(notebookRemoved(const QString &)), SLOT(notebookRemoved(const QString &)));
177 }
178
179+bool Notebooks::loading() const
180+{
181+ return NotesStore::instance()->notebooksLoading();
182+}
183+
184 QVariant Notebooks::data(const QModelIndex &index, int role) const
185 {
186 Notebook *notebook = NotesStore::instance()->notebook(m_list.at(index.row()));
187
188=== modified file 'src/plugin/Evernote/notebooks.h'
189--- src/plugin/Evernote/notebooks.h 2014-02-05 20:37:05 +0000
190+++ src/plugin/Evernote/notebooks.h 2014-04-30 21:18:05 +0000
191@@ -28,6 +28,8 @@
192 class Notebooks : public QAbstractListModel
193 {
194 Q_OBJECT
195+ Q_PROPERTY(bool loading READ loading NOTIFY loadingChanged)
196+
197 public:
198 enum Roles {
199 RoleGuid,
200@@ -37,6 +39,8 @@
201 };
202 explicit Notebooks(QObject *parent = 0);
203
204+ bool loading() const;
205+
206 QVariant data(const QModelIndex &index, int role) const;
207 int rowCount(const QModelIndex &parent) const;
208 QHash<int, QByteArray> roleNames() const;
209@@ -46,6 +50,9 @@
210 public slots:
211 void refresh();
212
213+signals:
214+ void loadingChanged();
215+
216 private slots:
217 void notebookAdded(const QString &guid);
218 void notebookRemoved(const QString &guid);
219
220=== modified file 'src/plugin/Evernote/notes.cpp'
221--- src/plugin/Evernote/notes.cpp 2014-02-17 22:50:10 +0000
222+++ src/plugin/Evernote/notes.cpp 2014-04-30 21:18:05 +0000
223@@ -27,6 +27,7 @@
224 QSortFilterProxyModel(parent),
225 m_onlyReminders(false)
226 {
227+ connect(NotesStore::instance(), &NotesStore::loadingChanged, this, &Notes::loadingChanged);
228 setSourceModel(NotesStore::instance());
229 setSortRole(NotesStore::RoleCreated);
230 sort(0, Qt::DescendingOrder);
231@@ -81,6 +82,11 @@
232 }
233 }
234
235+bool Notes::loading() const
236+{
237+ return NotesStore::instance()->loading();
238+}
239+
240 Note *Notes::note(const QString &guid)
241 {
242 return NotesStore::instance()->note(guid);
243
244=== modified file 'src/plugin/Evernote/notes.h'
245--- src/plugin/Evernote/notes.h 2014-02-05 17:26:54 +0000
246+++ src/plugin/Evernote/notes.h 2014-04-30 21:18:05 +0000
247@@ -31,6 +31,7 @@
248 Q_PROPERTY(QString filterNotebookGuid READ filterNotebookGuid WRITE setFilterNotebookGuid NOTIFY filterNotebookGuidChanged)
249 Q_PROPERTY(bool onlyReminders READ onlyReminders WRITE setOnlyReminders NOTIFY onlyRemindersChanged)
250 Q_PROPERTY(bool onlySearchResults READ onlySearchResults WRITE setOnlySearchResults NOTIFY onlySearchResultsChanged)
251+ Q_PROPERTY(bool loading READ loading NOTIFY loadingChanged)
252
253 public:
254 explicit Notes(QObject *parent = 0);
255@@ -44,6 +45,8 @@
256 bool onlySearchResults() const;
257 void setOnlySearchResults(bool onlySearchResults);
258
259+ bool loading() const;
260+
261 Q_INVOKABLE Note* note(const QString &guid);
262
263 protected:
264@@ -53,6 +56,7 @@
265 void filterNotebookGuidChanged();
266 void onlyRemindersChanged();
267 void onlySearchResultsChanged();
268+ void loadingChanged();
269
270 private:
271 QString m_filterNotebookGuid;
272
273=== modified file 'src/plugin/Evernote/notesstore.cpp'
274--- src/plugin/Evernote/notesstore.cpp 2014-02-18 14:04:33 +0000
275+++ src/plugin/Evernote/notesstore.cpp 2014-04-30 21:18:05 +0000
276@@ -40,7 +40,9 @@
277 NotesStore* NotesStore::s_instance = 0;
278
279 NotesStore::NotesStore(QObject *parent) :
280- QAbstractListModel(parent)
281+ QAbstractListModel(parent),
282+ m_loading(false),
283+ m_notebooksLoading(false)
284 {
285 connect(EvernoteConnection::instance(), &EvernoteConnection::tokenChanged, this, &NotesStore::refreshNotebooks);
286 connect(EvernoteConnection::instance(), SIGNAL(tokenChanged()), this, SLOT(refreshNotes()));
287@@ -60,6 +62,16 @@
288 return s_instance;
289 }
290
291+bool NotesStore::loading() const
292+{
293+ return m_loading;
294+}
295+
296+bool NotesStore::notebooksLoading() const
297+{
298+ return m_notebooksLoading;
299+}
300+
301 int NotesStore::rowCount(const QModelIndex &parent) const
302 {
303 Q_UNUSED(parent)
304@@ -170,6 +182,8 @@
305 m_notes.clear();
306 endResetModel();
307 } else {
308+ m_loading = true;
309+ emit loadingChanged();
310 FetchNotesJob *job = new FetchNotesJob(filterNotebookGuid);
311 connect(job, &FetchNotesJob::jobDone, this, &NotesStore::fetchNotesJobDone);
312 EvernoteConnection::instance()->enqueue(job);
313@@ -178,6 +192,9 @@
314
315 void NotesStore::fetchNotesJobDone(EvernoteConnection::ErrorCode errorCode, const QString &errorMessage, const evernote::edam::NotesMetadataList &results)
316 {
317+ m_loading = false;
318+ emit loadingChanged();
319+
320 if (errorCode != EvernoteConnection::ErrorCodeNoError) {
321 qWarning() << "Failed to fetch notes list:" << errorMessage;
322 return;
323@@ -228,6 +245,11 @@
324
325 void NotesStore::refreshNoteContent(const QString &guid)
326 {
327+ Note *note = m_notesHash.value(guid);
328+ if (note) {
329+ note->setLoading(true);
330+ }
331+
332 FetchNoteJob *job = new FetchNoteJob(guid, this);
333 connect(job, &FetchNoteJob::resultReady, this, &NotesStore::fetchNoteJobDone);
334 EvernoteConnection::instance()->enqueue(job);
335@@ -245,6 +267,7 @@
336 qWarning() << "can't find note for this update... ignoring...";
337 return;
338 }
339+ note->setLoading(false);
340 note->setNotebookGuid(QString::fromStdString(result.notebookGuid));
341 note->setTitle(QString::fromStdString(result.title));
342
343@@ -288,6 +311,8 @@
344 }
345 m_notebooks.clear();
346 } else {
347+ m_notebooksLoading = true;
348+ emit notebooksLoadingChanged();
349 FetchNotebooksJob *job = new FetchNotebooksJob();
350 connect(job, &FetchNotebooksJob::jobDone, this, &NotesStore::fetchNotebooksJobDone);
351 EvernoteConnection::instance()->enqueue(job);
352@@ -296,6 +321,9 @@
353
354 void NotesStore::fetchNotebooksJobDone(EvernoteConnection::ErrorCode errorCode, const QString &errorMessage, const std::vector<evernote::edam::Notebook> &results)
355 {
356+ m_notebooksLoading = false;
357+ emit notebooksLoadingChanged();
358+
359 if (errorCode != EvernoteConnection::ErrorCodeNoError) {
360 qWarning() << "Error fetching notebooks:" << errorMessage;
361 return;
362
363=== modified file 'src/plugin/Evernote/notesstore.h'
364--- src/plugin/Evernote/notesstore.h 2014-02-18 22:58:40 +0000
365+++ src/plugin/Evernote/notesstore.h 2014-04-30 21:18:05 +0000
366@@ -47,6 +47,8 @@
367 class NotesStore : public QAbstractListModel
368 {
369 Q_OBJECT
370+ Q_PROPERTY(bool loading READ loading NOTIFY loadingChanged)
371+ Q_PROPERTY(bool notebooksLoading READ notebooksLoading NOTIFY notebooksLoadingChanged)
372
373 public:
374 enum Roles {
375@@ -70,6 +72,9 @@
376 ~NotesStore();
377 static NotesStore *instance();
378
379+ bool loading() const;
380+ bool notebooksLoading() const;
381+
382 // reimplemented from QAbstractListModel
383 int rowCount(const QModelIndex &parent) const;
384 QVariant data(const QModelIndex &index, int role) const;
385@@ -96,6 +101,8 @@
386
387 signals:
388 void tokenChanged();
389+ void loadingChanged();
390+ void notebooksLoadingChanged();
391
392 void noteCreated(const QString &guid, const QString &notebookGuid);
393 void noteAdded(const QString &guid, const QString &notebookGuid);
394@@ -120,6 +127,9 @@
395 explicit NotesStore(QObject *parent = 0);
396 static NotesStore *s_instance;
397
398+ bool m_loading;
399+ bool m_notebooksLoading;
400+
401 QList<Note*> m_notes;
402 QList<Notebook*> m_notebooks;
403

Subscribers

People subscribed via source and target branches