Closed Bug 943104 Opened 8 years ago Closed 7 years ago
[B2G][Clock][Alarm]App lists Monday as first day of week when choosing days for alarm to go off
90.35 KB, image/png
87.99 KB, image/png
52 bytes, text/plain
When the user is setting an alarm using the Clock app, they may choose which days of the week to trigger the alarm. The list of days starts with Monday and ends with Sunday. This is inconsistent with other apps, which list the days starting with Sunday and ending with Saturday. Repro Steps: 1) Updated Buri to Build ID: 20131125004001 2) Launch Clock App. 3) Tap the bell icon in the upper right corner to create a new alarm. 4) Tap on the 'Repeat' droplist. 5) Observe droplist. Actual: First day listed is Monday. Expected: First day listed is Sunday. (This is consistent with other apps) Environmental Variables Device: Buri v1.2 Moz RIL Build ID: 20131125004001 Gecko: http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/368ea26d2136 Gaia: c2dea53b36bb9d4331a94976344515f60dc5a3d4 Platform Version: 26.0 RIL Version: 01.02.00.019.102 Firmware Version: V1.2_20131115 Notes: Repro frequency: 100% See attached screenshot.
Issue also occurs in 1.1
i am working in this bug.
Hi Vishnu, I'm doing some bug cleanup -- are you still working on this bug?
i finished it but you can cleanup.
Cool, could you post a link to your patch or a pull request?
Hi, I have made a pull request at https://github.com/mozilla-b2g/gaia/pull/15584 Please review it. Thank you.
Attachment #8363522 - Flags: review?(m)
Comment on attachment 8363522 [details] Pointer to Pull Request.html Thanks for the patch! Unfortunately, we actually need to go a little further than just a constant change to the weekday start. There's an i18n variable called 'weekStartsOnMonday' (a string '0' or '1') which might change depending on the locale. See <https://github.com/mozilla-b2g/gaia/pull/7620/files> for a somewhat old PR that did that. So we'll need to do something that checks the 'weekStartsOnMonday' i18n variable, and change the display based on that. We should be able to just change the order in the DOM and leave the other calculating logic the same. Does that make sense?
Attachment #8363522 - Flags: review?(m) → review-
if we not change in Index.html then Monday is display first and Sunday is last.so for displaying Monday is first i think it is necessary.
Assignee: nobody → m
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)
Comment on attachment 8364176 [details] Gaia PR https://github.com/mozilla-b2g/gaia/pull/15632#issuecomment-33100657 Comments on GH!
I've added a few comments to the pull request, as well.
Comment on attachment 8364176 [details] Gaia PR Hey Gareth and Mike, I talked to Gareth about this a bit during our workweek last week. Long story short, right now I'm assigned to three very similar bugs dealing with localization, so I felt it would be less error-prone to just fix it all in one go. I think it's a reasonable change to coalesce. Part of that involved slightly refactoring and simplifying the l10n shims we use, because I found that when testing I had a hard time following which shim was used for what purpose. Here's what I've changed, and if you disagree with this we can talk about it: - I've changed all references of a RequireJS-loaded 'l10n' library with direct references to `navigator.mozL10n`. My rationale is that because it's hoisted on `navigator`, it makes sense to treat it as a browser-supplied library for the purposes of dealing with localization. This simplifies code needing to use localization, in that it can always assume `navigator.mozL10n` will be present. - I've removed the 'l10n' and 'navigator_l10n' library fake shims from test/unit/, replacing them with a brief static shim in `setup.js` for unit tests. Previously, it was confusing as to which l10n shim was being used during testing; in line with my rationale above, it seems simpler to just assume that we have `mozL10n` available always, and it's shimmed appropriately for unit tests. - Note for Gareth: Disregard my comments from the workweek about using the real `navigator.mozL10n`; that's no longer applicable because I ended up using the static "fake" responses we previously used for returning strings from mozL10n.get(). In other words, we never look at the real translated strings in tests, so we don't have to worry about translators' changes messing anything up. - Also note for Gareth: I added unit tests for this bug's case; see `alarm_edit_test.js` line 233. - Mike has refactoring out in bug 922128, which should land after the integration tests in bug 907177 land pending Gareth's review. I'm OK with waiting to land this until after the integration tests land and then adding integration tests for this before finally landing this.
Also, potentially relevant: The other bugs I mentioned, which this patch also fixes: bug 913409 and bug 949082.
Hey Marcus, I understand your rationale for using a global reference instead of an explicit dependency, but I don't understand your motivation. Using the global definitely simplifies the code (since it has one less call to `require`), but the same argument could be made for any module. (One of the biggest benefits of AMD is the explicit management of dependencies, after all.) You also mention confusion about the l10n stub in the unit test environment. Can you elaborate on that a bit? Ideally, I would like to find some solution that simplifies the test setup but does not introduce a new global variable.
The shared l10n library automatically hoists itself onto navigator.mozL10n (whether we like it or not), so it's not creating another global, it's just reusing one that was already there. The confusion with the test setup was mainly because there were a couple of different l10n references -- those to navigator.mozL10n, which was shimmed, and also to the l10n library itself, which was also shimmed. It seems unnecessary to have both, when we could just reuse the existing one that the shared library provides.
To elaborate slightly more, I see the `navigator.mozL10n` interface as something system-wide, system-provided; the infrastructure supporting the localization was really designed to be something the entire system can take advantage of. To me, the fact that they hoisted mozL10n on navigator implies the same -- just as with any other API like mozAlarms, mozPower, etc. If, at some point, mozL10n becomes fully baked in (as the other navigator APIs are), having a completely separate loaded module would just be duplication. If they didn't hoist mozL10n on navigator and returned it as a normal module, obviously it wouldn't make sense to add another global just for this. But because the l10n library creates that reference no matter what, I see no reason to not take advantage of it. It eliminates the confusion of "why are there references to navigator.mozL10n when there's this shared l10n library too?"; similarly, shimming both is redundant. From my perspective, if "l10n" is going to provide `navigator.mozL10n` no matter what, it complicates things to _not_ use it. The fact that it's in shared is just an implementation detail -- that it isn't built in to B2G by default. If they decided to change their minds and treat l10n as a library in B2G apps, then I'd probably advocate for the other way around -- but also getting rid of any references to `navigator.mozL10n` in unit tests as well.
Okay, you convinced me. I'll update the PR to revert most of the changes referring to navigator, and further I'll try to make sure we don't have any unnecessary references to navigator.mozL10n. Your idea about AMDifying the shared/ module seems good, though I don't particularly want to tackle that right now :-) if you want to. My main beef is that it should be one way or the other -- and you've convinced me to go all-AMD for this and try to ignore the exported global unless there comes a day when it's actually built-in without extra JS requirements.
Comment on attachment 8364176 [details] Gaia PR Canceling :gaye's review, will probably pass it to Mike when I've reverted some of the l10n changes.
(In reply to Marcus Cavanaugh [:mcav] <firstname.lastname@example.org> from comment #19) > Your idea about AMDifying the shared/ module seems good, though I don't > particularly want to tackle that right now :-) if you want to. On it! See bug 968443
Okay, here's the cleaned-up patch, much easier to digest. :-) The only thing I changed with mozL10n is to remove the "navigator.mozL10n" shim from tests; it appears it was unused anyway. Unit tests pass.
Target Milestone: 1.3 C3/1.4 S3(31jan) → 1.4 S1 (14feb)
Comment on attachment 8371102 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15632/files Reflag when ready for re-re-review :)... comments on GH
Comment on attachment 8371102 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15632/files Hey Marcus, Looks like we have some integration test failures. Let me know if you'd like help debugging them!
Yeah, I don't think I accounted for your new integration tests when I wrote the latest changes. I'll update and re-flag both you and Gareth when these are ready.
(thanks for using the l10n value and not blindly changed to Sunday ;) )
Comment on attachment 8371102 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15632/files Okay Gareth and Mike, both the Marionette and unit tests (and Travis) pass. Could you take another look? One small note -- Mike, in the wait() we have for hitting the Lap button, my computer still needed a nudge extra time. Maybe there's a touch of latency somewhere along the chain that causes the lap to not register after we issue a .wait(11) call; I changed it to wait for 20ms and it works properly for me now. I think that should be fine, but wanted to clarify why I changed that value. I'm running it on a Mac Pro which could explain the speed discrepancy.
Attachment #8371102 - Flags: review?(gaye) → review+
Comment on attachment 8371102 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15632/files Looks good to me! (In reply to Marcus Cavanaugh [:mcav] <email@example.com> from comment #30) > Comment on attachment 8371102 [details] > Link to Github pull-request: > https://github.com/mozilla-b2g/gaia/pull/15632/files > > One small note -- Mike, in the wait() we have for hitting the Lap button, my > computer still needed a nudge extra time. Maybe there's a touch of latency > somewhere along the chain that causes the lap to not register after we issue > a .wait(11) call; I changed it to wait for 20ms and it works properly for me > now. I think that should be fine, but wanted to clarify why I changed that > value. I'm running it on a Mac Pro which could explain the speed discrepancy. This really should be hardware independent: `client.helper.wait` is implemented via a call to `setTimeout` in the context of the application. I'm thinking this may actually be a bug in the implementation of `start`. I'll follow up with you on a separate bug because I won't be able to verify my fix.
Attachment #8371102 - Flags: review?(mike) → review+
Landed in master: https://github.com/mozilla-b2g/gaia/commit/dfc094b8485f526a6a2a58ef90c172ea11b7fd88
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
[Verified info] Gaia 6e71ab4da1b08586ea0c758edb7aa199ee34cd2f Gecko https://hg.mozilla.org/mozilla-central/rev/bb030d47c946 BuildID 20140219160202 Version 30.0a1 Bug has fixed, I mark it to VERIFIED.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.