Indicator Date and Time

Merge lp://staging/~laney/indicator-datetime/timedated-timezone-property into lp://staging/indicator-datetime/15.10

  1. timedated-timezone-property
  2. Merge into trunk.15.10
Proposed by Iain Lane
Status: Rejected
Rejected by: Iain Lane
Proposed branch: lp://staging/~laney/indicator-datetime/timedated-timezone-property
Merge into: lp://staging/indicator-datetime/15.10
Diff against target: 1240 lines (+533/-351)
10 files modified
include/datetime/timezone-timedated.h (+10/-8)
include/datetime/timezones-live.h (+4/-4)
src/CMakeLists.txt (+2/-3)
src/main.cpp (+3/-3)
src/timezone-timedated.cpp (+155/-63)
src/timezones-live.cpp (+2/-3)
tests/CMakeLists.txt (+1/-1)
tests/test-live-actions.cpp (+21/-227)
tests/test-timezone-timedated.cpp (+34/-39)
tests/timedated-fixture.h (+301/-0)
To merge this branch: bzr merge lp://staging/~laney/indicator-datetime/timedated-timezone-property
Reviewer Review Type Date Requested Status
Lars Karlitski (community) Needs Fixing
PS Jenkins bot (community) continuous-integration Approve
Indicator Applet Developers Pending
Review via email: mp+269732@code.staging.launchpad.net

Description of the change

Use timedated's Timezone property to find out the current timezone and learn about changes to it

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:423
http://jenkins.qa.ubuntu.com/job/indicator-datetime-ci/326/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-datetime-wily-amd64-ci/16
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-datetime-wily-armhf-ci/16
        deb: http://jenkins.qa.ubuntu.com/job/indicator-datetime-wily-armhf-ci/16/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Revision history for this message
Iain Lane (laney) wrote :

Some IRC chat reveals that a good option to read the initial state is to get this from /etc/timezone on disk directly, then use dbus after that.

lp://staging/~laney/indicator-datetime/timedated-timezone-property updated
424. By Iain Lane

Avoid nested GMainLoops by reading from the file on startup

Restore some tests for this functionality.

425. By Iain Lane

Make test-live-actions quit itself once the tz has been changed, the mock no longer does this

Revision history for this message
Iain Lane (laney) wrote :

Updated - please re-review.

Is taking a reference to the GDBusConnection sane? Without it the testsuite kills the connection and the AddMatch fails, which causes some criticals.

(process:12769): GLib-GIO-CRITICAL **: Error while sending AddMatch() message: The connection is closed

(process:12769): GLib-GIO-CRITICAL **: g_dbus_connection_call_finish_internal: assertion 'G_IS_DBUS_CONNECTION (connection)' failed

Maybe we don't care or there's a better way to ensure this can't fail?

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:425
http://jenkins.qa.ubuntu.com/job/indicator-datetime-ci/327/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-datetime-wily-amd64-ci/17
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-datetime-wily-armhf-ci/17
        deb: http://jenkins.qa.ubuntu.com/job/indicator-datetime-wily-armhf-ci/17/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Revision history for this message
Lars Karlitski (larsu) wrote :

Thanks for working on this!

It's causing some unnecessary bus traffic though:

 - watch org.freedesktop.timedate1
 - call GetProperties and fetch all properties of that object
 - subscribe to PropertiesChanged of all properties instead of just the one

Really all you need here is to add a match rule for PropertiesChanged for that *one* property you're interested in. Using g_dbus_connection_signal_subscribe() results in only one call to the daemon and you'll only get signals when the property you're interested in changes.

(By the way, the name_vanished() handler is called immediately when the name doesn't exist. Currently it's spewing some GObject warnings because you're trying to access the proxy without checking it for NULL. However, don't fix this if you stop watching the name anyway)

review: Needs Fixing
Revision history for this message
Iain Lane (laney) wrote :

There is no point in reviewing this.

I'm not working on this change. Get it in some other way.

Unmerged revisions

425. By Iain Lane

Make test-live-actions quit itself once the tz has been changed, the mock no longer does this

424. By Iain Lane

Avoid nested GMainLoops by reading from the file on startup

Restore some tests for this functionality.

423. By Iain Lane

Add some comments

422. By Iain Lane

Rename FileTimezone to TimedatedTimezone

421. By Iain Lane

Add a timeout so we can't hang forever

420. By Iain Lane

Use timedated's Timezone property instead of watching /etc/timezone

Still need to rename everything to not use "timezone-file"

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