Merge lp:~mihirsoni/ubuntu-calendar-app/1308001 into lp:ubuntu-calendar-app

[Mihir Soni]

Event bubble height issue

Bug #1308001
Event bubble height issues.

[Mihir Soni (mihirsoni)]

It looks like :- http://imgur.com/yhzbElj

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

PASSED: Continuous integration, rev:258
http://91.189.93.70:8080/job/ubuntu-calendar-app-ci/425/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2305
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-calendar-app-raring-amd64-ci/425
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-calendar-app-saucy-amd64-ci/425
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-calendar-app-trusty-amd64-ci/259

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-calendar-app-ci/425/rebuild

review: Approve (continuous-integration)

[Kunal Parmar (pkunal-parmar)]

We need to apply margin on top and bottom as well. From image it looks like its missing

[Kunal Parmar (pkunal-parmar)]

+ x:units.gu(1)

Please use anchors for alighments

review: Needs Fixing

[Kunal Parmar (pkunal-parmar)]

64 + property int hourHeight: units.gu(12)

why we are changing the hourHeight ?

And if you change here I believe there are other place as well where we need to make changes related to this.

review: Needs Information

[Mihir Soni (mihirsoni)]

> 64 + property int hourHeight: units.gu(12)
>
> why we are changing the hourHeight ?
>
> And if you change here I believe there are other place as well where we need
> to make changes related to this.

Hi Kunal,It is being used to define the minimum height of the bubble. if you see the rest of the code where hourHeight is being used.

There is no other impact as far as I tested.

[Alan Pope 🍺🐧🐱 🦄 (popey)]

Comparing this merge with the design image from dpm in the linked bug.

This merge: https://imgur.com/fYWwYCn
Design from dpm: https://launchpadlibrarian.net/172944464/calendar_item.jpg

* The dot is too close to the edge
* There's insufficient border around the text
* The location is displayed

review: Needs Fixing

[Kunal Parmar (pkunal-parmar)]

> > 64 + property int hourHeight: units.gu(12)
> >
> > why we are changing the hourHeight ?
> >
> > And if you change here I believe there are other place as well where we need
> > to make changes related to this.
>
> Hi Kunal,It is being used to define the minimum height of the bubble. if you
> see the rest of the code where hourHeight is being used.
>
> There is no other impact as far as I tested.

hourHeight is height of strip. Not event bubble.

Event bubble height is decided by following logic

 var durationMin = (event.endDateTime.getHours() - event.startDateTime.getHours()) * 60;
        durationMin += (event.endDateTime.getMinutes() - event.startDateTime.getMinutes());
        var height = (durationMin * hourHeight )/ 60;
        eventBubble.height = (height > eventBubble.minimumHeight) ? height:eventBubble.minimumHeight ;

review: Needs Fixing

[Mihir Soni (mihirsoni)]

This is how it looks like now,
http://imgur.com/iz0T5SP

Kunal, i have reverted the changes in Timelinebase.qml file.

Would appreciate your feedback.

[lp:~mihirsoni/ubuntu-calendar-app/1308001]

259. By Mihir Soni on 2014-04-30

Description will be visible only if is fit to event bubble

260. By Mihir Soni on 2014-04-30

Resolved typo in color

[David Planella (dpm)]

Mihir, great work on that! One thing I've noticed is that the dot should have the same spacing to the top and to the right edges as the text. Now it seems that the dot is closer to the edges than the text

review: Needs Fixing

[lp:~mihirsoni/ubuntu-calendar-app/1308001]

261. By Mihir Soni on 2014-04-30

Same margin as text

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

PASSED: Continuous integration, rev:260
http://91.189.93.70:8080/job/ubuntu-calendar-app-ci/426/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2323
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-calendar-app-raring-amd64-ci/426
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-calendar-app-saucy-amd64-ci/426
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-calendar-app-trusty-amd64-ci/260

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-calendar-app-ci/426/rebuild

review: Approve (continuous-integration)

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

PASSED: Continuous integration, rev:261
http://91.189.93.70:8080/job/ubuntu-calendar-app-ci/427/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2324
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-calendar-app-raring-amd64-ci/427
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-calendar-app-saucy-amd64-ci/427
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-calendar-app-trusty-amd64-ci/261

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-calendar-app-ci/427/rebuild

review: Approve (continuous-integration)

[Alan Pope 🍺🐧🐱 🦄 (popey)]

Still seems *slightly* off.

https://imgur.com/DvPadti

Doesn't look uniformly distant from top and right edge of event bubble?

review: Needs Fixing

[lp:~mihirsoni/ubuntu-calendar-app/1308001]

262. By Mihir Soni on 2014-05-02

slight margin issues

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

PASSED: Continuous integration, rev:262
http://91.189.93.70:8080/job/ubuntu-calendar-app-ci/430/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2370
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-calendar-app-raring-amd64-ci/430
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-calendar-app-saucy-amd64-ci/430
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-calendar-app-trusty-amd64-ci/264

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-calendar-app-ci/430/rebuild

