Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[settings] fixed: set timezone during initialization #4095

Merged
merged 1 commit into from Feb 11, 2014
Merged

[settings] fixed: set timezone during initialization #4095

merged 1 commit into from Feb 11, 2014

Conversation

vkosh
Copy link
Contributor

@vkosh vkosh commented Jan 27, 2014

This sets timezone during xbmc loading.
To reproduce the bug:

  1. go to settings and set timezone to something different from system one, note the time changed on home screen.
  2. restart xbmc.
  3. note that time on home screen corresponds to system timezone, but not the selected one.
  4. go to settings and reset timezone, note the time changed to selected timezone.

@t-nelson
Copy link
Contributor

Does this need to happen more generally to hit platforms besides linux?

@Montellese
Copy link
Member

Using the OnSettingUpdate() is not the right way to go. The proper way to do this is to manually set the timezone on linux during startup after having loaded the settings. That's how it works for everything else as well (resolution, gui language etc).

@t-nelson: IIRC only linux has a timezone implementation right now.

@t-nelson
Copy link
Contributor

@Montellese: OK, I couldn't remember if the other platforms diddled TZ or
not.

@vkosh: I was also questioning the implementation via the update callback.
After looking at the settings code again, I agree with @Montellese. TZ
should be set during CApp init.

@davilla
Copy link
Contributor

davilla commented Jan 27, 2014

darwin does.

@vkosh
Copy link
Contributor Author

vkosh commented Jan 27, 2014

@t-nelson: I think CApplication::Initialize is also not correct place for setting TZ, because, for example, switching between profiles with the different TZ settings will not work in this case.
The right way IMO is to implement ISettingsHandler::OnSettingsLoaded in CLinuxTimezone.
@Montellese @t-nelson: if there is no objection I'll implement the fix this way.

@t-nelson
Copy link
Contributor

As @davilla has pointed out, darwin also diddles TZ. So, CLinuxTimezone
isn't the right place, IMO.

@vkosh
Copy link
Contributor Author

vkosh commented Jan 27, 2014

@t-nelson: As I can see in CSettings, CLinuxTimezone is the only handler for locale.timezone and locale.timezonecountry settings, and it affects linux systems only.

@davilla: Do the settings really change TZ on darwin platforms?

@vkosh
Copy link
Contributor Author

vkosh commented Jan 27, 2014

Looks like it's another regression.
On Frodo we have initialization during settings loading and TZ settings should work on any "_LINUX" platform except android: see https://github.com/xbmc/xbmc/blob/Frodo/configure.in#L551 and https://github.com/xbmc/xbmc/blob/Frodo/xbmc/settings/GUISettings.cpp#L1424
On Gotham there is no initialization (subject of the PR) and "TARGET_LINUX" platforms supported only: see https://github.com/xbmc/xbmc/blob/master/m4/xbmc_arch.m4 and https://github.com/xbmc/xbmc/blob/master/xbmc/settings/Settings.cpp#L1142

So, I think the PR should be rewritten as I mentioned above. And the second PR should be created to support platforms that were supported in Frodo.

@vkosh
Copy link
Contributor Author

vkosh commented Jan 28, 2014

Here is the new patch for tz initialization. I use TARGET_LINUX as the rest CLinuxTimezone callbacks currently registered for this target only. The other platforms support will be added in another PR.

@@ -170,6 +170,17 @@ void CLinuxTimezone::OnSettingChanged(const CSetting *setting)
}
}

void CLinuxTimezone::OnSettingsLoaded()
{
CSettingString *setting = (CSettingString*)CSettings::Get().GetSetting("locale.timezone");

This comment was marked as spam.

@Montellese
Copy link
Member

Yup looks like I missed the fact that darwin also handles timezones probably because of the name of CLinuxTimezone.

@vkosh
Copy link
Contributor Author

vkosh commented Jan 28, 2014

@Montellese: thanks, fixed

@t-nelson
Copy link
Contributor

jenkins build this please

@vkosh
Copy link
Contributor Author

vkosh commented Feb 11, 2014

looks like a problem with jenkins

@t-nelson
Copy link
Contributor

jenkins build this please

@t-nelson
Copy link
Contributor

Manual droid86 build queued at http://jenkins.xbmc.org/job/XBMC-Android-X86/227/

t-nelson added a commit that referenced this pull request Feb 11, 2014
[settings] fixed: set timezone during initialization
@t-nelson t-nelson merged commit bfc296b into xbmc:master Feb 11, 2014
@MartijnKaijser MartijnKaijser modified the milestones: Gotham13.0-alpha12 January 2014, Pending for inclusion Feb 12, 2014
@koying
Copy link
Contributor

koying commented Feb 15, 2014

IIRC, TARGET_LINUX is true for android, too, and this seems to cause a regression there...
cf. http://forum.xbmc.org/showthread.php?tid=186294

@t-nelson
Copy link
Contributor

I can't repro, but I have a test build going at
http://jenkins.xbmc.org/view/Dashboard/job/XBMC-Android-ARM/263/ with some
!defined(TARGET_ANDROID)s added.

I'll post the link in that thread when it's done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants