ubuntu-touch-coreapps-reviewers team mailing list archive

[Renato Araujo Oliveira Filho <renato.filho@xxxxxxxxxxxxx>]

@nick90: Since you are revamp the page is better to fix/update wrong code if necessary.

Check my inline comments.

Diff comments:

> 
> === modified file 'EventDetails.qml'
> --- EventDetails.qml	2016-03-02 19:55:52 +0000
> +++ EventDetails.qml	2016-03-09 18:35:25 +0000
> @@ -223,37 +228,22 @@
>          })
>      }
>  
> -    Keys.onEscapePressed: {
> -        pageStack.pop();
> -    }
> -
> -    Keys.onPressed: {
> -        if ((event.key === Qt.Key_E) && ( event.modifiers & Qt.ControlModifier)) {
> -            showEditEventPage(event, model);
> -        }
> -    }
> -
> -    head.actions: [
> -        Action {
> -            text: i18n.tr("Edit");
> -            objectName: "edit"
> -            iconName: "edit";
> -            onTriggered: {
> -                if( event.itemType === Type.EventOccurrence ) {
> -                    var dialog = PopupUtils.open(Qt.resolvedUrl("EditEventConfirmationDialog.qml"),root,{"event": event});
> -                    dialog.editEvent.connect( function(eventId){
> -                        if( eventId === event.parentId ) {
> -                            showEditEventPage(internal.parentEvent, model)
> -                        } else {
> -                            showEditEventPage(event, model)
> -                        }
> -                    });
> -                } else {
> -                    showEditEventPage(event, model)
> -                }
> +    Connections{
> +        target: pageStack
> +        onCurrentPageChanged:{
> +            if( pageStack.currentPage === root) {

>From the function name, I think it will populate the view with the event information? Is that correct?
But looking at the MR I noticed that most of the fields already have bindings for the event properties? Is this really necessary? 

If yes, we need to check if it is not breaking any already existing binding. 
This should not be here,  the best place to call this function is on 'onEventChanged' and probably will need to be connected to signal 'onItemChanged' from event object. Something like: Connections { target: event; onItemChanged: showEvent(event); }

> +                showEvent(event)
>              }
>          }
> -    ]
> +    }
> +
> +    RemindersModel {
> +        id: reminderModel
> +    }
> +
> +    ListModel {
> +        id: contactModel
> +    }
>  
>      EventUtils{
>          id:eventUtils


-- 
https://code.launchpad.net/~nik90/ubuntu-calendar-app/revamp-eventdetails-page/+merge/288492
Your team Ubuntu Calendar Developers is requested to review the proposed merge of lp:~nik90/ubuntu-calendar-app/revamp-eventdetails-page into lp:ubuntu-calendar-app.