review: Approve (continuous-integration)

[Alan Pope 🍺🐧🐱 🦄 (popey)]

https://imgur.com/Qb1qKAx

Looks great!

review: Approve

[Kunal Parmar (pkunal-parmar)]

9 + if( event.description && descriptionLabel.height > height) //If content is too much don't display.
10 descriptionLabel.text = event.description

Can you explain the logic, why description should be set only when description label's height is grater then height of bubble ?

review: Needs Information

[Mihir Soni (mihirsoni)]

> 9 + if( event.description && descriptionLabel.height >
> height) //If content is too much don't display.
> 10 descriptionLabel.text = event.description
>
> Can you explain the logic, why description should be set only when description
> label's height is grater then height of bubble ?

Hi kunal,

it was suggested by David , you can find it in description of bug #1308001

[Kunal Parmar (pkunal-parmar)]

> > 9 + if( event.description && descriptionLabel.height >
> > height) //If content is too much don't display.
> > 10 descriptionLabel.text = event.description
> >
> > Can you explain the logic, why description should be set only when
> description
> > label's height is grater then height of bubble ?
>
> Hi kunal,
>
> it was suggested by David , you can find it in description of bug #1308001

Yes but, in that case height of description should be less then height of bubble. And not otherwise.

[Mihir Soni (mihirsoni)]

Kunal like this ?

if (event.description && height > descriptionLabel.height)

[Mihir Soni (mihirsoni)]

> > > 9 + if( event.description && descriptionLabel.height
> >
> > > height) //If content is too much don't display.
> > > 10 descriptionLabel.text = event.description
> > >
> > > Can you explain the logic, why description should be set only when
> > description
> > > label's height is grater then height of bubble ?
> >
> > Hi kunal,
> >
> > it was suggested by David , you can find it in description of bug #1308001
>
> Yes but, in that case height of description should be less then height of
> bubble. And not otherwise.

If you try otherwise , it is showing event description if it is not able to fit in event bubble.

[Kunal Parmar (pkunal-parmar)]

> > > > 9 + if( event.description &&
> descriptionLabel.height
> > >
> > > > height) //If content is too much don't display.
> > > > 10 descriptionLabel.text = event.description
> > > >
> > > > Can you explain the logic, why description should be set only when
> > > description
> > > > label's height is grater then height of bubble ?
> > >
> > > Hi kunal,
> > >
> > > it was suggested by David , you can find it in description of bug #1308001
> >
> > Yes but, in that case height of description should be less then height of
> > bubble. And not otherwise.
>
> If you try otherwise , it is showing event description if it is not able to
> fit in event bubble.

You will need to consider y position as well, just checking height will not help.

[Mihir Soni (mihirsoni)]

Hi Kunal,

sorry for more question , but i am misunderstanding your comment.

if( event.description && height > descriptionLabel.height && width < descriptionLabel.width)

I did try above statement and it gave me same output as before with no change.

[Kunal Parmar (pkunal-parmar)]

> Hi Kunal,
>
> sorry for more question , but i am misunderstanding your comment.
>
> if( event.description && height > descriptionLabel.height && width <
> descriptionLabel.width)
>
> I did try above statement and it gave me same output as before with no change.
.
As far as I understand, you need to check descriptionLabel.y + descriptionLabel.height. and those should be less then height of bubble to make description visible.

Or you can check total height of column and compare that to height of bubble to decide if description label should be visible or not.

[Mihir Soni (mihirsoni)]

Hi Kunal,

I did try as you suggested but it is not working as expected like :

I have event with huge description , which shouldn't display but it is displaying if i use following condition

     if( event.description && descriptionLabel.y + descriptionLabel.height < height)

http://imgur.com/H4hrDd3

[Kunal Parmar (pkunal-parmar)]

Pls print the values and see what vlaues are coming and check if those are same with what you expect.

[lp:~mihirsoni/ubuntu-calendar-app/1308001]

263. By Mihir Soni on 2014-05-06

modfied the condition

264. By Mihir Soni on 2014-05-06

merge with trunk

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

PASSED: Continuous integration, rev:264
http://91.189.93.70:8080/job/ubuntu-calendar-app-ci/437/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2443
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-calendar-app-saucy-amd64-ci/437
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-calendar-app-trusty-amd64-ci/271

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-calendar-app-ci/437/rebuild

review: Approve (continuous-integration)

[Kunal Parmar (pkunal-parmar)]

