[clock] Clock refactor alarm object and add tests

RESOLVED FIXED

Status

Firefox OS
Gaia::Clock
--
enhancement
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: oconnore, Assigned: oconnore)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

46 bytes, text/x-github-pull-request
jugglinmike
: review+
Details | Review | Splinter Review
62.93 KB, image/png
Details
(Assignee)

Description

4 years ago
Currently, alarms are tracked using object literals which have no constructors.

We should use an Alarm object with a real constructor and prototype, and use that API consistently across the application.
(Assignee)

Comment 1

4 years ago
Created attachment 781997 [details] [review]
Pull request on github

My pull request also includes Mike's patches, because those haven't been merged yet.

I marked this bug as dependent on Mike's bugs.
Attachment #781997 - Flags: review?(iliu)
(In reply to Eric O'Connor [:oconnore] from comment #1)
> Created attachment 781997 [details] [review]
> Pull request on github
> 
> My pull request also includes Mike's patches, because those haven't been
> merged yet.
> 
> I marked this bug as dependent on Mike's bugs.
Hi Eric,

Thanks for the point out:) Mike has landed these patches.
Could you please rebase and reopen the pull request on Github?
I will comment for the pr.
Comment on attachment 781997 [details] [review]
Pull request on github

Eric,

I have reviewed the pr quickly. Basically, I agree with the majority modification. Since the pr is at close state, I would like to leave my comment on it. In order to mange the code base accurately, I prefer to land one major change(module) via one pull request(bug). It's also helpful for reviewing effort. We can check each functionality during reviewing stage.

So, could you please separate the request with one major change to one pull request, if it's possible to be subdivided. Thank you so much.
Attachment #781997 - Flags: review?(iliu) → review-
(Assignee)

Comment 4

4 years ago
Created attachment 786379 [details] [review]
pull request on master

Hi Ian,

I just made a new pull request with additional testing and fixes for the informal review that Rick gave me. 

I am not sure that it's possible to split this into multiple commits, since many of the changes are co-dependent.
Attachment #781997 - Attachment is obsolete: true
Attachment #786379 - Flags: review?(iliu)
(Assignee)

Updated

4 years ago
Depends on: 902305
Comment on attachment 786379 [details] [review]
pull request on master

This is a big refactor. I have addressed some comment on Github. Beside of these comment, I'm care about the functionality normally or not. Most of the broken feature are happened while alarm goes off. You have to make sure the alarm state for each case(repeat/non-repeat with snoozed/non-snoozed). The main problem are as following which be found out quickly.

1. The snoozed alarm is not canceled while user modify a alarm which is be scheduled with snooze.
2. Don't close(handle) the multiple alarms case while these alarms goes off at the same time.
3. The alarm status of alarm list is not correct while user closed the repeat alarm.
Attachment #786379 - Flags: review?(iliu) → review-
(Assignee)

Comment 6

4 years ago
Hi Ian,

I updated the PR addressing your comments on github and rebased onto master. 

1. This is fixed here: https://github.com/oconnore/gaia/commit/d8e1fba78b08153153b27c1fad857b7cd06b6e38#L3R354

2. I don't quite understand this comment. The existing behavior on  https://github.com/mozilla-b2g/gaia/blob/935b9b4849353568535d5063f49c14484df9efa2/apps/clock/js/active_alarm.js#L85 closes previous alarms. How are you proposing we change that? 

3. I'm not quite sure what you are referring to here. I believe this should be correct (after addressing some comments you made on the PR). 

Thanks for the feedback! I am flagging you for review again.
(Assignee)

Updated

