[Calendar] simplify time controller logic

RESOLVED FIXED in 2.1 S9 (21Nov)

Status

Firefox OS
Gaia::Calendar
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: millermedeiros, Assigned: millermedeiros)

Tracking

unspecified
2.1 S9 (21Nov)
Dependency tree / graph

Firefox Tracking Flags

(tracking-b2g:+)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
right now "controllers/time.js" have ~800 LOC and a complex busytimes/events cache mechanism (based on the IntervalTree, TimeObserver and binsearch) since it needs to handle different "scales" (week/day/month).

my proposal is to stop caring about different ranges and replace the caching logic with something that is "day based" and that doesn't require a binary tree. as soon as all views starts using the DayObserver there is no point in keeping the complex structure in place.

I'm expecting at least two modules; one to keep track of the current date selection; and another one to cache the busytimes/events data. - killing the cache that we have on DayObserver or maybe merging the cache with the DayObserver...

this should make the codebase easier to understand & change in the future.
(Assignee)

Comment 1

4 years ago
[Tracking Requested - why for this release]: part of the "large refactor" that we discussed on the productivity work week (July 2014). should reduce complexity drastically. this is a part of the app that was related to many bugs in the past and it was hard to track root causes.
tracking-b2g: --- → ?
(Assignee)

Updated

4 years ago
Blocks: 1027729
tracking-b2g: ? → +
(Assignee)

Comment 2

4 years ago
I just found out a race condition on the current dayObserver logic.. if the first dispatch happens before we are able to query events from the cache it will think there are no events for that day. (hard to reproduce on Flame, but can happen on new month view).

Another thing with current day observer logic is that we are calling `timeController.findAssociated` which is "expensive", specially when executed over multiple days (needs to talk to event & alarm stores). We should really simplify the way the data is cached to avoid querying for same data, over and over again. - For the month view we don't even need the "associated data", we just need to know the amount of busytimes on that given day.
Assignee: nobody → mmedeiros
Target Milestone: --- → 2.1 S9 (21Nov)
(Assignee)

Updated

4 years ago
Blocks: 1095621
(Assignee)

Updated

4 years ago
Blocks: 1097788
(Assignee)

Updated

4 years ago
Blocks: 1094740
(Assignee)

Comment 3

4 years ago
Created attachment 8529474 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26511

James and Kevin, I need one of you guys (or maybe both) to take a look at these changes on the Calendar app since Gareth is away. It is basically a refactor on the way we load/query/cache busytimes & events to boost the performance and also simplify the views logic drastically.

This is part of a bigger refactor that I started a couple months ago when implementing the 5-day week view.

Thanks!
Attachment #8529474 - Flags: review?(kgrandon)
Attachment #8529474 - Flags: review?(jlal)
Comment on attachment 8529474 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26511

Hey Miller! I'd love to help you get this landed. Unfortunately we're currently off for the next two days, and with Portland approaching I don't think I'd be able to get to this until mid to late next week. Swapping the review here for a needinfo and I will review this asap.

Sorry for the delay - the timing is just not good for me for such a big patch.
Flags: needinfo?(kgrandon)
Attachment #8529474 - Flags: review?(kgrandon)
Hey Miller - do I need to apply the patch from bug 1084014 to test this? I'll take a look at the code, but I think it would be good to apply and test this before leaving the R+. Thanks!
Flags: needinfo?(kgrandon) → needinfo?(mmedeiros)
(Assignee)

Comment 6

4 years ago
Hi Kevin, my PR contains commits for both bugs, so you don't need to apply any extra patch.
Flags: needinfo?(mmedeiros)
(Assignee)

Updated

4 years ago
See Also: → bug 1107573
(Assignee)

Comment 7

4 years ago
Comment on attachment 8529474 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26511

since gareth is back will ask him to review it
Attachment #8529474 - Flags: review?(jlal) → review?(gaye)
(Assignee)

Updated

4 years ago
Blocks: 1079557
Comment on attachment 8529474 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26511

Left some comments on GH but overall looks great. Nice work!
Attachment #8529474 - Flags: review?(gaye) → review+
(Assignee)

Comment 9

4 years ago
landed on master: https://github.com/mozilla-b2g/gaia/commit/7445d52e351859293bf7f77e357d25d211787ae6
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
See Also: → bug 1112400
You need to log in before you can comment on or make changes to this bug.