Review Request 114321: Fix timezone saving in System Settings -> Date & Time

[Thomas Lübking]

> On Dec. 6, 2013, 12:22 a.m., Thomas Lübking wrote:
> > kcontrol/dateandtime/helper.cpp, line 189
> > <http://git.reviewboard.kde.org/r/114321/diff/1/?file=222931#file222931line189>
> >
> >     assuming zic is not available and the fallback is required, this should maybe inspect the type of the present localtime (if) and link/copy the new timezone respectively?
> 
> Lukáš Tinkl wrote:
>     No issue, the /etc/timezone handling is still there, a few lines below

I meant handling of the symlink ./. inode type of /etc/localtime - nothing at all about /etc/timezone
See comments by Martin and John.


> On Dec. 6, 2013, 12:22 a.m., Thomas Lübking wrote:
> > kcontrol/dateandtime/helper.cpp, line 204
> > <http://git.reviewboard.kde.org/r/114321/diff/1/?file=222931#file222931line204>
> >
> >     this looks like it'll break compilation on solaris?
> 
> Lukáš Tinkl wrote:
>     I don't think so, previously the code was wrong:
>     
>     QString val = ':' + tz;
>     #endif // !USE_SOLARIS
>     setenv("TZ", val.toAscii(), 1);
>     
>     so it would be setting the TZ from an uninit value on Solaris, I don't know how this could ever compile

There're now two "QString val" declarations #if defined(USE_SOLARIS) (the other one is right above the #else)

So let me rephrase it:
It *will* break compilation on Solaris - for sure.


> On Dec. 6, 2013, 12:22 a.m., Thomas Lübking wrote:
> > kcontrol/dateandtime/helper.cpp, line 206
> > <http://git.reviewboard.kde.org/r/114321/diff/1/?file=222931#file222931line206>
> >
> >     iff ASCII is wrong, this should be rather ::toLocal8Bit(), yesno?
> 
> Lukáš Tinkl wrote:
>     Hmm, maybe, but I think utf8 is safer here

And you base that "think" upon what?
- Either the names (and so it seems) are by contract limited to ASII
  Then using ASCII or UTF-8 is equal, and i'd tend to use ASCII to stress that limitation
- Or it's not and then the correct format severely matters or the running process will fail to resolve the filename.

Seriously:
i18n and timehandling suck badly and if you intend to bring a "dunno why, but works for me" patch, I cannot support that at all, sorry.


> On Dec. 6, 2013, 12:22 a.m., Thomas Lübking wrote:
> > kcontrol/dateandtime/helper.cpp, line 180
> > <http://git.reviewboard.kde.org/r/114321/diff/1/?file=222931#file222931line180>
> >
> >     What's the problem about using zic?
> >     If zic is broken (distro related?) zic should be fixed, yesno?
> 
> Lukáš Tinkl wrote:
>     This step is not needed at all

Because?
Do you know it's not there, because distros handle(d) their specifics through zic patches?

Eg. plain "zic -l" prefers a hardlink and resorts to a symlink only. Eventually coyping as last resort (not checked)

I will admit that i cannot say whether that's good or bad, but if you want to change it, i suggest to reason that by some deeper knowledge.


- Thomas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/114321/#review45241
-----------------------------------------------------------


On Dec. 5, 2013, 11:24 p.m., Lukáš Tinkl wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/114321/
> -----------------------------------------------------------
> 
> (Updated Dec. 5, 2013, 11:24 p.m.)
> 
> 
> Review request for kde-workspace.
> 
> 
> Bugs: 159171 and 323511
>     http://bugs.kde.org/show_bug.cgi?id=159171
>     http://bugs.kde.org/show_bug.cgi?id=323511
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> -------
> 
> - fix saving/loading of timezones in kcmclock
> - do not mark the module as changed right after the new timezone gets loaded back
> 
> Besides the above mentioned bugs, it also fixes https://bugzilla.redhat.com/show_bug.cgi?id=990146
> 
> 
> Diffs
> -----
> 
>   kcontrol/dateandtime/dtime.cpp 518afe5 
>   kcontrol/dateandtime/helper.cpp 9168db3 
>   kcontrol/dateandtime/main.cpp 2fa0f3e 
> 
> Diff: http://git.reviewboard.kde.org/r/114321/diff/
> 
> 
> Testing
> -------
> 
> In the past, this would for some reason only work sporadically and make ktimezoned utterly confused; now it correctly sets a symlink (instead of copying the file over) from /etc/localtime to the respective file under /usr/share/zoneinfo (as described in "man tzset"). The symlink points to the right location after each save. Launching the module again, it shows the correct timezone, as previously saved. 
> 
> 
> Thanks,
> 
> Lukáš Tinkl
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20131206/3cb5b9fa/attachment.htm>