Indicator Date and Time

Merge lp://qastaging/~charlesk/indicator-datetime/lp-1204532 into lp://qastaging/~indicator-applet-developers/indicator-datetime/trunk.13.10

lp-1204532
Merge into trunk.13.10

Proposed by Charles Kerr on 2013-09-05

Status:

Merged

Approved by:

Ted Gould on 2013-09-06

Approved revision:

257

Merged at revision:

250

Proposed branch:

lp://qastaging/~charlesk/indicator-datetime/lp-1204532

Merge into:

lp://qastaging/~indicator-applet-developers/indicator-datetime/trunk.13.10

Diff against target:

1014 lines (+477/-176)

4 files modified

src/planner-eds.c (+261/-80)
src/planner.c (+29/-2)
src/planner.h (+33/-7)
src/service.c (+154/-87)

To merge this branch:

bzr merge lp://qastaging/~charlesk/indicator-datetime/lp-1204532

Related bugs:

Bug #1197645: indicator-datetime-service crashed with SIGSEGV in cal_client_emit_backend_property_changed_idle_cb()	Medium	Fix Released
Bug #1204532: indicator-service-datetime hangs on startup when loading a problematic calendar	High	Fix Released
Bug #1210864: evolution-calendar-factory severely leaking memory	High	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Ted Gould (community)		2013-09-05	Approve on 2013-09-06
PS Jenkins bot (community)	continuous-integration		Approve on 2013-09-06
Review via email: mp+184155@code.qastaging.launchpad.net

Commit message

Make the EDS planner nonblocking.

Description of the change

Make the EDS planner nonblocking.

Summary of changes:

1. Instead of synchronously creating short-term EDS sources & clients in indicator_datetime_planner_get_appointments(), load them asynchronously when the planner object is instantiated and store references to them. When a connection changes or is added or removed, notify the datetime service via planner's "appointments-changed" signal.

2. Make indicator_datetime_planner_get_appointments() nonblocking.

3. In the service, respond to the "appointments-changed" signal by asynchronously fetching appointments, then updating the appropriate menu sections (ie, calendar & appointments).

This branch should make headway on three bug tickets:

1. bug #1204532, which is straightforward: when EDS is misconfigured, datetime's sync calls to it can block, hanging datetime

2. bug #1210864, which is exacerbated by indicator-datetime frequently cycling through EDS sources & clients. This patch removes that churn.

3. bug #1207060, which is currently the top bug at <https://errors.ubuntu.com/?release=Ubuntu%2013.10&user=indicator-applet-developers&period=month>. That ticket's trace is vague; however, it looks like a dangling pointer issue in our EDS code. Eliminating the EDS churn mentioned above may solve this. (Note, this is a wishy-washy assessment. I'd feel more confident on this ticket if I could reproduce the crash or if the stacktrace were more informative.)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-09-05:

PASSED: Continuous integration, rev:251
http://jenkins.qa.ubuntu.com/job/indicator-datetime-ci/88/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-datetime-saucy-amd64-ci/42
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-datetime-saucy-armhf-ci/43
        deb: http://jenkins.qa.ubuntu.com/job/indicator-datetime-saucy-armhf-ci/43/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/indicator-datetime-ci/88/rebuild

review: Approve (continuous-integration)

lp://qastaging/~charlesk/indicator-datetime/lp-1204532 updated on 2013-09-05

252. By Charles Kerr on 2013-09-05: use GSlice for allocating/freeing indicator appt structs
253. By Charles Kerr on 2013-09-05: use GSlice for allocating/freeing service's setlocation_data structs
254. By Charles Kerr on 2013-09-05: use GSlice for allocating/freeing service's TimeLocation structs

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-09-05:

PASSED: Continuous integration, rev:254
http://jenkins.qa.ubuntu.com/job/indicator-datetime-ci/89/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-datetime-saucy-amd64-ci/43
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-datetime-saucy-armhf-ci/44
        deb: http://jenkins.qa.ubuntu.com/job/indicator-datetime-saucy-armhf-ci/44/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/indicator-datetime-ci/89/rebuild

review: Approve (continuous-integration)

Revision history for this message

Ted Gould (ted) wrote on 2013-09-05:

A could little comments from the code review. I'm going to run this and look at it as well. Complex, but good!

* For decrementing the subtask_count you should probably use g_atomic_int_dec_and_test().

* In on_appointments_change() you should probably handle the case of a GDateTime being NULL. I think that the indicator functions are fine, but the g_datetime_unref() is not.

Revision history for this message

Ted Gould (ted) wrote on 2013-09-05:

Installed on my machine it is much faster to get the service up and time displayed, which is awesome. It doesn't seem like clicking on days in the calendar work though. It looks like the menu refreshes, but the 5 appointments stay the same.

review: Needs Fixing

Revision history for this message

Charles Kerr (charlesk) wrote on 2013-09-05:

> * For decrementing the subtask_count you should probably use g_atomic_int_dec_and_test().

Fixed r255

> * In on_appointments_change() you should probably handle the case of a GDateTime being NULL. I think that the indicator functions are fine, but the g_datetime_unref() is not.