4 years ago
Attachment #786379 - Flags: review- → review?(iliu)
(In reply to Eric O'Connor [:oconnore] from comment #6)
> 2. I don't quite understand this comment. The existing behavior on 
> https://github.com/mozilla-b2g/gaia/blob/
> 935b9b4849353568535d5063f49c14484df9efa2/apps/clock/js/active_alarm.js#L85
> closes previous alarms. How are you proposing we change that? 
Looks like it does not work as before with the refactor.
Step:
a. Set multiple alarms with the same alarm time(Alarm A --> am8:00, Alarm B --> am8:00, Alarm C --> am9:00, given different label is helpful to identify them)
b. Wait the time is up
c. Will see only one attention screen while the alarm time we set
d. Make sure that other alarm is turned off without attention screen
> 
> 3. I'm not quite sure what you are referring to here. I believe this should
> be correct (after addressing some comments you made on the PR). 
Step:
a. Set a repeat alarm
b. Wait alarm goes off
c. Close the alarm
d. Make sure the alarm status:
   Indicator on status bar: alarm icon is enable
   The alarm in alarm list: is disable(It should be enable since it's a repeat alarm.)
> 
> Thanks for the feedback! I am flagging you for review again.
Thanks.
(In reply to Ian Liu [:ianliu] from comment #7)
> (In reply to Eric O'Connor [:oconnore] from comment #6)
> > 2. I don't quite understand this comment. The existing behavior on 
> > https://github.com/mozilla-b2g/gaia/blob/
> > 935b9b4849353568535d5063f49c14484df9efa2/apps/clock/js/active_alarm.js#L85
> > closes previous alarms. How are you proposing we change that? 
> Looks like it does not work as before with the refactor.
> Step:
> a. Set multiple alarms with the same alarm time(Alarm A --> am8:00, Alarm B
> --> am8:00, Alarm C --> am9:00, given different label is helpful to identify
> them)
> b. Wait the time is up
> c. Will see only one attention screen while the alarm time we set
> d. Make sure that other alarm is turned off without attention screen
> > 
It still does not work. The indicator and status of alarm list are not correct.(Alarmdb is right, but UI layout is out of updated.) Looks like parallel callback function is not be run in this case.
https://github.com/mozilla-b2g/gaia/pull/11383/files#L1R40
> > 3. I'm not quite sure what you are referring to here. I believe this should
> > be correct (after addressing some comments you made on the PR). 
> Step:
> a. Set a repeat alarm
> b. Wait alarm goes off
> c. Close the alarm
> d. Make sure the alarm status:
>    Indicator on status bar: alarm icon is enable
>    The alarm in alarm list: is disable(It should be enable since it's a
> repeat alarm.)
> > 
For the section, it works fine after applied your latest patch. Thanks.
> > Thanks for the feedback! I am flagging you for review again.
> Thanks.
Comment on attachment 786379 [details] [review]
pull request on master

Eric,

Could you actually test the code before submitting for review? It will help you to find out the major problem and decrease our reviewing time between different timezone. Please fix the above issue and Rick's comment. And set review again for me. Thanks.
Attachment #786379 - Flags: review?(iliu)
Eric asked me to check out his patch and test out these features. They seem to
be working for me. I want to be sure that I am not making any incorrect
assumptions about the intended behavior, so I will document my experience in
detail.

Eric and Ian: please let me know where your experience is different or if some
part of my report reflects a bug.

---

## 12:36PM

1. I created a new alarm for 12:39PM. I named it "a", changed the "Sound"
   setting to "No Sound", and ensured that the "Repeat" setting was "Never".
2. I created a new alarm for 12:39PM. I named it "b", changed the "Sound"
   setting to "No Sound", and ensured that the "Repeat" setting was "Never".
3. I created a new alarm for 12:43PM. I named it "c", changed the "Sound"
   setting to "No Sound", and ensured that the "Repeat" setting was "Never".
   Here is a text-based representation of the main Clock view as it appeared on
   my phone after taking these steps:
   
   > 12:39 PM   a      [x]
   > 12:39 PM   b      [x]
   > 12:43 PM   c      [x]

## 12:38 PM

4. The phone began to vibrate and the "attention screen" was rendered. It
   described alarm "b".
5. I tapped the button labeled "Close".
6. The phone stopped vibrating and the main Clock view was rendered.  Alarm "c"
   was still active and rendered at the top of the list. Alarms "a" and "b"
   were inactive and rendered below (in reverse order). Here is a text-based
   representation:
   
   > 12:43 PM   c      [x]
   > 12:39 PM   b      [ ]
   > 12:39 PM   a      [ ]

## 12:43 PM

7. The phone began to vibrate and the "attention screen" was rendered. It
   described alarm "c".
8. I tapped the button labeled "Close".
9. The phone stopped vibrating and the main Clock view was rendered. All three
   alarms were inactive. The alarms were rendered in the same order.  Here is a
   text-based representation:
   
   > 12:43 PM   c      [ ]
   > 12:39 PM   b      [ ]
   > 12:39 PM   a      [ ]
Flags: needinfo?(iliu)
Flags: needinfo?(eric)
Created attachment 789125 [details]
refactor-full-pass.png

This screenshot illustrates the current green status of this patch
Hi Mike, 

Your experience is also working fine for me. And I find out that I wrote an incorrect alarm time in comment 7. It should be "Alarm C --> am9:00" ==> "Alarm C --> am8:00". 

It means that we set three alarms or more with same alarm time. The callback could not be run for updating indicator and alarm status while alarm goes off.(https://github.com/mozilla-b2g/gaia/pull/11383/files#L1R40) My reproduced rate is 60%. It is not hard to repro it. But it's an edge case for a user. Is it possible to have unit test for this case? Thanks.
Flags: needinfo?(iliu)
(In reply to Rick Waldron from comment #11)
> Created attachment 789125 [details]
> refactor-full-pass.png
> 
> This screenshot illustrates the current green status of this patch

Unit test is very important and meaningful for verifying a pull request. But some of the user stories are not be covered via it. That is why Productivity team is implementing integration test for these case. So, I would like to say that unit test is a good preliminary reference for expected function input/output. But we still need to check the functionality carefully while landing a patch. Especially this is a big refactor patch.
(Assignee)

Comment 14

4 years ago
Comment on attachment 786379 [details] [review]
pull request on master

Resolved issue with 3 alarms, where the onload event was not firing.

Added comments.

Added property hiding for id, repeat, enabled, and registeredAlarms.
Attachment #786379 - Flags: review?(mike)
Comment on attachment 786379 [details] [review]
pull request on master

Alrighty, Eric: I've finished reviewing your patch on GitHub. You can find my comments there.

This feedback didn't fit as an in-line comment, so I'll leave it here: can you change the subject on the commit to "Bug 898666 - [clock] Clock refactor alarm object and add test"?
Attachment #786379 - Flags: review?(mike)
(Assignee)

Comment 16

4 years ago
Comment on attachment 786379 [details] [review]
pull request on master

Pushed review changes
Attachment #786379 - Flags: review?(mike)
Comment on attachment 786379 [details] [review]
pull request on master

Great work, Eric! I'll land this for ya
Attachment #786379 - Flags: review?(mike) → review+
master: https://github.com/mozilla-b2g/gaia/commit/3746b1cdc0e344460c58cd52f85564b0c4f5940a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
backed out: https://github.com/mozilla-b2g/gaia/commit/3746b1cdc0e344460c58cd52f85564b0c4f5940a

This patch caused a UI regression--new Alarm list items were appended to the alarms list (rather than inserted at the beginning).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
master: https://github.com/mozilla-b2g/gaia/commit/46dcd65c2075f0872a4597169ad9c9f7bdba0335

Thanks for working so quickly to resolve the regression, Eric
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Flags: needinfo?(eric)
Attachment mime type: text/plain → text/x-github-pull-request
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.