25 + Item{
26 id: detailsColumn
27 width: parent.width

I think we dont need this new Item, you can directly assign margin to column element.

review: Needs Fixing

[Kunal Parmar (pkunal-parmar)]

31 + Column{
32 width: parent.width

You have put anchors.fill: parent, so we dont need to assign width, we can just remove above statement

review: Needs Fixing

[Kunal Parmar (pkunal-parmar)]

45 + anchors.fill: parent
46 + anchors.topMargin: units.gu(0.5)
47 + anchors.leftMargin: units.gu(1)

Why topMargin and leftMargin is not similar ? For symmetry I would expect it to be same.

review: Needs Information

[Kunal Parmar (pkunal-parmar)]

46 + anchors.topMargin: units.gu(0.5)
47 + anchors.leftMargin: units.gu(1)

We can put rightMargin as well here and simplify below statement. (- units.gu(0.5) won't be necessary)

54 + color:"gray"
55 + width: parent.width - rect.width - units.gu(0.5)

review: Needs Fixing

[lp:~mihirsoni/ubuntu-calendar-app/1308001]

265. By Mihir Soni on 2014-05-08

modification as per review comments

266. By Mihir Soni on 2014-05-08

Merge with trunk

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

PASSED: Continuous integration, rev:266
http://91.189.93.70:8080/job/ubuntu-calendar-app-ci/446/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2497
        deb: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2497/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-utopic/31
        deb: http://91.189.93.70:8080/job/generic-mediumtests-utopic/31/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-calendar-app-trusty-amd64-ci/280
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-calendar-app-utopic-amd64-ci/4

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-calendar-app-ci/446/rebuild

review: Approve (continuous-integration)

[Kunal Parmar (pkunal-parmar)]

I meant something like this

http://pastebin.ubuntu.com/7415851/

[lp:~mihirsoni/ubuntu-calendar-app/1308001]

267. By Mihir Soni on 2014-05-09

changes as per review comments

268. By Mihir Soni on 2014-05-09

Merge from trunk

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

PASSED: Continuous integration, rev:268
http://91.189.93.70:8080/job/ubuntu-calendar-app-ci/456/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2556
        deb: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2556/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-utopic/92
        deb: http://91.189.93.70:8080/job/generic-mediumtests-utopic/92/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-calendar-app-trusty-amd64-ci/290
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-calendar-app-utopic-amd64-ci/14

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-calendar-app-ci/456/rebuild

review: Approve (continuous-integration)

[Kunal Parmar (pkunal-parmar)]

64 + anchors.bottom: parent.Bottom
65 +

Can you check if we need this ?

[lp:~mihirsoni/ubuntu-calendar-app/1308001]

269. By Mihir Soni on 2014-05-09

Removed anchors

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

PASSED: Continuous integration, rev:269
http://91.189.93.70:8080/job/ubuntu-calendar-app-ci/457/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2558
        deb: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2558/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-utopic/94
        deb: http://91.189.93.70:8080/job/generic-mediumtests-utopic/94/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-calendar-app-trusty-amd64-ci/291
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-calendar-app-utopic-amd64-ci/15

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-calendar-app-ci/457/rebuild

review: Approve (continuous-integration)

[Kunal Parmar (pkunal-parmar)]

looks good, thanks

review: Approve

=== modified file 'EventBubble.qml'
--- EventBubble.qml	2014-03-26 14:29:37 +0000
+++ EventBubble.qml	2014-05-09 07:52:55 +0000
@@ -56,9 +56,14 @@
 
                 if( event.displayLabel)
                     titleLabel.text = event.displayLabel;
-
                 if( event.description)
+                {
                     descriptionLabel.text = event.description
+                    //If content is too much don't display.
+                    if( height < descriptionLabel.height + descriptionLabel.y){
+                        descriptionLabel.text = ""
+                    }
+                }
             } else {
                 //narrow type shows only time and title
                 timeLabel.text = startTime
@@ -103,31 +108,31 @@
 
     Column{
         id: detailsColumn
-        width: parent.width
+
+        anchors.fill: parent
+        anchors.topMargin: units.gu(1)
+        anchors.leftMargin: units.gu(1)
+        anchors.rightMargin: units.gu(1)
 
         Row{
             width: parent.width
 
+            Label{
+                id: timeLabel
+                fontSize:"small";
+                color:"gray"
+                width: parent.width - rect.width
+            }
             Rectangle{
+                id:rect
                 width: units.gu(1)
                 radius: width/2
                 height: width
                 color: "#715772"
-                anchors.verticalCenter: parent.verticalCenter
-                antialiasing: true
-            }
-
-            Label{
-                id: timeLabel
-                fontSize:"small";
-                color:"gray"
-                width: parent.width
             }
         }
-
         Label{
             id: titleLabel
-            x: units.gu(1)
             fontSize:"small";
             color:"black"
             wrapMode: Text.WrapAtWordBoundaryOrAnywhere
@@ -136,7 +141,6 @@
 
         Label{
             id: descriptionLabel
-            x: units.gu(1)
             fontSize:"small";
             color:"gray"
             wrapMode: Text.WrapAtWordBoundaryOrAnywhere
@@ -145,6 +149,7 @@
         }
     }
 
+
     MouseArea{
         anchors.fill: parent
         onClicked: {