Merge lp:~pkunal-parmar/ubuntu-calendar-app/nextYearFix into lp:ubuntu-calendar-app

Proposed by Kunal Parmar
Status: Merged
Approved by: Olivier Tilloy
Approved revision: 194
Merged at revision: 195
Proposed branch: lp:~pkunal-parmar/ubuntu-calendar-app/nextYearFix
Merge into: lp:ubuntu-calendar-app
Diff against target: 195 lines (+32/-23)
7 files modified
MonthView.qml (+4/-2)
PathViewBase.qml (+5/-4)
TimeLineHeader.qml (+1/-1)
WeekView.qml (+2/-2)
YearView.qml (+4/-8)
tests/autopilot/calendar_app/tests/test_weekview.py (+2/-1)
tests/autopilot/calendar_app/tests/test_yearview.py (+14/-5)
To merge this branch: bzr merge lp:~pkunal-parmar/ubuntu-calendar-app/nextYearFix
Reviewer Review Type Date Requested Status
Olivier Tilloy Approve
Ubuntu Phone Apps Jenkins Bot continuous-integration Approve
Review via email: mp+197624@code.launchpad.net

Commit message

Fix for "Buffered next year is wrong" from observation reported by jacekn on
https://bugs.launchpad.net/ubuntu-calendar-app/+bug/1222846

"If I do it slowly and move it just enough to see the year it is initially showing previous year. Then when I scroll a bit more then is short lock up and year is update to what it should be."

Description of the change

Fix for "Buffered next year is wrong" from observation reported by jacekn on
https://bugs.launchpad.net/ubuntu-calendar-app/+bug/1222846

"If I do it slowly and move it just enough to see the year it is initially showing previous year. Then when I scroll a bit more then is short lock up and year is update to what it should be."

To post a comment you must log in.
Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Olivier Tilloy (osomon) wrote :

This fixes the issue raised by Jacek. However this code could be made much easier to read. How about:

    property var year: getDateFromYear(getYear())
    function getYear() {
        switch( root.indexType(index)) {
        case 0:
            return currentYear.getFullYear() - 1
        case -1:
            return currentYear.getFullYear() + 1
        case 1:
            return currentYear.getFullYear()
        }
    }

And while we’re at it, I don’t understand how come case 0 is not for current year, -1 for previous and +1 for next, as function indexType(…) documents it. This is confusing, can you shed a light on this?

review: Needs Fixing
174. By Kunal Parmar

merge from trunk

Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Approve (continuous-integration)
175. By Kunal Parmar

PathView changed

Revision history for this message
Kunal Parmar (pkunal-parmar) wrote :

> And while we’re at it, I don’t understand how come case 0 is not for current
> year, -1 for previous and +1 for next, as function indexType(…) documents it.
> This is confusing, can you shed a light on this?

This was the case due to how Path was created in PathView, now I changed path in pathview and it works as expected.

0 = visible item
-1 = previous item
1 = next item

Please review again

Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Needs Fixing (continuous-integration)
176. By Kunal Parmar

yearview horizontal scrolling fix

Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Needs Fixing (continuous-integration)
177. By Kunal Parmar

Autopilot for YearView fixed

178. By Kunal Parmar

weekview autopilot test fix

Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Needs Fixing (continuous-integration)
179. By Kunal Parmar

delay year update rollback

180. By Kunal Parmar

merge from trunk

Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Olivier Tilloy (osomon) wrote :

82 + //To make horizontal movement smooth, not using bindig for year
83 + /*Component.onCompleted: {
84 + year = getYear();
85 + }
86 +
87 + Connections{
88 + target:root
89 + onMovementEnded: {
90 + //update only when movement ends, to avoid calculation while animation
91 + year = getYear();
92 + }
93 + }*/
94 +

Is this commented out code a leftover? Can it be removed?

Revision history for this message
Olivier Tilloy (osomon) wrote :

71 QtObject{
72 id: intern
73 - property var startYear: getDateFromYear(currentYear.getFullYear()-1);
74 + property var startYear: currentYear;
75 }

It looks like this 'intern' object has become useless, can’t we remove it and replace all instances of intern.startYear with currentYear?

Revision history for this message
Olivier Tilloy (osomon) wrote :

121 months = year_grid.select_many("MonthComponent")
122 self.assert_current_year_is_default_one(months[0])
123
124 - february = months[1]
125 + february = months[0]

No way February is the first month of the year.
select_many(…) doesn’t guarantee the order of the returned elements, which is why it seems January doesn’t come first, so hardcoding the index of the month is unreliable and this test is flaky.
What you need to do is to sort the list of months using their properties before accessing them by index. Something like that:

  months.sort(key=lambda month: month.monthDate)

With that, you can revert the index change, you will be guaranteed that february == months[1]