get_calendar_date() doesn't return NULL, and the arguments to g_date_time_new_local() all look sane. I don't see the NULL case you're wanting to safeguard against?

> the menu refreshes, but the 5 appointments stay the same.

Oog. in progress.

lp://qastaging/~charlesk/indicator-datetime/lp-1204532 updated on 2013-09-05

255. By Charles Kerr on 2013-09-05: in planner-eds, use g_atomic_int for enc/dec of the subtask_count

Revision history for this message

Ted Gould (ted) wrote on 2013-09-05:

On Thu, 2013-09-05 at 21:35 +0000, Charles Kerr wrote:

> > * In on_appointments_change() you should probably handle the case of a GDateTime being NULL. I think that the indicator functions are fine, but the g_datetime_unref() is not.
>
> get_calendar_date() doesn't return NULL, and the arguments to g_date_time_new_local() all look sane. I don't see the NULL case you're wanting to safeguard against?

Yeah, I don't know. It was happening in one of the other bugs I fixed.
Not sure if it's bad system time or what. What's weirder is that it was
the begin time... no clue. Be safe out there :-)

Revision history for this message

Marco Trevisan (Treviño) (3v1n0) wrote on 2013-09-05:

FYI, I can confirm that this fixes #1204532

Revision history for this message

Marco Trevisan (Treviño) (3v1n0) wrote on 2013-09-05:

Mh, I don't see the calendar widget in it though...

Revision history for this message

Charles Kerr (charlesk) wrote on 2013-09-05:

Ted, I can confirm your report of the appointments section not updating properly when the calendar date changes.

Trevino, just to confirm, you're looking at the desktop? There's no calendar widget in datetime indicator's phone menu. I'm seeing it on the desktop though.

lp://qastaging/~charlesk/indicator-datetime/lp-1204532 updated on 2013-09-05

256. By Charles Kerr on 2013-09-05: fix 'upcoming appointment' section menuitems issue reported by ted
257. By Charles Kerr on 2013-09-05: in update_appointment_lists(), safeguard against NULL GDateTimes.

Revision history for this message

Charles Kerr (charlesk) wrote on 2013-09-05:

> the menu refreshes, but the 5 appointments stay the same.

Fixed r256

> * In on_appointments_change() you should probably handle the case of a GDateTime being NULL. I think that the indicator functions are fine, but the g_datetime_unref() is not.

Fixed r257

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-09-05:

FAILED: Continuous integration, rev:254
http://jenkins.qa.ubuntu.com/job/indicator-datetime-ci/90/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-datetime-saucy-amd64-ci/44
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-datetime-saucy-armhf-ci/45/console
        deb: http://jenkins.qa.ubuntu.com/job/indicator-datetime-saucy-armhf-ci/45/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/indicator-datetime-ci/90/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-09-05:

FAILED: Continuous integration, rev:254
http://jenkins.qa.ubuntu.com/job/indicator-datetime-ci/91/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-datetime-saucy-amd64-ci/45
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-datetime-saucy-armhf-ci/46/console
        deb: http://jenkins.qa.ubuntu.com/job/indicator-datetime-saucy-armhf-ci/46/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/indicator-datetime-ci/91/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Ted Gould (ted) wrote on 2013-09-06:

Odd to me Jenkins kept reporting 254 as the revision. Running again at 257.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-09-06:

PASSED: Continuous integration, rev:257
http://jenkins.qa.ubuntu.com/job/indicator-datetime-ci/92/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-datetime-saucy-amd64-ci/46
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-datetime-saucy-armhf-ci/47
        deb: http://jenkins.qa.ubuntu.com/job/indicator-datetime-saucy-armhf-ci/47/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/indicator-datetime-ci/92/rebuild

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-09-06:

PASSED: Continuous integration, rev:257
http://jenkins.qa.ubuntu.com/job/indicator-datetime-ci/93/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-datetime-saucy-amd64-ci/47
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-datetime-saucy-armhf-ci/48
        deb: http://jenkins.qa.ubuntu.com/job/indicator-datetime-saucy-armhf-ci/48/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/indicator-datetime-ci/93/rebuild

review: Approve (continuous-integration)

Revision history for this message

Marco Trevisan (Treviño) (3v1n0) wrote on 2013-09-06:

Charles: I'm talking about the desktop... I only see the day and the "Add event…" menu item. But no calendar widget.

I don't know if this may be due to the fact that one of my calendars seems to be lazy and the reply to the call doesn't arrive (same issue that lead to bug #1204532).

Revision history for this message

Ted Gould (ted) wrote on 2013-09-06:

This is good for me now. Thanks!

review: Approve

Revision history for this message

Marco Trevisan (Treviño) (3v1n0) wrote on 2013-09-06:

Sorry I only had a configuration issue... Calendar is working as it should here now.

Revision history for this message

Charles Kerr (charlesk) wrote on 2013-09-06:

Ted, thanks for catching that regression -- it shouldn't've slipped through. I've started a new side-branch to increase test coverage in datetime.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes:

Charles Kerr

Indicator Applet Developers