Merge lp://staging/~charlesk/indicator-datetime/lp-1429388-fix-start-of-day-calculation into lp://staging/indicator-datetime/15.04

Proposed by Charles Kerr
Status: Merged
Approved by: Ted Gould
Approved revision: 413
Merged at revision: 404
Proposed branch: lp://staging/~charlesk/indicator-datetime/lp-1429388-fix-start-of-day-calculation
Merge into: lp://staging/indicator-datetime/15.04
Diff against target: 968 lines (+352/-188)
19 files modified
include/datetime/date-time.h (+14/-6)
src/actions-live.cpp (+2/-3)
src/actions.cpp (+9/-3)
src/alarm-queue-simple.cpp (+1/-1)
src/clock-live.cpp (+1/-1)
src/date-time.cpp (+91/-32)
src/menu.cpp (+2/-2)
src/planner-month.cpp (+2/-7)
src/planner-upcoming.cpp (+1/-1)
tests/CMakeLists.txt (+1/-0)
tests/manual-test-snap.cpp (+2/-6)
tests/test-actions.cpp (+18/-15)
tests/test-alarm-queue.cpp (+4/-15)
tests/test-datetime.cpp (+143/-0)
tests/test-formatter.cpp (+35/-52)
tests/test-live-actions.cpp (+11/-22)
tests/test-menus.cpp (+2/-6)
tests/test-planner.cpp (+10/-10)
tests/test-snap.cpp (+3/-6)
To merge this branch: bzr merge lp://staging/~charlesk/indicator-datetime/lp-1429388-fix-start-of-day-calculation
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Ted Gould (community) Approve
Review via email: mp+252987@code.staging.launchpad.net

Commit message

Fix bug that prevented clicking on calendar days where DST sprang forward.

Description of the change

== Description of Change

The bug is coming from calculating the start-of-the-day with day.add_full(0,0,0,-day.hours(),-day.minutes(),-day.seconds()) on a day when DST changes the hour count.

The fix is to replace that code with use date_time_new(tz, y, m, d, 0, 0, 0). Unfortunately GDateTime's timezone pointer is private, so to pass the right tz as the first argument, our DateTime class needs to keep its own copy. This changes DateTime's public API since it can't construct or assign from a simple GDateTime pointer anymore -- it always needs an explicit timezone as well.

The -day.hours() idiom was used in several places, so new convenience methods DateTime.start_of_day() and DateTime.start_of_minute() are added and used.

So this patch:

1. Adds a regression test to confirm the fix
2. Replaces the add_full(-day.hours()) idiom with start_of_day().
3. Syncs DateTime instantiation/assignment with the necessary API changes

== Checklist

> Are there any related MPs required for this MP to build/function as expected? Please list.

No

> Is your branch in sync with latest trunk? (e.g. bzr pull lp:trunk -> no changes)

Yes

> Did the code build without warnings?

Yes

> Did the tests run successfully?

Yes

> Did you perform an exploratory manual test run of your code change and any related functionality?

Yes

> If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?

N/A

> What device (or emulator) has your component test plan been executed successfully on?

Tested on vivid desktop.

This feature doesn't exist on phone, so device image is N/A

> What manual tests are relevant for this MP?

None -- the MP includes an automated regression test.

> Did you include a link to the MR Review Checklist Template to make your reviewer's life easier?

https://wiki.ubuntu.com/Process/Merges/Checklists/indicator-datetime

To post a comment you must log in.
408. By Charles Kerr

in DateTime::Local() and DateTime::NowLocal(), avoid redundant construction of the local timezone.

409. By Charles Kerr

fix GAction leakage found while testing new unit tests with valgrind

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Ted Gould (ted) wrote :

Only a couple small comments inline. I like the new start_of*() functions, I think it makes the code more clear.

Revision history for this message
Ted Gould (ted) :
review: Approve
410. By Charles Kerr

make utc variable name more sensible.

411. By Charles Kerr

in DateTime::DateTime(GTimeZone*,GDateTime*), don't allow either argument to be nullptr

412. By Charles Kerr

in DateTime::is_set(), include timezone test

413. By Charles Kerr

add DateTime::end_of_month(), DateTime::end_of_day(). Add unit tests for them.

Revision history for this message
Charles Kerr (charlesk) wrote :

Responses in inline comments and in revisions 410 through 413

Revision history for this message
Ted Gould (ted) wrote :

+1 more :-)

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)

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