review: Needs Fixing
Revision history for this message
Kunal Parmar (pkunal-parmar) wrote :

> 82 + //To make horizontal movement smooth, not using bindig for
> year
> 83 + /*Component.onCompleted: {
> 84 + year = getYear();
> 85 + }
> 86 +
> 87 + Connections{
> 88 + target:root
> 89 + onMovementEnded: {
> 90 + //update only when movement ends, to avoid
> calculation while animation
> 91 + year = getYear();
> 92 + }
> 93 + }*/
> 94 +
>
> Is this commented out code a leftover? Can it be removed?

Hi, I will remove it for now,
But it's possible, can you checkout this code and see how it works ?
This code I wrote to avoid lag while changing year by swiping left right. on my device it seems to work good, I commented out code as it was failing test case.

But if you find code it good then I can try to fix testcase instead and keep this code.

Revision history for this message
Kunal Parmar (pkunal-parmar) wrote :

> 71 QtObject{
> 72 id: intern
> 73 - property var startYear:
> getDateFromYear(currentYear.getFullYear()-1);
> 74 + property var startYear: currentYear;
> 75 }
>
> It looks like this 'intern' object has become useless, can’t we remove it and
> replace all instances of intern.startYear with currentYear?

removed

181. By Kunal Parmar

Commented out code removed

182. By Kunal Parmar

Intern object in yearview removed

183. By Kunal Parmar

test case fix

Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Kunal Parmar (pkunal-parmar) wrote :

> 121 months = year_grid.select_many("MonthComponent")
> 122 self.assert_current_year_is_default_one(months[0])
> 123
> 124 - february = months[1]
> 125 + february = months[0]
>
> No way February is the first month of the year.
> select_many(…) doesn’t guarantee the order of the returned elements, which is
> why it seems January doesn’t come first, so hardcoding the index of the month
> is unreliable and this test is flaky.
> What you need to do is to sort the list of months using their properties
> before accessing them by index. Something like that:
>
> months.sort(key=lambda month: month.monthDate)
>

Done, can you review again
> With that, you can revert the index change, you will be guaranteed that
> february == months[1]

Revision history for this message
Olivier Tilloy (osomon) wrote :

> > 82 + //To make horizontal movement smooth, not using bindig for
> > year
> > 83 + /*Component.onCompleted: {
> > 84 + year = getYear();
> > 85 + }
> > 86 +
> > 87 + Connections{
> > 88 + target:root
> > 89 + onMovementEnded: {
> > 90 + //update only when movement ends, to avoid
> > calculation while animation
> > 91 + year = getYear();
> > 92 + }
> > 93 + }*/
> > 94 +
> >
> > Is this commented out code a leftover? Can it be removed?
>
> Hi, I will remove it for now,
> But it's possible, can you checkout this code and see how it works ?
> This code I wrote to avoid lag while changing year by swiping left right. on
> my device it seems to work good, I commented out code as it was failing test
> case.
>
> But if you find code it good then I can try to fix testcase instead and keep
> this code.

Let’s do this in a separate branch/MR, please. I’ll gladly test it then.

Revision history for this message
Olivier Tilloy (osomon) wrote :

Because of bug #1268574 and bug #1268640, I fail to run the autopilot tests both locally and on my device (Galaxy Nexus). Therefore I cannot verify that this code doesn’t introduce regressions, even though it seems to fix the issue it claims it fixes.

Revision history for this message
Olivier Tilloy (osomon) wrote :

119 months = year_grid.select_many("MonthComponent")
120 self.assert_current_year_is_default_one(months[0])
121
122 + months.sort(key=lambda month: month.monthDate)

This works, but for corectness the sort should be done before asserting anything on the months:

    months = year_grid.select_many("MonthComponent")
    months.sort(key=lambda month: month.monthDate)
    self.assert_current_year_is_default_one(months[0])

review: Needs Fixing
Revision history for this message
Olivier Tilloy (osomon) wrote :

105 + timeline = header.select_many("TimeLineHeaderComponent")[1]
118 + year_grid = self.year_view.select_many("QQuickGridView")[1]
131 + selected_month = month_view.select_many("MonthComponent")[1]
140 + year_grid = self.year_view.select_many("QQuickGridView")[1]

As I pointed out earlier, select_many(…) doesn’t guarantee the return order, so this might break at any point in time. If you want to get a specific component, either use specific properties like the objectName or a property that identifies it unambiguously, or sort the list returned by select_many(…) using an unambiguous key.

review: Needs Fixing
184. By Kunal Parmar

YearView autopilot text fix

185. By Kunal Parmar

Other testcase fixed

Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Needs Fixing (continuous-integration)
186. By Kunal Parmar

error resolved

187. By Kunal Parmar

pyflakes error resolved

