Re: Review Request 114314: Fix traceback in Python runner plugins
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114314/#review45249 --- This review has been submitted with commit 8c5902e49b6387c3c2e82c475ff5a1f491da7604 by Harald Sitter to branch KDE/4.11. - Commit Hook On Dec. 5, 2013, 11:11 a.m., Harald Sitter wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114314/ --- (Updated Dec. 5, 2013, 11:11 a.m.) Review request for kde-workspace and KDE Bindings. Bugs: https://bugs.launchpad.net/ubuntu/+source/kde-workspace/+bug/1258088 http://bugs.kde.org/show_bug.cgi?id=https://bugs.launchpad.net/ubuntu/+source/kde-workspace/+bug/1258088 Repository: kde-workspace Description --- Plamascript.Runner is the base of python krunner plugins. These plugins implement the C++ signals prepare, teardown, createRunOptions and reloadConfiguration in actual methods (the signal wiring happens in pyrunner.py which is the loading component). As a result of this calls to any of these methods will fall through to plasmascript.Runner whenever the actual runner does not implement them. However plasmascript.Runner is missing the implicit 'self' argument such that one gets silly python backtraces like File /usr/share/kde4/apps/plasma_scriptengine_python/pyrunner.py, line 90, in reloadConfiguration self.pyrunner.reloadConfiguration() To prevent this from happening the functions now have the implicit self argument. Also see: https://bugs.launchpad.net/ubuntu/+source/kde-workspace/+bug/1258088 CCMAIL: 1258...@bugs.launchpad.net Diffs - plasma/generic/scriptengines/python/plasmascript.py 0ec38eb826cd8b7a052ed47081d05a3b644b03d1 Diff: http://git.reviewboard.kde.org/r/114314/diff/ Testing --- Simple runner plugin only implementing match, run and reloadConfiguration, each is called and no exceptions are thrown WRT attribute errors. from PyKDE4 import plasmascript from PyKDE4.plasma import Plasma from PyKDE4.kdeui import KIcon class KittehRunner(plasmascript.Runner): def match(self, context): print match def run(self, context, match): print run def reloadConfiguration(self): print reloadConfig def CreateRunner(parent): return KittehRunner(parent) Thanks, Harald Sitter
Re: Review Request 114314: Fix traceback in Python runner plugins
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114314/ --- (Updated Dec. 6, 2013, 9:12 a.m.) Status -- This change has been marked as submitted. Review request for kde-workspace and KDE Bindings. Bugs: https://bugs.launchpad.net/ubuntu/+source/kde-workspace/+bug/1258088 http://bugs.kde.org/show_bug.cgi?id=https://bugs.launchpad.net/ubuntu/+source/kde-workspace/+bug/1258088 Repository: kde-workspace Description --- Plamascript.Runner is the base of python krunner plugins. These plugins implement the C++ signals prepare, teardown, createRunOptions and reloadConfiguration in actual methods (the signal wiring happens in pyrunner.py which is the loading component). As a result of this calls to any of these methods will fall through to plasmascript.Runner whenever the actual runner does not implement them. However plasmascript.Runner is missing the implicit 'self' argument such that one gets silly python backtraces like File /usr/share/kde4/apps/plasma_scriptengine_python/pyrunner.py, line 90, in reloadConfiguration self.pyrunner.reloadConfiguration() To prevent this from happening the functions now have the implicit self argument. Also see: https://bugs.launchpad.net/ubuntu/+source/kde-workspace/+bug/1258088 CCMAIL: 1258...@bugs.launchpad.net Diffs - plasma/generic/scriptengines/python/plasmascript.py 0ec38eb826cd8b7a052ed47081d05a3b644b03d1 Diff: http://git.reviewboard.kde.org/r/114314/diff/ Testing --- Simple runner plugin only implementing match, run and reloadConfiguration, each is called and no exceptions are thrown WRT attribute errors. from PyKDE4 import plasmascript from PyKDE4.plasma import Plasma from PyKDE4.kdeui import KIcon class KittehRunner(plasmascript.Runner): def match(self, context): print match def run(self, context, match): print run def reloadConfiguration(self): print reloadConfig def CreateRunner(parent): return KittehRunner(parent) Thanks, Harald Sitter
Review Request 114328: re-add customstyleelement suite to kstyle
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114328/ --- Review request for kdelibs. Repository: kdelibs Description --- re-add customstyleelement suite to kstyle Diffs - tier4/frameworkintegration/src/kstyle/kstyle.h 8a881f5 tier4/frameworkintegration/src/kstyle/kstyle.cpp 27d407e Diff: http://git.reviewboard.kde.org/r/114328/diff/ Testing --- compiles, works, fix kde-workspace build also: will be used when moving oxygen from qcommonstyle back to kstyle (right now we have a fork of some of the said methods) Thanks, Hugo Pereira Da Costa
Re: Review Request 114321: Fix timezone saving in System Settings - Date Time
On Dec. 6, 2013, 1: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? This step is not needed at all On Dec. 6, 2013, 1: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? No issue, the /etc/timezone handling is still there, a few lines below On Dec. 6, 2013, 1: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? 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 On Dec. 6, 2013, 1: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? Hmm, maybe, but I think utf8 is safer here - Lukáš --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114321/#review45241 --- On Dec. 6, 2013, 12:24 a.m., Lukáš Tinkl wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114321/ --- (Updated Dec. 6, 2013, 12:24 a.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
Re: Review Request 114321: Fix timezone saving in System Settings - Date Time
On Dec. 6, 2013, 12:52 a.m., Martin Klapetek wrote: Note that Debian-based systems actually do copy the file rather than symlink - main reason being that if you use a symlink and your /usr is mounted on a separate partition, anything that starts before /usr gets mounted will not have the correct timezone. John Layt wrote: And it's not just that... Old versions of Red Hat also copied the file, and Debian stores the current tz name in /etc/timezone, and Red Hat and openSUSE store it in /etc/sysconfig/clock. Some older distros have the tz files in /usr/lib/zoneinfo instead of /usr/share/zoneinfo. And who knows what else other distros do! This code scares me (the old code especially so, why is it calling zic?), we're making all sorts of assumptions about the underlying system that we can't be sure of and changing things that could break the users system. Personally, I think the distros need to take care of this side of things by providing a script for us to call. I think most distros do have different command line tools to do it, but finding and maintaining them all would be a nightmare. Perhaps we need to install and call a script kde-set-timezone which the distros can then modify to call their own tools? H, I almost find myself wishing systemd would take care of all this for us :-) Jan Kundrát wrote: You mean http://www.freedesktop.org/wiki/Software/systemd/timedated/ , right? John Layt wrote: Yes, but having looked at that before it is also broken in exactly the same way. It's obvious it has never been properly integrated on a Debian system. It needs changing to allow for distro specific config scripts for setting the tz. It could also use some extra calls added like we have in ktimezoned. Yeah I tried using timedated as well but it fails spectacularly to update the Timezone property when you change it from outside (like on RHEL/Fedora) using system-config-date. This way it works using all the testcases I tried - Lukáš --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114321/#review45240 --- On Dec. 6, 2013, 12:24 a.m., Lukáš Tinkl wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114321/ --- (Updated Dec. 6, 2013, 12:24 a.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
Re: Review Request 114321: Fix timezone saving in System Settings - Date Time
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
