Merge lp://staging/~paolorotolo/ubuntu-clock-app/fix-for-1291502 into lp://staging/ubuntu-clock-app/saucy

Proposed by Paolo Rotolo
Status: Rejected
Rejected by: Nekhelesh Ramananthan
Proposed branch: lp://staging/~paolorotolo/ubuntu-clock-app/fix-for-1291502
Merge into: lp://staging/ubuntu-clock-app/saucy
To merge this branch: bzr merge lp://staging/~paolorotolo/ubuntu-clock-app/fix-for-1291502
Reviewer Review Type Date Requested Status
Nekhelesh Ramananthan Needs Fixing
Ubuntu Phone Apps Jenkins Bot continuous-integration Pending
Review via email: mp+211405@code.staging.launchpad.net

Commit message

- Stopwatch: Lap list now disappears automatically when empty;
- Stopwatch: Added new animation of text label;
Fixes LP: #1291502.

Description of the change

- Stopwatch: Lap list now disappears automatically when empty;
- Stopwatch: Added new animation of text label;
Fixes LP: #1291502.

To post a comment you must log in.
Revision history for this message
Nekhelesh Ramananthan (nik90) wrote :

Hey Paolo, thnx for working on this. For some reason launchpad is not showing me the code diff, so in case you are unable to find some of the things in my review, feel free to ask me.

I believe Lucas's design solution for this bug can be found at https://launchpadlibrarian.net/169527292/Captura%20de%20pantalla%20de%202014-03-14%2017%3A11%3A17.png.

I noticed that you hide the Recorded Laps header when the laps count is 0. However if you look at lucas's design you will notice that it is supposed to slide in from the bottom to let the user know that something is present in the bottom. Can you fix this pls.

review: Needs Fixing
Revision history for this message
Nekhelesh Ramananthan (nik90) wrote :

Another thing I noticed is that you used a timer in the StopwatchPage.qml,

// Timer to restore original size of the test after a new lap.
    Timer {
        id: animation_timer
        interval: 100
        repeat: false
        onTriggered: {
            analogStopwatch.innerLabel.font.pixelSize = units.dp(41);
        }
    }

I am pretty sure that we don't need this timer at all. What you need to do here is use a sequential animation which first increases the text size to units.dp(41) and then decrease it back to units.dp(30). I have added a sample code below to help you.

     SequentialAnimation {
         id: fontAnimation
         UbuntuNumberAnimation { target: innerLabel; property: "font.pixelSize"; to: units.dp(41) }
         UbuntuNumberAnimation { target: innerLabel; property: "font.pixelSize"; to: units.dp(30) }
     }

And then when you press the lap button, you can start the animation by,

     fontAnimation.start()

For more information on sequential animation, please refer to http://qt-project.org/doc/qt-4.8/qml-sequentialanimation.html

review: Needs Fixing
Revision history for this message
Nekhelesh Ramananthan (nik90) wrote :

I noticed this line code below,
      if (laps.count !== 0, analogStopwatch.timerStatus == true ) {

I am pretty sure you cannot use a comma (,) like shown above :P. Can you fix this as well.

review: Needs Fixing
Revision history for this message
Paolo Rotolo (paolorotolo) wrote :

About diff, Launchpad sent me this mail:
"Launchpad encountered an internal error during the following operation: generating the diff for a merge proposal. It was logged with id OOPS-af32959a101fe70a730c7a28b7afb201. Sorry for the inconvenience."

Revision history for this message
Nekhelesh Ramananthan (nik90) wrote :

Yup I got that email as well 5 times :P. I guess we need to work on this MP without the code diff ;)

Revision history for this message
Nekhelesh Ramananthan (nik90) wrote :

I fixed this bug in another MP at https://code.launchpad.net/~nik90/ubuntu-clock-app/add-stopwatch-lap-animation/+merge/212889. I discussed with Lucas and we decided to remove the text size animation as it was distracting. Please help review the other MP I created.

I am also disapproving this MP since Launchpad isn't showing the diff which I desperately need to see what changed.

Unmerged revisions

378. By Paolo Rotolo

Fixed some issue with animation.

377. By Paolo Rotolo

 - Stopwatch: Lap list now disappears automatically when empty.
 - Stopwatch: Added new animation of text label.

Fixes LP: #1291502.

Subscribers

People subscribed via source and target branches