188. By Kunal Parmar

pyflakes resolved

189. By Kunal Parmar

merge from trunk

Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Needs Fixing (continuous-integration)
190. By Kunal Parmar

WeekView test case fix

Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Needs Fixing (continuous-integration)
191. By Kunal Parmar

trobleshooting test

Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Needs Fixing (continuous-integration)
192. By Kunal Parmar

test fail fixed

Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Kunal Parmar (pkunal-parmar) wrote :

> 105 + timeline = header.select_many("TimeLineHeaderComponent")[1]
> 118 + year_grid = self.year_view.select_many("QQuickGridView")[1]
> 131 + selected_month = month_view.select_many("MonthComponent")[1]
> 140 + year_grid = self.year_view.select_many("QQuickGridView")[1]
>
> As I pointed out earlier, select_many(…) doesn’t guarantee the return order,
> so this might break at any point in time. If you want to get a specific
> component, either use specific properties like the objectName or a property
> that identifies it unambiguously, or sort the list returned by select_many(…)
> using an unambiguous key.

I fixed hard coded index issue, kindly have another look

Revision history for this message
Olivier Tilloy (osomon) wrote :

130 + compArray = header.select_many("TimeLineHeaderComponent")
131 + timeline = None
132 +
133 + for tilelineComponent in compArray:
134 + if tilelineComponent.isCurrentItem:
135 + timeline = tilelineComponent
136 + break

Autopilot allows to do this in a much simpler way:

    timeline = header.select_single("TimeLineHeaderComponent", isCurrentItem=True)

The same applies to get_current_year() and to selected_month.

review: Needs Fixing
193. By Kunal Parmar

Merge from trunk

194. By Kunal Parmar

review comment

Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Olivier Tilloy (osomon) wrote :

