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

[Kunal Parmar]

TimeLine view added

TimeLine view added

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

PASSED: Continuous integration, rev:22
http://91.189.93.125:8080/job/ubuntu-calendar-app-ci/1/
Executed test runs:
    SUCCESS: http://91.189.93.125:8080/job/ubuntu-calendar-app-quantal-amd64-ci/1
    SUCCESS: http://91.189.93.125:8080/job/ubuntu-calendar-app-raring-amd64-ci/1

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

review: Approve (continuous-integration)

[Olivier Tilloy (osomon)]

Somes typos:

146 + id: scrolllView

 should be scrollView (I’d even suggest to rename to something more explicit, e.g. "timeline", but I’ll leave it up to you

368 + id: seperator

 should be spelled "separator"

review: Needs Fixing

[Olivier Tilloy (osomon)]

44 + delegate: Item{

Replace with a Loader item (http://qt-project.org/doc/qt-5.0/qtquick/qml-qtquick2-loader.html) to avoid the whole subdelegate complexity: just set the 'source' property of the loader to either "DiaryView.qml" or "TimeLineView.qml".

review: Needs Fixing

[Olivier Tilloy (osomon)]

As a bonus for using a Loader, you won’t need to destroy the subdelegates yourself, this is automatically handled by QML, and you can replace the 'timeLineViewEnable' boolean property by a string property that’s an alias to the loader’s 'source' property.

[Olivier Tilloy (osomon)]

The TimeLineView component has a number of properties and signals in common with the DiaryView component: they should inherit from a common base component.

review: Needs Fixing

[Olivier Tilloy (osomon)]

269 + z:0

314 + z:1

 those can be removed: by default, children are stacked in the order they appear in the hierarchy

review: Needs Fixing

[Olivier Tilloy (osomon)]

If I read correctly the code of the eventLineColumn, there can be only one event per hour. And even if there’s only one (or no) event, there are 24 invisible delegates sitting there, eating up memory. Or am I misunderstanding the code?

I think this is typically a case where we want dynamic object creation and deletion, not a repeater inside a column. This will give us more flexibility, allowing to have as many (or as few) events as needed, positioned exactly where they need to be.

review: Needs Fixing

[lp:~pkunal-parmar/ubuntu-calendar-app/TimeLineView]

23. By Kunal Parmar on 2013-05-13

Review comment addressed

24. By Kunal Parmar on 2013-05-13

typo fixed

[Kunal Parmar (pkunal-parmar)]

> If I read correctly the code of the eventLineColumn, there can be only one
> event per hour. And even if there’s only one (or no) event, there are 24
> invisible delegates sitting there, eating up memory. Or am I misunderstanding
> the code?
>
> I think this is typically a case where we want dynamic object creation and
> deletion, not a repeater inside a column. This will give us more flexibility,
> allowing to have as many (or as few) events as needed, positioned exactly
> where they need to be.

I addressed almost all your concern except following one.

"The TimeLineView component has a number of properties and signals in common with the DiaryView component: they should inherit from a common base component."

This I will address in another review request.

[lp:~pkunal-parmar/ubuntu-calendar-app/TimeLineView]

25. By Kunal Parmar on 2013-05-13

unnecessary print statement removed

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

PASSED: Continuous integration, rev:25
http://91.189.93.125:8080/job/ubuntu-calendar-app-ci/3/
Executed test runs:
    SUCCESS: http://91.189.93.125:8080/job/ubuntu-calendar-app-raring-amd64-ci/3

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

review: Approve (continuous-integration)

[lp:~pkunal-parmar/ubuntu-calendar-app/TimeLineView]

26. By Kunal Parmar on 2013-05-13

minor changes

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

PASSED: Continuous integration, rev:26
http://91.189.93.125:8080/job/ubuntu-calendar-app-ci/4/
Executed test runs:
    SUCCESS: http://91.189.93.125:8080/job/ubuntu-calendar-app-raring-amd64-ci/4

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

review: Approve (continuous-integration)

[Olivier Tilloy (osomon)]

19 - onClicked: {
20 - if (expanded)
21 + onClicked: {
22 + if (expanded) {
23 compress()
24 - else
25 + }
26 + else {
27 expand()
28 + }

This change is unrelated to the functionality introduced by this MR, and it introduces trailing whitespaces. Please revert.

review: Needs Fixing

[Olivier Tilloy (osomon)]

55 + property bool timeLineViewEnable : false

You can get rid of this extraneous property, and instead expose eventViewDelegate.source as an alias at the top level of EventView. Thus, instead of flipping the value of a boolean property, you’d simply set the source component directly. That’s less code, easier to read, and more flexible in case we want to add other types of views in the future.

85 + function loadSubDelegate() {
86 + source = eventView.timeLineViewEnable ? "TimeLineView.qml" : "DiaryView.qml"
87 + }
88 +
89 + Component.onCompleted: {
90 + loadSubDelegate();
91 + }

The above becomes useless if you get rid of timeLineViewEnable.

111 + onTimeLineViewEnableChanged :{
112 + loadSubDelegate();
113 + }

Same goes for the above.

review: Needs Fixing

[Olivier Tilloy (osomon)]

115 + onExpandedChanged:{
116 + item.expanded = eventView.expanded;
117 + }

To achieve the same result in a more declarative way, use the Binding item:

  Binding {
    target: eventViewDelegate.item
    property: "expanded"
    value: eventView.expanded
  }

review: Needs Fixing

[Olivier Tilloy (osomon)]

> "The TimeLineView component has a number of properties and signals in common
> with the DiaryView component: they should inherit from a common base
> component."
>
> This I will address in another review request.

Not sure why this would benefit from being addressed in a separate branch, but OK.

[Olivier Tilloy (osomon)]

322 + function createSeperator(hour) {
323 + var sepatator = separatorComponent.createObject(bubbleOverLay);

There’s still a typo there. Actually there’s two of them now :)

  s/createSeperator/createSeparator/g
  s/sepatator/separator/g

Not that it breaks the functionality, but fixing them would make the code easier to read (and ultimately, to maintain).

review: Needs Fixing

[Olivier Tilloy (osomon)]

138 + property bool expanded: false
139 + property bool expanding: false
140 + property bool compressing: false

As a side note, the above code (and all associated animations) could be largely simplified by using QML states (see http://qt-project.org/doc/qt-5.0/qtquick/qtquick-statesanimations-states.html).
This can be addressed in a separate branch though.

[Olivier Tilloy (osomon)]

301 + function destroyAllChilds() {

should be destroyAllChildren

review: Needs Fixing

[Olivier Tilloy (osomon)]

301 + function destroyAllChilds() {
302 + for(var i=0 ; i < children.length ; ++i ) {
303 + children[i].destroy(100);
304 + }
305 + }

Is there a good reason for specifying a delay of 100 msec before each child is destroyed?

Also, this loop is incorrect, as children are destroyed their length decreases, but the iterator keeps increasing, and will eventually become invalid. You need to do something like this:

  while (children.length > 0) {
    children[0].destroy()
  }

review: Needs Fixing

[lp:~pkunal-parmar/ubuntu-calendar-app/TimeLineView]

27. By Kunal Parmar on 2013-05-17

Merge from Trunk

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

PASSED: Continuous integration, rev:27
http://91.189.93.125:8080/job/ubuntu-calendar-app-ci/6/
Executed test runs:
    SUCCESS: http://91.189.93.125:8080/job/ubuntu-calendar-app-raring-amd64-ci/6

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

review: Approve (continuous-integration)

[Kunal Parmar (pkunal-parmar)]

> 55 + property bool timeLineViewEnable : false
>
> You can get rid of this extraneous property, and instead expose
> eventViewDelegate.source as an alias at the top level of EventView. Thus,
> instead of flipping the value of a boolean property, you’d simply set the
> source component directly. That’s less code, easier to read, and more flexible
> in case we want to add other types of views in the future.
>
> 85 + function loadSubDelegate() {
> 86 + source = eventView.timeLineViewEnable ? "TimeLineView.qml" :
> "DiaryView.qml"
> 87 + }
> 88 +
> 89 + Component.onCompleted: {
> 90 + loadSubDelegate();
> 91 + }
>
> The above becomes useless if you get rid of timeLineViewEnable.
>
> 111 + onTimeLineViewEnableChanged :{
> 112 + loadSubDelegate();
> 113 + }
>
> Same goes for the above.

Some how I am not able to expose eventViewDelegate.source property under EventView scope, it gives me error "Invalid alias reference. Unable to find id "eventViewDelegate".

I instead created new property under EventView "property string eventViewType", and binded it to eventViewDelegate "source: eventView.eventViewType"

[lp:~pkunal-parmar/ubuntu-calendar-app/TimeLineView]

28. By Kunal Parmar on 2013-05-17

changes in DiaryView.qml reverted

29. By Kunal Parmar on 2013-05-17

Unnecessary property timeLineViewEnable removed

30. By Kunal Parmar on 2013-05-17

Binding element used, instead of updating expanded value manually using signal

31. By Kunal Parmar on 2013-05-17

Typo fixed

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

PASSED: Continuous integration, rev:30
http://91.189.93.125:8080/job/ubuntu-calendar-app-ci/7/
Executed test runs:
    SUCCESS: http://91.189.93.125:8080/job/ubuntu-calendar-app-raring-amd64-ci/7

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

review: Approve (continuous-integration)

[Kunal Parmar (pkunal-parmar)]

> 138 + property bool expanded: false
> 139 + property bool expanding: false
> 140 + property bool compressing: false
>
> As a side note, the above code (and all associated animations) could be
> largely simplified by using QML states (see http://qt-
> project.org/doc/qt-5.0/qtquick/qtquick-statesanimations-states.html).
> This can be addressed in a separate branch though.

I will address this, when I will work on creating common base for DiaryView and TimeLineView

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

PASSED: Continuous integration, rev:31
http://91.189.93.125:8080/job/ubuntu-calendar-app-ci/8/
Executed test runs:
    SUCCESS: http://91.189.93.125:8080/job/ubuntu-calendar-app-raring-amd64-ci/8

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

review: Approve (continuous-integration)

[Kunal Parmar (pkunal-parmar)]

> 301 + function destroyAllChilds() {
> 302 + for(var i=0 ; i < children.length ; ++i ) {
> 303 + children[i].destroy(100);
> 304 + }
> 305 + }
>
> Is there a good reason for specifying a delay of 100 msec before each child is
> destroyed?

no, i guess i was just trying to understand how it works with time and without it
>
> Also, this loop is incorrect, as children are destroyed their length
> decreases, but the iterator keeps increasing, and will eventually become
> invalid. You need to do something like this:
>
> while (children.length > 0) {
> children[0].destroy()
> }

   while (children.length > 0) {
     children[0].destroy()
   }

This will create a infinite loop, as we are not removing object from children array/list. we are just destroying object so length of array will not change. so existing loop works fine, but then array length will keep increasing so to reset length i am now resetting array with following statement.

children = [];

[lp:~pkunal-parmar/ubuntu-calendar-app/TimeLineView]

32. By Kunal Parmar on 2013-05-17

destroyAllChilds ->destroyAllChildren and resetting children array

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

PASSED: Continuous integration, rev:32
http://91.189.93.125:8080/job/ubuntu-calendar-app-ci/9/
Executed test runs:
    SUCCESS: http://91.189.93.125:8080/job/ubuntu-calendar-app-raring-amd64-ci/9

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

review: Approve (continuous-integration)

[Olivier Tilloy (osomon)]

> > Also, this loop is incorrect, as children are destroyed their length
> > decreases, but the iterator keeps increasing, and will eventually become
> > invalid. You need to do something like this:
> >
> > while (children.length > 0) {
> > children[0].destroy()
> > }
>
> while (children.length > 0) {
> children[0].destroy()
> }
>
> This will create a infinite loop, as we are not removing object from children
> array/list. we are just destroying object so length of array will not change.
> so existing loop works fine, but then array length will keep increasing so to
> reset length i am now resetting array with following statement.
>
> children = [];

Good point, I should have tested my suggestion first.
However, I don’t think you should reset the value of children to [], that’s likely to break automatic memory management, since we don’t have a guarantee of when the children are actually removed from the list. I think the following would work (destroying the children in reverse order, thus ensuring the iterator never becomes invalid):

    function destroyAllChildren() {
        for(var i = children.length; i >= 0; --i) {
            children[i].destroy();
        }
    }

review: Needs Fixing

[Olivier Tilloy (osomon)]

> Some how I am not able to expose eventViewDelegate.source property under
> EventView scope, it gives me error "Invalid alias reference. Unable to find id
> "eventViewDelegate".
>
> I instead created new property under EventView "property string
> eventViewType", and binded it to eventViewDelegate "source:
> eventView.eventViewType"

Your solution works. For reference, what I was suggesting would look like this:

[Olivier Tilloy (osomon)]

Scrap the above comment (which by the way was posted incomplete), my suggestion could not work because the Loader is a delegate, so it can be instantiated several times. Your solution works nicely.

[Olivier Tilloy (osomon)]

34 + property bool timeLineViewEnable : false

this property is unused now, it should be removed

review: Needs Fixing

[Olivier Tilloy (osomon)]

75 + onDayStartChanged: {
76 + if (status == Loader.Ready) {
77 + item.dayStart = dayStart;
78 + }
79 + }

To make this more declarative, you can replace the above block (and remove the assignment of item.dayStart in onStatusChanged) by:

    Binding {
        target: item
        property: "dayStart"
        value: eventViewDelegate.dayStart
    }

(no need to check the status, as item will be null if the status is ≠ Loader.Ready).

review: Needs Fixing

[Olivier Tilloy (osomon)]

> > > Also, this loop is incorrect, as children are destroyed their length
> > > decreases, but the iterator keeps increasing, and will eventually become
> > > invalid. You need to do something like this:
> > >
> > > while (children.length > 0) {
> > > children[0].destroy()
> > > }
> >
> > while (children.length > 0) {
> > children[0].destroy()
> > }
> >
> > This will create a infinite loop, as we are not removing object from
> children
> > array/list. we are just destroying object so length of array will not
> change.
> > so existing loop works fine, but then array length will keep increasing so
> to
> > reset length i am now resetting array with following statement.
> >
> > children = [];
>
> Good point, I should have tested my suggestion first.
> However, I don’t think you should reset the value of children to [], that’s
> likely to break automatic memory management, since we don’t have a guarantee
> of when the children are actually removed from the list. I think the following
> would work (destroying the children in reverse order, thus ensuring the
> iterator never becomes invalid):
>
> function destroyAllChildren() {
> for(var i = children.length; i >= 0; --i) {
> children[i].destroy();
> }
> }

Hmmm, that’s embarassing, I’m struggling with indexes today… Make that:

    function destroyAllChildren() {
        for(var i = children.length - 1; i >= 0; --i) {
            children[i].destroy();
        }
    }

[lp:~pkunal-parmar/ubuntu-calendar-app/TimeLineView]

33. By Kunal Parmar on 2013-05-18

redundant property timeLineViewEnable removed

34. By Kunal Parmar on 2013-05-18

Binding used instead of manual assignment

35. By Kunal Parmar on 2013-05-18

destroyAllChildren loop reversed

[Kunal Parmar (pkunal-parmar)]

addressed the latest comments

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

PASSED: Continuous integration, rev:35
http://91.189.93.125:8080/job/ubuntu-calendar-app-ci/10/
Executed test runs:
    SUCCESS: http://91.189.93.125:8080/job/ubuntu-calendar-app-raring-amd64-ci/10

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

review: Approve (continuous-integration)

[Olivier Tilloy (osomon)]

232 + // FIXME: how to represent
233 + text: index+":00"

This should be localized, probably something like this (not sure it would work for all possible regional settings, but should be better than nothing):

    text: i18n.tr("%1:00").arg(index)

What exactly was the FIXME about?

review: Needs Fixing

[Olivier Tilloy (osomon)]

208 + Rectangle{
209 + id: background; anchors.fill: parent
210 + }

There’s no color set for this background rectangle, is it actually useful?

[Olivier Tilloy (osomon)]

239 + height:units.gu(0.1)

Here (and in other places where using units.gu(0.1), you probably want to use units.dp(1) instead: this will basically ensure the line is one pixel high.

review: Needs Fixing

[David Planella (dpm)]

Al 20/05/13 12:12, En/na Olivier Tilloy ha escrit:
> Review: Needs Fixing
>
> 232 + // FIXME: how to represent
> 233 + text: index+":00"
>
> This should be localized, probably something like this (not sure it would work for all possible regional settings, but should be better than nothing):
>

Numbers, as in the "00" after the colon, can automatically be localized
too. See the numberToLocaleString() function the Clock team uses:

https://code.launchpad.net/~nik90/ubuntu-clock-app/replace-number-translations/+merge/164582

[Kunal Parmar (pkunal-parmar)]

> 208 + Rectangle{
> 209 + id: background; anchors.fill: parent
> 210 + }
>
> There’s no color set for this background rectangle, is it actually useful?

This sets white background, as Rectangle's default color is White, I did not mentioned it explicitly. But I will change it to be more clear.

[lp:~pkunal-parmar/ubuntu-calendar-app/TimeLineView]

36. By Kunal Parmar on 2013-05-22

units.gu(0.1) -> units.dp(1)

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

PASSED: Continuous integration, rev:36
http://91.189.93.125:8080/job/ubuntu-calendar-app-ci/11/
Executed test runs:
    SUCCESS: http://91.189.93.125:8080/job/ubuntu-calendar-app-raring-amd64-ci/11

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

review: Approve (continuous-integration)

[Kunal Parmar (pkunal-parmar)]

> 232 + // FIXME: how to represent
> 233 + text: index+":00"
>
> This should be localized, probably something like this (not sure it would work
> for all possible regional settings, but should be better than nothing):
>
> text: i18n.tr("%1:00").arg(index)
>
> What exactly was the FIXME about?

As David suggested, I tried to use code from Clock source code. I am using following code now. Does it looks ok ?

My implementation is only hourToLocaleString() this function. Other are copied from Clock.

    function numberToLocaleString(num) {
        return Number(num).toLocaleString(Qt.locale(), "f", 0)
    }

    function zeroleft ( number, zeroCount ) {
        var resstring = numberToLocaleString(number.toString());
        var numchart = zeroCount;
        var numberlength = resstring.length;
        for (var i = numberlength; i < numchart; i++) {
            resstring = numberToLocaleString("0") + resstring;
        }
        return resstring;
    }

    function hourToLocaleString(hour) {
        return zeroleft(hour,2)+":"+ zeroleft(0,2);
    }

[lp:~pkunal-parmar/ubuntu-calendar-app/TimeLineView]

37. By Kunal Parmar on 2013-05-22

Hour label localization issue addressed

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

PASSED: Continuous integration, rev:37
http://91.189.93.125:8080/job/ubuntu-calendar-app-ci/12/
Executed test runs:
    SUCCESS: http://91.189.93.125:8080/job/ubuntu-calendar-app-raring-amd64-ci/12

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

review: Approve (continuous-integration)

[Olivier Tilloy (osomon)]

> > 208 + Rectangle{
> > 209 + id: background; anchors.fill: parent
> > 210 + }
> >
> > There’s no color set for this background rectangle, is it actually useful?
>
> This sets white background, as Rectangle's default color is White, I did not
> mentioned it explicitly. But I will change it to be more clear.

Please do, to set it to "white" explicitly. The fact that it is white by default might change in the future.

[Olivier Tilloy (osomon)]

> As David suggested, I tried to use code from Clock source code. I am using
> following code now. Does it looks ok ?
>
> My implementation is only hourToLocaleString() this function. Other are copied
> from Clock.
>
> function numberToLocaleString(num) {
> return Number(num).toLocaleString(Qt.locale(), "f", 0)
> }
>
> function zeroleft ( number, zeroCount ) {
> var resstring = numberToLocaleString(number.toString());
> var numchart = zeroCount;
> var numberlength = resstring.length;
> for (var i = numberlength; i < numchart; i++) {
> resstring = numberToLocaleString("0") + resstring;
> }
> return resstring;
> }
>
> function hourToLocaleString(hour) {
> return zeroleft(hour,2)+":"+ zeroleft(0,2);
> }

If that is the way the Clock app does it, then it’s wrong. The zeroleft function is not i18n friendly, and there’s a much shorter solution:

  Label {
    id: timeLabel
    text: new Date(0, 0, 0, index).toLocaleTimeString(Qt.locale(), "HH:mm")
  }

review: Needs Fixing

[David Planella (dpm)]

Ah, I hadn't picked up on the fact that the zeroleft function was used, as that part was not internationalized in the Clock app. I was pointing at the other parts in which they internationalized numbers.

Olivier's solution looks most sensible to me. My experience on i18n on code is mostly on Python or C apps based on gettext and GTK, so I'm still figuring out what the equivalents in the QML/JavaScript world are.

[Olivier Tilloy (osomon)]

> > […]
>
> If that is the way the Clock app does it, then it’s wrong. The zeroleft
> function is not i18n friendly, and there’s a much shorter solution:
>
> Label {
> id: timeLabel
> text: new Date(0, 0, 0, index).toLocaleTimeString(Qt.locale(), "HH:mm")
> }

And even better, the format string should be localized, with a comment for translators:

  Label {
    id: timeLabel
    // TRANSLATORS: this is a time formatting string, see http://qt-project.org/doc/qt-5.0/qtqml/qml-qtquick2-date.html#details for valid expressions
    text: new Date(0, 0, 0, index).toLocaleTimeString(Qt.locale(), i18n.tr("HH:mm"))
  }

[lp:~pkunal-parmar/ubuntu-calendar-app/TimeLineView]

38. By Kunal Parmar on 2013-05-23

Localization issue fixed for hour label

39. By Kunal Parmar on 2013-05-23

Merge from trunck code

[Kunal Parmar (pkunal-parmar)]

> > > […]
> >
> > If that is the way the Clock app does it, then it’s wrong. The zeroleft
> > function is not i18n friendly, and there’s a much shorter solution:
> >
> > Label {
> > id: timeLabel
> > text: new Date(0, 0, 0, index).toLocaleTimeString(Qt.locale(), "HH:mm")
> > }
>
> And even better, the format string should be localized, with a comment for
> translators:
>
> Label {
> id: timeLabel
> // TRANSLATORS: this is a time formatting string, see http://qt-
> project.org/doc/qt-5.0/qtqml/qml-qtquick2-date.html#details for valid
> expressions
> text: new Date(0, 0, 0, index).toLocaleTimeString(Qt.locale(),
> i18n.tr("HH:mm"))
> }

Ok, I used above code as it is. my experience in localization is almost zero, so thanks for helping me :)

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

PASSED: Continuous integration, rev:39
http://91.189.93.125:8080/job/ubuntu-calendar-app-ci/13/
Executed test runs:
    SUCCESS: http://91.189.93.125:8080/job/ubuntu-calendar-app-raring-amd64-ci/13

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

review: Approve (continuous-integration)

[Olivier Tilloy (osomon)]

Looks good. Two last details:

 - You haven’t addressed my comment about setting the background color to "white" explicitly.

 - Since there are new strings, you need to regenerate the translation template. To do that, just invoke the following command in your branch, and commit the modified .pot file:

    xgettext --from-code=UTF-8 -o po/ubuntu-calendar-app.pot -D . --c++ --qt --add-comments=TRANSLATORS --keyword=tr --package-name=ubuntu-calendar-app -s *.qml

review: Needs Fixing

[lp:~pkunal-parmar/ubuntu-calendar-app/TimeLineView]

40. By Kunal Parmar on 2013-05-23

setting white as background color

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

PASSED: Continuous integration, rev:40
http://91.189.93.125:8080/job/ubuntu-calendar-app-ci/14/
Executed test runs:
    SUCCESS: http://91.189.93.125:8080/job/ubuntu-calendar-app-raring-amd64-ci/14

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

review: Approve (continuous-integration)

[lp:~pkunal-parmar/ubuntu-calendar-app/TimeLineView]

41. By Kunal Parmar on 2013-05-23

regenerating the translation template

[Kunal Parmar (pkunal-parmar)]

> Looks good. Two last details:
>
> - You haven’t addressed my comment about setting the background color to
> "white" explicitly.
>
> - Since there are new strings, you need to regenerate the translation
> template. To do that, just invoke the following command in your branch, and
> commit the modified .pot file:
>
> xgettext --from-code=UTF-8 -o po/ubuntu-calendar-app.pot -D . --c++ --qt
> --add-comments=TRANSLATORS --keyword=tr --package-name=ubuntu-calendar-app -s
> *.qml

ahh, I thought I made changes to set background, but then forgotten about it. Did it now. and also updated the pot file.

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

PASSED: Continuous integration, rev:41
http://91.189.93.125:8080/job/ubuntu-calendar-app-ci/15/
Executed test runs:
    SUCCESS: http://91.189.93.125:8080/job/ubuntu-calendar-app-raring-amd64-ci/15

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

review: Approve (continuous-integration)

[Olivier Tilloy (osomon)]

Looks good now. Let’s merge!

review: Approve

=== removed file '.bzrignore.moved'
--- .bzrignore.moved	2013-04-11 19:47:11 +0000
+++ .bzrignore.moved	1970-01-01 00:00:00 +0000
@@ -1,6 +0,0 @@
-calendar.qmlproject.user
-
-debian/files
-debian/ubuntu-calendar-app/
-debian/*.debhelper.log
-debian/*.substvars

=== modified file 'DiaryViewDelegate.qml'
--- DiaryViewDelegate.qml	2013-04-13 04:43:04 +0000
+++ DiaryViewDelegate.qml	2013-05-23 14:11:30 +0000
@@ -38,7 +38,9 @@
         var now = new Date;
         var lastEvent = eventModel.get(index-1);
 
-        if( endTime >= now && (lastEvent === undefined || lastEvent.endTime < now )  ) {
+        if( endTime >= now
+                && (lastEvent === undefined || lastEvent.endTime < now )
+                && endTime.isSameDay(now) ) {
             collapse(false);
             bgColor = "#fffdaa";
             seperator.visible = true;

=== modified file 'EventView.qml'
--- EventView.qml	2013-03-29 23:12:40 +0000
+++ EventView.qml	2013-05-23 14:11:30 +0000
@@ -6,6 +6,7 @@
     id: eventView
 
     property var currentDayStart: (new Date()).midnight()
+    property string eventViewType: "DiaryView.qml";
 
     signal incrementCurrentDay
     signal decrementCurrentDay
@@ -58,23 +59,36 @@
 
     model: 3
 
-    delegate: DiaryView {
-        id: diaryView
+    delegate: Loader {
+        id: eventViewDelegate
 
         width: eventView.width
         height: eventView.height
+        source: eventView.eventViewType
 
-        dayStart: {
+        property var dayStart: {
             if (index == intern.currentIndex) return intern.currentDayStart
             var previousIndex = intern.currentIndex > 0 ? intern.currentIndex - 1 : 2
-            if (index == previousIndex) return intern.currentDayStart.addDays(-1)
+            if (index === previousIndex) return intern.currentDayStart.addDays(-1)
             return intern.currentDayStart.addDays(1)
         }
 
-        expanded: eventView.expanded
-
-        onExpand: eventView.expand()
-        onCompress: eventView.compress()
-        onNewEvent: eventView.newEvent()
+        onLoaded: {
+            item.expand.connect(eventView.expand);
+            item.compress.connect(eventView.compress);
+            item.newEvent.connect(eventView.newEvent);
+        }
+
+        Binding {
+            target: item
+            property: "dayStart"
+            value: eventViewDelegate.dayStart
+        }
+
+        Binding {
+            target: item
+            property: "expanded"
+            value: eventView.expanded
+        }
     }
 }

=== added file 'TimeLineView.qml'
--- TimeLineView.qml	1970-01-01 00:00:00 +0000
+++ TimeLineView.qml	2013-05-23 14:11:30 +0000
@@ -0,0 +1,259 @@
+import QtQuick 2.0
+import Ubuntu.Components 0.1
+
+import "dateExt.js" as DateExt
+import "dataService.js" as DataService
+
+
+Flickable{
+    id: timeLineView
+
+    property var dayStart : new Date();
+
+    property bool expanded: false
+    property bool expanding: false
+    property bool compressing: false
+
+    signal expand()
+    signal compress()
+    signal newEvent()
+
+    function scroll() {
+        //scroll to first event or current hour
+        var hour = intern.now.getHours();
+        if(eventListModel.count > 0) {
+            hour = eventListModel.get(0).startTime.getHours();
+        }
+
+        timeLineView.contentY = hour * units.gu(10);
+
+        if(timeLineView.contentY >= timeLineView.contentHeight - timeLineView.height) {
+            timeLineView.contentY = timeLineView.contentHeight - timeLineView.height
+        }
+    }
+
+    function createEventMap() {
+        var eventMap = {};
+        for(var i = 0 ; i < eventListModel.count ; ++i) {
+            var event = eventListModel.get(i);
+            eventMap[event.startTime.getHours()] = event
+        }
+        return eventMap;
+    }
+
+    function createEvents() {
+        intern.eventMap = createEventMap();
+
+        bubbleOverLay.destroyAllChildren();
+
+        for( var i=0; i < 24; ++i ) {
+            var event = intern.eventMap[i];
+            if( event ) {
+                bubbleOverLay.createEvent(event,i);
+            } else if( i === intern.now.getHours()
+                      && intern.now.isSameDay( timeLineView.dayStart )) {
+                bubbleOverLay.createSeparator(i);
+            }
+        }
+
+        scroll();
+    }
+
+    function showEventDetails(hour) {
+        var event = intern.eventMap[hour];
+        pageStack.push(Qt.resolvedUrl("EventDetails.qml"),{"event":event});
+    }
+
+    onContentYChanged: {
+        // console.log(expanded, expanding, compressing, dragging, flicking, moving, contentY)
+        if (expanding || compressing || !dragging) return
+
+        if (expanded) {
+            if (contentY < -units.gu(0.5)) {
+                compressing = true
+                expanding = false
+            }
+        }
+        else {
+            if (contentY < -units.gu(0.5)) {
+                expanding = true
+                compressing = false
+            }
+        }
+    }
+
+    onDraggingChanged: {
+        if (dragging) return
+
+        if (expanding) {
+            expanding = false
+            expand()
+        }
+        else if (compressing) {
+            compressing = false
+            compress()
+        }
+    }
+
+    clip: true
+
+    contentHeight: timeLineColumn.height + units.gu(3)
+    contentWidth: width
+
+    QtObject {
+        id: intern
+        property var eventMap;
+        property var now : new Date();
+        property var hourHeight : units.gu(10)
+    }
+
+    EventListModel {
+        id: eventListModel
+        termStart: timeLineView.dayStart
+        termLength: Date.msPerDay
+
+        onReload: {
+            createEvents();
+        }
+    }
+
+    Rectangle{
+        id: background; anchors.fill: parent
+        color: "white"
+    }
+
+    //Time line view
+    Column{
+        id: timeLineColumn
+        anchors.top: parent.top
+        anchors.topMargin: units.gu(3)
+        width: parent.width        
+
+        Repeater{
+            model: 24 // hour in a day
+
+            delegate: Item {
+                id: delegate
+                width: parent.width
+                height: intern.hourHeight
+
+                Row {
+                    width: parent.width
+                    y: -timeLabel.height/2
+                    Label{
+                        id: timeLabel
+                        // TRANSLATORS: this is a time formatting string,
+                        // see http://qt-project.org/doc/qt-5.0/qtqml/qml-qtquick2-date.html#details for valid expressions
+                        text: new Date(0, 0, 0, index).toLocaleTimeString(Qt.locale(), i18n.tr("HH:mm"))
+                        color:"gray"
+                        anchors.top: parent.top
+                    }
+                    Rectangle{
+                        width: parent.width -timeLabel.width
+                        height:units.dp(1)
+                        color:"gray"
+                        anchors.verticalCenter: parent.verticalCenter
+                    }
+                }
+
+                Rectangle{
+                    width: parent.width - units.gu(5)
+                    height:units.dp(1)
+                    color:"gray"
+                    anchors.verticalCenter: parent.verticalCenter
+                    anchors.horizontalCenter: parent.horizontalCenter
+                }
+            }
+        }
+    }
+
+    Item {
+        id: bubbleOverLay
+
+        width: timeLineColumn.width
+        height: timeLineColumn.height
+        anchors.top: parent.top
+        anchors.topMargin: units.gu(3)
+
+        function destroyAllChildren() {
+            for( var i = children.length - 1; i >= 0; --i ) {
+                children[i].destroy();
+            }
+        }
+
+        function createEvent( event ,hour) {
+            var eventBubble = infoBubbleComponent.createObject(bubbleOverLay);
+            eventBubble.title = event.title;
+            eventBubble.location = "test";
+            eventBubble.hour = hour;
+
+            var yPos = (( event.startTime.getMinutes() * intern.hourHeight) / 60) + hour * intern.hourHeight
+            eventBubble.y = yPos;
+
+            var durationMin = (event.endTime.getHours() - event.startTime.getHours()) * 60;
+            durationMin += (event.endTime.getMinutes() - event.startTime.getMinutes());
+            var height = (durationMin * intern.hourHeight )/ 60;
+            eventBubble.height = height;
+        }
+
+        function createSeparator(hour) {
+            var separator = separatorComponent.createObject(bubbleOverLay);
+            var yPos = ((intern.now.getMinutes() * intern.hourHeight) / 60) + hour * intern.hourHeight
+            separator.visible = true;
+            separator.y = yPos;
+            separator.x = (parent.width - separator.width)/2
+        }
+    }
+
+    Component{
+        id: infoBubbleComponent
+        Rectangle{
+            id: infoBubble
+
+            property string title;
+            property string location;
+            property int hour;
+
+            color:'#fffdaa';
+            width: timeLineView.width - units.gu(8)
+            x: units.gu(5)
+
+            border.color: "#f4d690"
+
+            Column{
+                id: column
+                anchors {
+                    left: parent.left
+                    right: parent.right
+                    top: parent.top
+
+                    leftMargin: units.gu(1)
+                    rightMargin: units.gu(1)
+                    topMargin: units.gu(1)
+                }
+                spacing: units.gu(1)
+                Label{text:infoBubble.title;fontSize:"medium";color:"black"}
+                Label{text:infoBubble.location; fontSize:"small"; color:"black"}
+            }
+
+            MouseArea{
+                anchors.fill: parent
+                onClicked: {
+                    timeLineView.showEventDetails(hour);
+                }
+            }
+        }
+    }
+
+    Component {
+        id: separatorComponent
+        Rectangle {
+            id: separator
+            height: units.gu(0.5)
+            width: timeLineView.width - units.gu(2)
+            color: "#c94212"
+            visible: false
+        }
+    }
+}
+

=== modified file 'calendar.qml'
--- calendar.qml	2013-05-22 16:00:01 +0000
+++ calendar.qml	2013-05-23 14:11:30 +0000
@@ -39,7 +39,15 @@
                 Action {
                     iconSource: Qt.resolvedUrl("avatar.png")
                     text: i18n.tr("Timeline")
-                    onTriggered:; // FIXME
+                    onTriggered: {
+                        if( eventView.eventViewType  === "DiaryView.qml") {
+                            eventView.eventViewType = "TimeLineView.qml";
+                            text = i18n.tr("Diary")
+                        } else {
+                            eventView.eventViewType = "DiaryView.qml";
+                            text = i18n.tr("Timeline")
+                        }
+                    }
                 }
             }
 

=== modified file 'dateExt.js'
--- dateExt.js	2013-03-19 18:00:35 +0000
+++ dateExt.js	2013-05-23 14:11:30 +0000
@@ -79,3 +79,9 @@
 Date.prototype.weeksInMonth = function(weekday) {
     return Date.weeksInMonth(this.getFullYear(), this.getMonth(), weekday)
 }
+
+Date.prototype.isSameDay = function ( otherDay ) {
+    return ( this.getDate() === otherDay.getDate()
+    && this.getMonth() === otherDay.getMonth()
+    && this.getFullYear() === otherDay.getFullYear() );
+}

=== modified file 'po/ubuntu-calendar-app.pot'
--- po/ubuntu-calendar-app.pot	2013-05-15 09:23:44 +0000
+++ po/ubuntu-calendar-app.pot	2013-05-23 14:11:30 +0000
@@ -1,14 +1,14 @@
-# Ubuntu Calendar app translation.
-# Copyright (C) Canonical Ltd. 2013.
-# This file is distributed under the same license as the ubuntu-calendar-app package.
+# SOME DESCRIPTIVE TITLE.
+# Copyright (C) YEAR THE PACKAGE'S COPYRIGHT HOLDER
+# This file is distributed under the same license as the PACKAGE package.
 # FIRST AUTHOR <EMAIL@ADDRESS>, YEAR.
 #
 #, fuzzy
 msgid ""
 msgstr ""
-"Project-Id-Version: PACKAGE VERSION\n"
+"Project-Id-Version: ubuntu-calendar-app\n"
 "Report-Msgid-Bugs-To: \n"
-"POT-Creation-Date: 2013-05-15 10:19+0200\n"
+"POT-Creation-Date: 2013-05-23 23:02+0900\n"
 "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
 "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
 "Language-Team: LANGUAGE <[e-mail]>\n"
@@ -17,55 +17,64 @@
 "Content-Type: text/plain; charset=CHARSET\n"
 "Content-Transfer-Encoding: 8bit\n"
 
-#: /home/dpm/dev/coreapps/ubuntu-calendar-app/calendar.qml:29
-msgid "To-do"
-msgstr ""
-
-#: /home/dpm/dev/coreapps/ubuntu-calendar-app/calendar.qml:34
-msgid "New Event"
-msgstr ""
-
-#: /home/dpm/dev/coreapps/ubuntu-calendar-app/calendar.qml:39
-msgid "Timeline"
-msgstr ""
-
-#: /home/dpm/dev/coreapps/ubuntu-calendar-app/DiaryView.qml:54
+#: DiaryView.qml:54
 msgid "(+) New Event"
 msgstr ""
 
-#: /home/dpm/dev/coreapps/ubuntu-calendar-app/EventDetails.qml:69
+#: NewEvent.qml:104
+msgid "Add Location"
+msgstr ""
+
+#: NewEvent.qml:25
+msgid "Add event name"
+msgstr ""
+
+#: EventDetails.qml:69
 msgid "Add invite"
 msgstr ""
 
-#: /home/dpm/dev/coreapps/ubuntu-calendar-app/EventDetails.qml:75
+#: NewEvent.qml:20
+msgid "Create event"
+msgstr ""
+
+#: calendar.qml:45
+msgid "Diary"
+msgstr ""
+
+#: EventDetails.qml:75
 msgid "Edit"
 msgstr ""
 
-#: /home/dpm/dev/coreapps/ubuntu-calendar-app/EventDetails.qml:150
-#: /home/dpm/dev/coreapps/ubuntu-calendar-app/NewEvent.qml:112
+#. TRANSLATORS: this is a time formatting string,
+#. see http://qt-project.org/doc/qt-5.0/qtqml/qml-qtquick2-date.html#details for valid expressions
+#: TimeLineView.qml:147
+msgid "HH:mm"
+msgstr ""
+
+#: NewEvent.qml:117
+msgid "Invite People"
+msgstr ""
+
+#: NewEvent.qml:99
+msgid "Location"
+msgstr ""
+
+#: calendar.qml:36
+msgid "New Event"
+msgstr ""
+
+#: EventDetails.qml:150 NewEvent.qml:112
 msgid "People"
 msgstr ""
 
-#: /home/dpm/dev/coreapps/ubuntu-calendar-app/NewEvent.qml:20
-msgid "Create event"
-msgstr ""
-
-#: /home/dpm/dev/coreapps/ubuntu-calendar-app/NewEvent.qml:25
-msgid "Add event name"
-msgstr ""
-
-#: /home/dpm/dev/coreapps/ubuntu-calendar-app/NewEvent.qml:99
-msgid "Location"
-msgstr ""
-
-#: /home/dpm/dev/coreapps/ubuntu-calendar-app/NewEvent.qml:104
-msgid "Add Location"
-msgstr ""
-
-#: /home/dpm/dev/coreapps/ubuntu-calendar-app/NewEvent.qml:117
-msgid "Invite People"
-msgstr ""
-
-#: /home/dpm/dev/coreapps/ubuntu-calendar-app/NewEvent.qml:128
+#: NewEvent.qml:128
 msgid "Save"
 msgstr ""
+
+#: calendar.qml:41 calendar.qml:48
+msgid "Timeline"
+msgstr ""
+
+#: calendar.qml:31
+msgid "To-do"
+msgstr ""