Fix alarms firing in server timezone instead of player timezone#1514
Fix alarms firing in server timezone instead of player timezone#1514boudekerk wants to merge 12 commits intoLMS-Community:public/9.2from
Conversation
When a player in a different timezone to the server sets an alarm, the alarm would fire at the correct time according to the server's local clock rather than the player's. For example, a player in America/New_York connected to a server in Europe/Amsterdam would have a 7:00 AM alarm fire at 1:00 AM local time. The fix stores an optional Olson timezone name (_timezone) with each alarm. When present, findNextTime() uses that timezone for scheduling instead of the server's localtime(). Backward compatibility is preserved: alarms without a timezone continue to use server local time unchanged. On the server side, the Jive alarm menu now includes the player's registered timezone in the alarm add/update action params, so it is sent automatically when the player creates or modifies an alarm via the touch UI. Signed-off-by: Bartosz Oudekerk <lot+github@unreachablehost.net>
The rescan plugin's checkScanTimer() used localtime() (server timezone) to interpret the scheduled time, causing misfires for players in different timezones. - Add rescanplugin settimer command accepting time + timezone params - Jive menu now passes the player's timezone (from playerpref) when scheduling a rescan time via the device UI - checkScanTimer() overrides TZ/tzset() when a timezone is stored, falling back to server localtime() for existing schedules (backwards compatible) - Web settings UI captures browser timezone via Intl.DateTimeFormat and stores it alongside the scheduled time Signed-off-by: Bartosz Oudekerk <lot+github@unreachablehost.net>
…mezone When a user sets an alarm time via the web UI, the entered time should be interpreted in the browser's timezone (where the user and player are), not the server's timezone. Capture the browser timezone via Intl.DateTimeFormat and store it on the alarm object, consistent with the approach used for scheduled rescan. Signed-off-by: Bartosz Oudekerk <lot+github@unreachablehost.net>
86ae5db to
339b1db
Compare
|
I'm sorry, that's not going to happen. We're not adding 400+ files to solve one person's issue - which likely could be solved using an environment variable. |
|
@michaelherger It's still work in progress. But thanks for keeping an eye on it. All comments are appreciated. |
… scheduling
POSIX::tzset with $ENV{TZ} does not work on Windows, where Perl requires
Windows-native timezone names rather than Olson identifiers. This made
alarm scheduling in a player's timezone silently fall back to the server
timezone on Windows.
Replace with DateTime::TimeZone, which uses a bundled IANA timezone
database and works identically on all platforms.
- Add Slim::Utils::DateTime::localtimeInTZ($epoch, $tzName), a thin
wrapper around DateTime::TimeZone that returns a localtime()-style
list in the named Olson timezone, with a per-process cache and a
graceful fallback to server timezone for unrecognised names
- Use localtimeInTZ in Slim::Utils::Alarm::findNextTime and
Slim::Plugin::Rescan::Plugin::checkScanTimer, replacing the
POSIX::tzset save/restore blocks
- Bundle DateTime::TimeZone 2.66 (IANA tzdata 2025c) and its
dependencies in CPAN/
Signed-off-by: Bartosz Oudekerk <lot+github@unreachablehost.net>
ec0d329 to
bf3cbd8
Compare
|
@michaelherger @mw9 @ralph-irving Can I please pick your brain here? @michaelherger commented on was trying out using DateTime::TimeZone. That will result in the cleanest code IMO. And, crucially, the Strawberry perl you're using for the Windows installer already includes this dependency anyway. Including it explicitly in lms, it indeed pulls in 400+ files, although ~300 of those simply contain the timezone data. I see the following options available (let me know if you see others):
1b. is my least favourite as it will increase the maintenance burden for little value. Of the ~400 files the DateTime::TimZone dependency introduces ~300 are simply the timezone data. So trimming it down will be marginal anyway and make dependency updates harder. I'd say either we include it as a dependency or we don't, but let me know if you disagree. My preference is actually 1a. Clean code and a clear dependency. 400 files seems like a lot, but those are included on Windows anyway and 300 of those are simply small timezone data files. Easy to update as well. My second choice would be 1c. Please let me know which approach you would prefer and I can make the required changes and put it forward for review. Thanks. P.S. @michaelherger Your suggestion of an env variable, won't actually work on Windows AFAIK. I'm not sure what workaround for this bug there is available there today. |
|
Another option is to have 1c with a fallback to 2. |
I wanted to suggest this: use if (main::ISWINDOWS) {
...
} |
|
@mw9 @ralph-irving Any comments? If not I'll start work next week on the multi-tiered approach as discussed with @michaelherger. |
No I don't really have anything to offer on these changes. I've never used the alarm features of the firmware. |
…bbr to alarm display - DateTime::TimeZone is now optional; falls back to POSIX::tzset - Remove bundled DateTime::TimeZone CPAN modules (no longer required) - Extract TZName validation into a shared function; apply it consistently across alarm, web UI, and /time/tz - Add tz_abbr to alarm query responses and Jive menu display for user feedback Signed-off-by: Bartosz Oudekerk <lot+github@unreachablehost.net>
- Existing stamped alarms show tz_abbr (e.g. CET) next to the time - New alarm form shows the browser's timezone so the user knows which timezone the alarm will be created in Signed-off-by: Bartosz Oudekerk <lot+github@unreachablehost.net>
POSIX::tzset does not understand Olson timezone names on Windows; fall back to server localtime() there instead of returning misleading results. Signed-off-by: Bartosz Oudekerk <lot+github@unreachablehost.net>
…lable When a new alarm is created without a player timezone, fall back to the server's Olson timezone name so the alarm fires at the intended local time. The server timezone is resolved via a layered fallback and cached on first use. The timezone abbreviation display falls back to the Olson name when the abbreviation cannot be computed. Signed-off-by: Bartosz Oudekerk <lot+github@unreachablehost.net>
- Only stamp timezone on new alarms or when the fire time changes; enable/disable and day changes no longer affect the stored timezone - Server timezone fallback in Commands.pm restricted to alarm add, not update, to avoid retroactively stamping existing alarms - tz_abbr is now always returned in alarm queries (empty string for unstamped alarms) for consistent API behaviour - Alarm scheduling log messages now show time in the alarm's own timezone rather than the server timezone - DateTime::TimeZone load and constructor failures now log the actual error instead of a misleading "Unrecognised timezone" message Signed-off-by: Bartosz Oudekerk <lot+github@unreachablehost.net>
|
@michaelherger I think this is now in a state where it could be merged. If you could have a look and let me know if you agree, that would be appreciated. I've tried to be as defensive as possible with this bugfix. Any existing alarms are left broken
For any new alarms they will use the players timezone. If no player timezone can be determined they will use the server timezone (not time!).
Please let me know what you think. |
|
And I should add that the upgrade is not dependent on the player upgrade. A player on old firmware will simply set old-style alarms. All functionality has been extensively tested, but let me know if I missed something. |
Move the timezone abbreviation to immediately follow the alarm time rather than appearing at the end after the day abbreviations, giving a more natural reading order (e.g. "07:30 CET Ma Di Vr" instead of "07:30 Ma Di Vr CET"). Signed-off-by: Bartosz Oudekerk <lot+github@unreachablehost.net>
Signed-off-by: Bartosz Oudekerk <lot+github@unreachablehost.net>
There was a problem hiding this comment.
Thanks for your dedication. But... is it really worth it? I mean: that's your very personal feature. I don't know whether you'd find another dozen users for it. And you're far from done: what about the Classic UI? Neither rescan nor alarm settings are covered in this PR. Nor could I find setting a timezone for the Radio/Touch?
There's a lot of repeated logic. This should all be implemented in one single place.
I spent about 45 minutes reviewing this. More will be needed. And we'll have to deal with unexpected side effects reported by the community. I'm not sure I want to take this on.
| # Build tz_abbr map for stamped alarms | ||
| my %alarmTzAbbrs; | ||
| for my $alarm ( @{ $paramRef->{'prefs'}->{'alarms'} } ) { | ||
| my $alarmTZ = $alarm->timezone; | ||
| if ($alarmTZ) { | ||
| my $tzAbbr = Slim::Utils::DateTime::tzAbbr($alarm->nextDue || time, $alarmTZ) // $alarmTZ; | ||
| $alarmTzAbbrs{$alarm->id} = $tzAbbr if $tzAbbr; | ||
| } | ||
| } | ||
| $paramRef->{'alarmTzAbbrs'} = \%alarmTzAbbrs; |
There was a problem hiding this comment.
Could this be moved to getAlarams()? That's where we loop through all alarms and build the information for them.
| my $alarmTZ = $alarm->timezone; | ||
| my $tzAbbr = defined $alarmTZ | ||
| ? Slim::Utils::DateTime::tzAbbr($alarm->nextDue || time, $alarmTZ) // $alarmTZ | ||
| : undef; |
There was a problem hiding this comment.
This should be implemented as an accessor. It's done repeatedly, eg. in this very module again on line 1000.
| <div class="prefHead" style="font-weight:normal;">[% "ALARM_SET" | string %]</div> | ||
| <div class="prefs" style="max-height:14px"> | ||
| <input type="text" class="stdedit timeControl" name="alarmtime[% alarm.id %]" id="alarmtime[% alarm.id %]" value="[% alarm.timeStr | html %]" size="15"> | ||
| [% IF alarm.id != newAlarmID AND alarmTzAbbrs.${alarm.id} %] <span style="font-size:smaller;">[% alarmTzAbbrs.${alarm.id} | html %]</span>[% END %] |
There was a problem hiding this comment.
See comment in the handler: this information should be part of the alarm object.
| <input type="hidden" name="alarmtimezone" id="alarmtimezone" value="" /> | ||
| <script> | ||
| document.getElementById('alarmtimezone').value = Intl.DateTimeFormat().resolvedOptions().timeZone; | ||
| </script> |
There was a problem hiding this comment.
I'm not sure I understand what that is supposed to do.
| if (defined $params->{timezone} && Slim::Utils::DateTime::validateTZName($params->{timezone})) { | ||
| $alarm->timezone($params->{timezone}); | ||
| } elsif ($params->{cmd} eq 'add' && !defined $alarm->timezone) { | ||
| my $serverTZ = Slim::Utils::DateTime::getServerTZName(); | ||
| $alarm->timezone($serverTZ) if $serverTZ; | ||
| } |
There was a problem hiding this comment.
Should be handled in the accessor: if tz is defined and valid, set it, otherwise set to default value. That way we'd always have a defined tz per alarm, even for existing ones.
There was a problem hiding this comment.
Regarding the default, since you and others were worried about fallout after this change, I made the design decision to not touch existing alarms. At all. So it possibly being undefined is by design.
By setting a default we'd be introducing a behavioural change. The time becomes immutable and will no longer change with timezone changes.
I'm fine with this as long as the consequence is understood. Let me know which you'd prefer.
There was a problem hiding this comment.
This is for 9.2 - which is considered "under development". Let's go all in: store a timezone with each alarm, whether new or updated. I'm using the alarm myself. I'll tell you if you broke it 😉.
| my $alarmTZ = $alarm->timezone(); | ||
| $request->addResultLoop($loopname, $cnt, 'timezone', $alarmTZ) if defined $alarmTZ; | ||
| my $tzEpoch = $alarm->nextDue() || time; | ||
| my $tzAbbr = defined $alarmTZ | ||
| ? Slim::Utils::DateTime::tzAbbr($tzEpoch, $alarmTZ) // $alarmTZ | ||
| : ''; |
| } | ||
|
|
||
| # 2. OS — Linux/macOS only | ||
| unless ($^O eq 'MSWin32') { |
There was a problem hiding this comment.
please use if (!main::ISWINDOWS) for readability
| if (!$_dttAvailable) { | ||
| my $reason = $@ ? " ($@)" : ''; | ||
| $reason =~ s/\s+$//; | ||
| if ($^O eq 'MSWin32') { |
|
|
||
| # POSIX::tzset fallback — Linux/macOS only; safe in LMS's cooperative | ||
| # single-threaded event loop. Save/restore via 'local' + explicit tzset. | ||
| return localtime($epoch) if $^O eq 'MSWin32'; |
| } | ||
|
|
||
| # POSIX::tzset fallback — Linux/macOS only | ||
| return undef if $^O eq 'MSWin32'; |
Wow, that was quick. Thanks for having a look! I (obviously) do think its worth it. And I do not believe it's that personal. Whether it affects the majority of users or not, there is clearly a bug here (but I'm happy to mark this a new feature if that's what it takes to get it merged). If you search the forum this has come up a number of times over the years and many other users will simply have figured out the issue (like me) and fixed it by scheduling their time accordingly without reporting anything. AFAIK Windows also does not have a workaround for it. Of course proper timezone handling could also be useful for other (future) plugins/features, it's not alarm specific in that sense. I'll have a look at your detailed review comments later. P.S. the Touch is the device I tested with, that's fully handled. I thought the Radio as well, but I don't own one to test, so maybe it works differently. I'll investigate. |
I might have missed it. But I can't find where you set the client side timezone? |
See this PR: ralph-irving/squeezeos-squeezeplay#28 It's set once on startup/init, and then only when a user explicitly changes it. |
Ok, got it. Could you please still add a section to the player's basic settings (web UI)? That would cover other player types, too. And then you'd likely have to tweak the screensavers for the Classic line of players, too (SB1/2/3/Classic, Boom, Transporter). As whatever is displayed on them, including the date/time, is served by LMS, it would need to be adjusted for the different timezone setting. |
Yeah, I'd already had a look earlier at the classics and had come to the very same conclusion. 😎 So I was thinking about showing the timezone for all players in settings (it's still useful to know), but for new players have it "greyed out", with a comment explaining where it can be changed. But how to handle this in the UI? Do you know if there's some convenience method to check which generation of player we're dealing with? Or would we need to maintain a hardcoded list ourselves? |
|
Got it: |
Heh... you beat me to it! Came here to post exactly this. I'd say: for touch/radio: on device. No syncing up from the server. For ip3k: server only, no on-device menu. There aren't too many of them out there. Keep it simple. In the Settings web UI you can add a hint about where to change this to the description string. |
|
@michaelherger I've been thinking to change the approach slightly. I think we can ditch a lot of code, UI & display complexity if we just save the alarm without a timezone and simply have it fire in whatever the player timezone is. I.e. A user sets an alarm for 7:30 Europe/Amsterdam, and after moving to America/New_York, this same alarm now fires at 07:30 America/New_York. Probably what the user wants anyway. This does come with one caveat. We have no way of knowing which the alarms were already present and what the intent for them was. So for users where server tz != player tz, and which set a wrong time so it fires at the right (player) time they will then have their alarm fire at a different time. I.e. my 6:30 alarm (which wakes me at 7:30 local time), will start waking me at 6:30. But as this will be in 9.2 maybe this is an acceptable trade-off to have a better user experience and cleaner code. And just mention it in the release notes knowing it will affect only a small subset of users. I'll have a think about this over the weekend, but would appreciate your thoughts on introducing a breaking change for 9.2. |
I'm glad you did. Because you're right: timezone shouldn't be a property of an alarm, bot of the client device only. All time related handling and display for a device should be as expected by the user in that device's timezone. If I'm sitting in Switzerland, I want to deal with Swiss time, no matter what the server thinks. If I take that radio to the US, I set the device's timezone to the local timezone, don't touch my alarm, and I'm woken up at the same time as at home, but in the new timezone. 6:30 remain 6.30, no calculation from the user needed. The same applies to the time display of Classic players: once the timezone set for the device, the display should show the local (to the device) time. This is what the behaviour was for MySqueezebox at the time: no matter whether your device was connected to the servers in Europe or the US, as long as you set the correct timezone for your player, the server handled it correctly.
Exactly.
Again: that's an absolute minority, and I still bet you'd be having a hard time finding a dozen of them within 24h of searching. Users having to calculate to get what time to set for their alarm will at first be taken by surprise (if they didn't read the announcement), and then will experience delight as they no longer have to do the maths.
Timezone should be a no brainer for 99.99% of the users. Let's not deal with unless the user goes and sets a timezone for their device. In that case the alarm scheduler or the DateTime screensaver should take the timezone into account when scheduling or rendering a point in time. No UI change except allowing the user to select a timezone should be needed. |
|
One more thing: please start your work on a new, clean branch. We've had so much going back and forth here, I wouldn't want to have all those libraries you added at some point in the Git history. Or rebase/squash it to get rid of those commits, if you prefer. |
Glad we're in agreement. The initial responses to my idea and MR seemed to worry (too much IMO) about user impact, so I made design decisions with that in mind. But in the end I figured it just wasn't worth the hassle. I'm closing this one and will open a new mr. |
When a player in a different timezone to the server sets an alarm, the alarm would fire at the correct time according to the server's local clock rather than the player's. For example, a player in America/New_York connected to a server in Europe/Amsterdam would have a 7:00 AM alarm fire at 1:00 AM local time.
The fix stores an optional Olson timezone name (_timezone) with each alarm. When present, findNextTime() uses that timezone for scheduling instead of the server's localtime(). Backward compatibility is preserved: alarms without a timezone continue to use server local time unchanged.
On the server side, the Jive alarm menu now includes the player's registered timezone in the alarm add/update action params, so it is sent automatically when the player creates or modifies an alarm via the touch UI.