Looks good now, thanks!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'MonthView.qml'
2--- MonthView.qml 2014-01-09 23:15:57 +0000
3+++ MonthView.qml 2014-02-10 13:56:10 +0000
4@@ -14,7 +14,7 @@
5 PathViewBase{
6 id: monthViewPath
7
8- property var startMonth: addMonth(currentMonth,-1);
9+ property var startMonth: currentMonth;
10
11 anchors.top:parent.top
12
13@@ -44,6 +44,8 @@
14 }
15
16 delegate: MonthComponent{
17+ property bool isCurrentItem: index === monthViewPath.currentIndex
18+
19 width: parent.width - units.gu(5)
20 height: parent.height - units.gu(5)
21
22@@ -54,7 +56,7 @@
23 case 0:
24 return monthViewPath.startMonth;
25 case -1:
26- return monthViewPath.addMonth(monthViewPath.startMonth,2);
27+ return monthViewPath.addMonth(monthViewPath.startMonth,-1);
28 case 1:
29 return monthViewPath.addMonth(monthViewPath.startMonth,1);
30 }
31
32=== modified file 'PathViewBase.qml'
33--- PathViewBase.qml 2013-09-16 14:04:01 +0000
34+++ PathViewBase.qml 2014-02-10 13:56:10 +0000
35@@ -9,11 +9,12 @@
36 signal nextItemHighlighted();
37 signal previousItemHighlighted();
38
39+ preferredHighlightBegin: 0.5
40+ preferredHighlightEnd: 0.5
41+
42 path: Path {
43- startX: -(root.width/2); startY: root.height/2
44- PathLine { relativeX: root.width; relativeY: 0 }
45- PathLine { relativeX: root.width; relativeY: 0 }
46- PathLine { relativeX: root.width; relativeY: 0 }
47+ startX: -(root.width); startY: root.height/2
48+ PathLine { x: (root.width)*2 ; relativeY: 0; }
49 }
50
51 // 0= current index, -1= previous index, 1 next index
52
53=== modified file 'TimeLineHeader.qml'
54--- TimeLineHeader.qml 2013-09-28 09:37:52 +0000
55+++ TimeLineHeader.qml 2014-02-10 13:56:10 +0000
56@@ -64,7 +64,7 @@
57 case 0:
58 return date;
59 case -1:
60- return date.addDays(14);
61+ return date.addDays(-7);
62 case 1:
63 return date.addDays(7);
64 }
65
66=== modified file 'WeekView.qml'
67--- WeekView.qml 2014-01-28 01:51:29 +0000
68+++ WeekView.qml 2014-02-10 13:56:10 +0000
69@@ -31,7 +31,7 @@
70 id: weekViewPath
71
72 property var visibleWeek: dayStart.weekStart(Qt.locale().firstDayOfWeek);
73- property var weekStart: weekViewPath.visibleWeek.addDays(-7)
74+ property var weekStart: weekViewPath.visibleWeek
75
76 width: parent.width
77 height: root.height - weekViewPath.y
78@@ -71,7 +71,7 @@
79 return weekViewPath.weekStart;
80 case -1:
81 var weekStartDay= weekViewPath.weekStart.weekStart(Qt.locale().firstDayOfWeek);
82- return weekStartDay.addDays(14);
83+ return weekStartDay.addDays(-7);
84 case 1:
85 var weekStartDay = weekViewPath.weekStart.weekStart(Qt.locale().firstDayOfWeek);
86 return weekStartDay.addDays(7);
87
88=== modified file 'YearView.qml'
89--- YearView.qml 2014-01-09 23:15:57 +0000
90+++ YearView.qml 2014-02-10 13:56:10 +0000
91@@ -25,25 +25,21 @@
92 return new Date(year,0,1,0,0,0,0);
93 }
94
95- QtObject{
96- id: intern
97- property var startYear: getDateFromYear(currentYear.getFullYear()-1);
98- }
99-
100 delegate: GridView{
101 id: yearView
102 clip: true
103
104+ property bool isCurrentItem: index == root.currentIndex
105 property var year: getYear();
106
107 function getYear() {
108 switch( root.indexType(index)) {
109 case 0:
110- return intern.startYear;
111+ return currentYear;
112 case -1:
113- return getDateFromYear(intern.startYear.getFullYear() - 1);
114+ return getDateFromYear(currentYear.getFullYear() - 1);
115 case 1:
116- return getDateFromYear(intern.startYear.getFullYear() + 1);
117+ return getDateFromYear(currentYear.getFullYear() + 1);
118 }
119 }
120
121
122=== modified file 'tests/autopilot/calendar_app/tests/test_weekview.py'
123--- tests/autopilot/calendar_app/tests/test_weekview.py 2014-01-09 23:30:55 +0000
124+++ tests/autopilot/calendar_app/tests/test_weekview.py 2014-02-10 13:56:10 +0000
125@@ -70,7 +70,8 @@
126
127 def _get_date_label_headers(self):
128 header = self.main_view.select_single(objectName="weekHeader")
129- timeline = header.select_many("TimeLineHeaderComponent")[0]
130+ timeline = header.select_single("TimeLineHeaderComponent",
131+ isCurrentItem=True)
132 dateLabels = timeline.select_many("Label", objectName="dateLabel")
133 return dateLabels
134
135
136=== modified file 'tests/autopilot/calendar_app/tests/test_yearview.py'
137--- tests/autopilot/calendar_app/tests/test_yearview.py 2014-01-09 23:28:15 +0000
138+++ tests/autopilot/calendar_app/tests/test_yearview.py 2014-02-10 13:56:10 +0000
139@@ -10,7 +10,6 @@
140 """
141
142 from datetime import datetime
143-
144 from autopilot.matchers import Eventually
145 from testtools.matchers import Equals, NotEquals
146
147@@ -28,13 +27,19 @@
148
149 self.year_view = self.main_view.get_year_view()
150
151+ def get_current_year(self):
152+ return self.year_view.select_single("QQuickGridView",
153+ isCurrentItem=True)
154+
155 def test_selecting_a_month_switch_to_month_view(self):
156 """It must be possible to select a month and open the month view."""
157
158 # TODO: the component indexed at 1 is the one currently displayed,
159 # investigate a way to validate this assumption visually.
160- year_grid = self.year_view.select_many("QQuickGridView")[0]
161+ year_grid = self.get_current_year()
162+ self.assertThat(year_grid, NotEquals(None))
163 months = year_grid.select_many("MonthComponent")
164+ months.sort(key=lambda month: month.monthDate)
165 self.assert_current_year_is_default_one(months[0])
166
167 february = months[1]
168@@ -48,7 +53,11 @@
169
170 month_view = self.main_view.get_month_view()
171 self.assertThat(month_view.visible, Eventually(Equals(True)))
172- selected_month = month_view.select_many("MonthComponent")[0]
173+
174+ selected_month = month_view.select_single("MonthComponent",
175+ isCurrentItem=True)
176+
177+ self.assertThat(selected_month, NotEquals(None))
178
179 self.assertThat(self.main_view.get_year(selected_month),
180 Equals(expected_year))
181@@ -108,13 +117,13 @@
182 if now.month > 6:
183 self.drag_page_up()
184
185- year_grid = self.year_view.select_many("QQuickGridView")[0]
186+ year_grid = self.get_current_year()
187+ self.assertThat(year_grid, NotEquals(None))
188 months = year_grid.select_many("MonthComponent")
189
190 for month in months:
191 current_month_label = month.select_single(
192 "Label", objectName="monthLabel")
193-
194 if current_month_name == current_month_label.text:
195 return month
196

Subscribers

People subscribed via source and target branches

to status/vote changes: