ubuntu-touch-coreapps-reviewers team mailing list archive

[Riccardo Padovani <riccardo@xxxxxxxxxxxxx>]

Review: Needs Information

Looks good to me, just a couple of comments inline (clarification, nothing wrong)

Diff comments:

> === modified file 'CMakeLists.txt'
> --- CMakeLists.txt	2015-06-14 23:34:29 +0000
> +++ CMakeLists.txt	2015-07-05 19:26:31 +0000
> @@ -20,7 +20,7 @@
>  set(AUTOPILOT_DIR reminders)
>  set(APP_HARDCODE reminders)
>  set(MAIN_QML ${APP_HARDCODE}.qml)
> -set(EXEC "reminders %u")
> +set(EXEC "reminders %u -d JobQueue")

Why is that necessary?

>  set(UBUNTU_MANIFEST_PATH "manifest.json.in" CACHE INTERNAL "Relative path to the manifest file")
>  
>  set(ACCOUNT_ICON_DIR ${CMAKE_INSTALL_DATADIR}/icons/hicolor/32x32/apps)
> 
> === modified file 'src/app/qml/reminders.qml'
> --- src/app/qml/reminders.qml	2015-06-22 10:53:02 +0000
> +++ src/app/qml/reminders.qml	2015-07-05 19:26:31 +0000
> @@ -371,32 +371,35 @@
>          appId: root.applicationName + "_reminders"
>  
>          onNotificationsChanged: {
> -            print("PushClient notification:", notifications)
> -            var notification = JSON.parse(notifications)["payload"];
> -
> -            if (notification["userId"] != UserStore.userId) { // Yes, we want type coercion here.
> -                console.warn("user mismatch:", notification["userId"], "!=", UserStore.userId)
> -                return;
> -            }
> -
> -            switch(notification["reason"]) {
> -            case "update":
> -                print("Note updated on server:", notification["guid"])
> -                if (NotesStore.note(notification["guid"]) === null) {
> +            print("Received PushClient notifications:", notifications.length)
> +            for (var i = 0; i < notifications.length; i++) {
> +                print("notification", i, ":", notifications[i])
> +                var notification = JSON.parse(notifications[i])["payload"];
> +
> +                if (notification["userId"] != UserStore.userId) { // Yes, we want type coercion here.

What if a user has more than one account?

> +                    console.warn("user mismatch:", notification["userId"], "!=", UserStore.userId)
> +                    return;
> +                }
> +
> +                switch(notification["reason"]) {
> +                case "update":
> +                    print("Note updated on server:", notification["guid"])
> +                    if (NotesStore.note(notification["guid"]) === null) {
> +                        NotesStore.refreshNotes();
> +                    } else {
> +                        NotesStore.refreshNoteContent(notification["guid"]);
> +                    }
> +                    break;
> +                case "create":
> +                    print("New note appeared on server:", notification["guid"])
>                      NotesStore.refreshNotes();
> -                } else {
> -                    NotesStore.refreshNoteContent(notification["guid"]);
> +                    break;
> +                case "notebook_update":
> +                    NotesStore.refreshNotebooks();
> +                    break;
> +                default:
> +                    console.warn("Unhandled push notification:", notification["reason"])
>                  }
> -                break;
> -            case "create":
> -                print("New note appeared on server:", notification["guid"])
> -                NotesStore.refreshNotes();
> -                break;
> -            case "notebook_update":
> -                NotesStore.refreshNotebooks();
> -                break;
> -            default:
> -                console.warn("Unhandled push notification:", notification["reason"])
>              }
>          }
>  


-- 
https://code.launchpad.net/~mzanetti/reminders-app/fix-writeback-issue/+merge/263848
Your team Ubuntu Reminders app developers is subscribed to branch lp:reminders-app.