Re: Review Request 111272: Correctly handle executable names typed into KOpenWithDialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111272/#review35416 --- kdecore/services/kservice.h http://git.reviewboard.kde.org/r/111272/#comment25945 How about a setExec() method rather than a special constructor? This seems a lot clearer to me (and e.g. easier to grep for in order to find usages, etc.). But anyway, I have my doubts that this really works: returning a KService that isn't exactly like the one in ksycoca seems fragile to me. E.g. when krun uses that service, it will call KToolInvocation with the entry path to the service - i.e. just the path to the desktop file. The modified Exec field (in memory) won't be used in that case. What KRun can do, though, is work with a temp kservice, created like this: KService::Ptr service(new KService(_name, _exec, _icon)); This is detected as not in ksycoca and therefore not given to KToolInvocation (but to runTempService). I think this would recreate the issue you saw though, since indeed nowadays it's not enough to have %U in the exec line, one must also have Categories or X-KDE-Protocols indicating that KIO protocols are supported. But this could be copied from the orig file, using a new setter in KService. Hmm. Maybe it would be simpler to just clear entryPath() in the KService::setExec() I'm suggesting initially. i.e.: if we're modifying the KService then the path to the file that has been cached in ksycoca, no longer applies. And then KRun will go to runTempService, when running it. kio/kfile/kopenwithdialog.cpp http://git.reviewboard.kde.org/r/111272/#comment25944 this comment should move with the code that it affected - David Faure On July 2, 2013, 2:41 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111272/ --- (Updated July 2, 2013, 2:41 a.m.) Review request for kdelibs and David Faure. Description --- The attached patch addresses a bug where a user enters the name of a KDE application in OpenWith dialog to open a remote file and the file is opened as if the user requested to open it with a non KDE application. That is a local copy of the file is created first. Currently this problem can be reproduced with kate because the Exec= line in its desktop file contains an additional option, -b. Note that this patch only addresses the specific condition where the user only typed in the KDE executable name. Other scenarios, like the user typing in not only the name of the KDE app but also additional command line options, will still produce this same issue. This addresses bug 222519. http://bugs.kde.org/show_bug.cgi?id=222519 Diffs - kdecore/services/kservice.h 3843bad kdecore/services/kservice.cpp e2cc86f kio/kfile/kopenwithdialog.cpp 84465cd Diff: http://git.reviewboard.kde.org/r/111272/diff/ Testing --- Try to open a remote text or source file by typing kate in the open with dialog. Thanks, Dawit Alemayehu
Re: Review Request 111171: Deprecate (in)active(Title/Text)Color in favor of KColorScheme
On July 1, 2013, 2:39 p.m., Kevin Ottens wrote: kdeui/util/kglobalsettings.h, line 265 http://git.reviewboard.kde.org/r/71/diff/2/?file=166657#file166657line265 I think it's fine. lxr.kde.org seem to indicate that those colors got abused at some places anyway, and what's proposed here seems like a proper replacement to me (except if you're kwin of course, but I think the people behind it know that by now). Thomas Lübking wrote: @deprecated since 5.0, This used to export the color of the titlebar. If you wanted to query the color of the window background, use KColorScheme(QPalette::Active, KColorScheme::Window).foreground(KColorScheme::ActiveText).color(), otherwise please read the setting from where it's moved directly? where it's moved would be kwin Yes, sounds good to me. On July 1, 2013, 2:39 p.m., Kevin Ottens wrote: khtml/misc/helper.cpp, line 160 http://git.reviewboard.kde.org/r/71/diff/2/?file=166658#file166658line160 Why unrelated? It's removing calls to the newly deprecated API from KHTML (one of the abusers from my previous comment). Could be made in a separate commit indeed, but for review purpose it seems relevant to the topic for me. Thomas Lübking wrote: The function so far handed the color of the titlebar, now it hands the color of the window. a) this has been wrong so far (as you suggest): then this fixes an unrelated present bug - i personally don't mid too much, but it's still an unrelated bug ;P b) this has been correct so far: then this introduces a behavioral change, implying titlebar color == window color, which is either intentionally unrelated (only relation being the opportunity) or unintentionally, ie. the change in reaction to the deprecation is wrong and accidentally alters behavior. To recap my worries and position on this topic: If there shall be a global alignment of titlebar color == window background color (with the freedom of single decorations to deviate, of course) I want that decision explicitly *taken* and not somehow implied - to avoid any sequel to that oxygen/ozone kindergarten session. I would be (a) in this case. So yes, would have been better as a separate commit, but that's no big deal IMO. - Kevin --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/71/#review35358 --- On June 28, 2013, 5:10 p.m., Àlex Fiestas wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/71/ --- (Updated June 28, 2013, 5:10 p.m.) Review request for KDE Frameworks and kdelibs. Description --- Deprecate: inactiveTitleColor, inactiveTextColor, activeTitleColor, activeTextColor in favor of KColorScheme and replace the implementation of those methods with it. Diffs - kdeui/util/kglobalsettings.h 4b77ed5 khtml/misc/helper.cpp dccb9bf Diff: http://git.reviewboard.kde.org/r/71/diff/ Testing --- I have compared the colors returned by the methods before and after this patch, they are close enough. Additionally used some apps like filelight with the change, and it seems to work for them as well. Thanks, Àlex Fiestas
Re: Review Request 111345: KParts::StatusBarExtension: Remember activated state
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111345/#review35426 --- Ship it! Makes sense. - David Faure On July 1, 2013, 4:01 p.m., Aurélien Gâteau wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111345/ --- (Updated July 1, 2013, 4:01 p.m.) Review request for kdelibs and David Faure. Description --- When the StatusBarExtension receives a GUIActivateEvent, it shows or hides its statusbar items accordingly, but it does not remember it: if an item is added after receiving a GUIActivateEvent(false), the item will be shown. This patch adds a bool m_activated field to keep track of the state and take it into account when a new item is added. This change is needed for an Akregator fix: https://git.reviewboard.kde.org/r/111346/ . Without the patch, when Akregator restores its session at startup, the statubar items for all background tabs appear. Diffs - kparts/statusbarextension.cpp db7e4197dd42497f91c66d1654e9b1c8cc45d793 Diff: http://git.reviewboard.kde.org/r/111345/diff/ Testing --- Akregator startup: statusbar items do not appear when the main tab is focused and other tabs are loaded. Statusbar items do appear when the corresponding tab is focused. Konqueror: played around with tabs and splitted views, did not notice any behavior change in the statusbar. Thanks, Aurélien Gâteau
Fwd: Re: KIO progress towards tier 1 framework?
I suspect a lot of people read both, but it's probably better to keep frameworks discussions on the frameworks list as much as possible. Forwarding there now. Alex Original Message Subject: Re: KIO progress towards tier 1 framework? Date: Tue, 2 Jul 2013 13:05:25 +0200 From: Mark mark...@gmail.com To: kdelibs kde-core-devel@kde.org Ahh, i just noticed that i mailed this one to k-c-d instead of the frameworks list. Will this one do? On Mon, Jul 1, 2013 at 9:03 PM, Mark mark...@gmail.com wrote: Hi, When i'm looking in the kdelibs splitting document [1] i only see KIO pop up twice. One to get kbookmarks out of KIO (which is work in progress). Another one with kio-core as work in progress. But i can't find any documentation anywhere that gives me a clear view as to how far KIO is on it's way to a tier 1 framework. The meeting notes don't say anything either (besides david making progress on the kio split). I'm asking this because i'm getting more and more interested in KIO. I want to get it running on windows, linux and mac as tier 1 framework. Or more specifically, a Qt module. Starting from August 1st, I'll be trying to get that done so i'm really keen on knowing how far KIO is at this moment in reaching that goal and what needs to be done. I don't know it's current state, but my interest is there (has been there for a few years now) and if more people are interested then i'd even like to propose a small KIO Sprint somewhere in August and somewhere in Europe. Before the first reply is attend akademy.. I wish i could. My current job is quite demanding due to a few very tight deadlines so i simply don't have the time this month. I will next month because of transition time between jobs. Someone wise once told me: You should dive into KIO, it can be really rewarding! (looking at you, David ^_-) Cheers, Mark [1] http://community.kde.org/Frameworks/Epics/Splitting_kdelibs
Re: Review Request 111272: Correctly handle executable names typed into KOpenWithDialog
On July 2, 2013, 7:48 a.m., David Faure wrote: kdecore/services/kservice.h, line 82 http://git.reviewboard.kde.org/r/111272/diff/2/?file=167013#file167013line82 How about a setExec() method rather than a special constructor? This seems a lot clearer to me (and e.g. easier to grep for in order to find usages, etc.). But anyway, I have my doubts that this really works: returning a KService that isn't exactly like the one in ksycoca seems fragile to me. E.g. when krun uses that service, it will call KToolInvocation with the entry path to the service - i.e. just the path to the desktop file. The modified Exec field (in memory) won't be used in that case. What KRun can do, though, is work with a temp kservice, created like this: KService::Ptr service(new KService(_name, _exec, _icon)); This is detected as not in ksycoca and therefore not given to KToolInvocation (but to runTempService). I think this would recreate the issue you saw though, since indeed nowadays it's not enough to have %U in the exec line, one must also have Categories or X-KDE-Protocols indicating that KIO protocols are supported. But this could be copied from the orig file, using a new setter in KService. Hmm. Maybe it would be simpler to just clear entryPath() in the KService::setExec() I'm suggesting initially. i.e.: if we're modifying the KService then the path to the file that has been cached in ksycoca, no longer applies. And then KRun will go to runTempService, when running it. Well I created a special constructor instead of a setter because I wanted to limit the scope of change to KService's behavior as low as possible. If I added a setExec, it would allow the Exec= to be changed at anytime regardless of how KService was created. To me that did not make sense as the need to change the Exec= line is very specific scenarios like the one we have in the openWith dialog. Moreover, we already have several different ctors for different use cases right now. Adding one that could be merged with an existing on in KF5 would be the lowest impact change that could be made to KService itself. Are you sure adding a setExec() is the better way to go in this case? - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111272/#review35416 --- On July 2, 2013, 2:41 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111272/ --- (Updated July 2, 2013, 2:41 a.m.) Review request for kdelibs and David Faure. Description --- The attached patch addresses a bug where a user enters the name of a KDE application in OpenWith dialog to open a remote file and the file is opened as if the user requested to open it with a non KDE application. That is a local copy of the file is created first. Currently this problem can be reproduced with kate because the Exec= line in its desktop file contains an additional option, -b. Note that this patch only addresses the specific condition where the user only typed in the KDE executable name. Other scenarios, like the user typing in not only the name of the KDE app but also additional command line options, will still produce this same issue. This addresses bug 222519. http://bugs.kde.org/show_bug.cgi?id=222519 Diffs - kdecore/services/kservice.h 3843bad kdecore/services/kservice.cpp e2cc86f kio/kfile/kopenwithdialog.cpp 84465cd Diff: http://git.reviewboard.kde.org/r/111272/diff/ Testing --- Try to open a remote text or source file by typing kate in the open with dialog. Thanks, Dawit Alemayehu
Re: Review Request 111345: KParts::StatusBarExtension: Remember activated state
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111345/#review35444 --- This review has been submitted with commit 4c3b5a452b4090019852d1691ad91f12cf91d362 by Aurélien Gâteau to branch master. - Commit Hook On July 1, 2013, 4:01 p.m., Aurélien Gâteau wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111345/ --- (Updated July 1, 2013, 4:01 p.m.) Review request for kdelibs and David Faure. Description --- When the StatusBarExtension receives a GUIActivateEvent, it shows or hides its statusbar items accordingly, but it does not remember it: if an item is added after receiving a GUIActivateEvent(false), the item will be shown. This patch adds a bool m_activated field to keep track of the state and take it into account when a new item is added. This change is needed for an Akregator fix: https://git.reviewboard.kde.org/r/111346/ . Without the patch, when Akregator restores its session at startup, the statubar items for all background tabs appear. Diffs - kparts/statusbarextension.cpp db7e4197dd42497f91c66d1654e9b1c8cc45d793 Diff: http://git.reviewboard.kde.org/r/111345/diff/ Testing --- Akregator startup: statusbar items do not appear when the main tab is focused and other tabs are loaded. Statusbar items do appear when the corresponding tab is focused. Konqueror: played around with tabs and splitted views, did not notice any behavior change in the statusbar. Thanks, Aurélien Gâteau
Re: Review Request 111345: KParts::StatusBarExtension: Remember activated state
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111345/ --- (Updated July 2, 2013, 2:05 p.m.) Status -- This change has been marked as submitted. Review request for kdelibs and David Faure. Description --- When the StatusBarExtension receives a GUIActivateEvent, it shows or hides its statusbar items accordingly, but it does not remember it: if an item is added after receiving a GUIActivateEvent(false), the item will be shown. This patch adds a bool m_activated field to keep track of the state and take it into account when a new item is added. This change is needed for an Akregator fix: https://git.reviewboard.kde.org/r/111346/ . Without the patch, when Akregator restores its session at startup, the statubar items for all background tabs appear. Diffs - kparts/statusbarextension.cpp db7e4197dd42497f91c66d1654e9b1c8cc45d793 Diff: http://git.reviewboard.kde.org/r/111345/diff/ Testing --- Akregator startup: statusbar items do not appear when the main tab is focused and other tabs are loaded. Statusbar items do appear when the corresponding tab is focused. Konqueror: played around with tabs and splitted views, did not notice any behavior change in the statusbar. Thanks, Aurélien Gâteau
KF5 Update Meeting Minutes 2013-w27
Hello everyone, This is the minutes of the Week 27 KF5 meeting. As usual it has been held on #kde-devel at 4pm Paris time. Were present: apol, ben2367, dfaure, R_Rios, sandsmark, scarpino, sebas, wojtask9 and myself. Topics discussed: * QFontsDatabase patch is getting ready; * Support for ctrl+enter for the OK button in Qt dialog is blocked for now, apol is waiting for Lars to take position to decide how to deal with it; * Progress on the KGlobalSettings tasks by apol; * KCrash framework is in the making by ben2367; * Still work on going with QUrl by dfaure with thiago on the review end; * KIO splitting resumed; * help:/ support for QDesktopServices to replace invokeHelp; * KPtyProcess identified problem might not be a regression, it shows up on 4.10, to be confirmed; * Buildsystem cleanup in plasma-framework is done; * KPluginFactory work is making progresses again thanks to sebas (sebas/kplugins branches); * http://wstaw.org/m/2013/07/02/plasma-desktopDY2308.png * Plasma::DataEngine resurrection is in progress; * wojtask9 is looking into kacceleratormanager move; * he also plans to port kkeymodifierkeyinfo to xcb; * scarpino is making progress on splitting XMLGUI; * Tests are green again in kdelibs-frameworks; * Still cleaning up the remains of kdecore; * KUrl is in kde4support now; * KUser and KShell moved in kcoreaddons; * Patches to Qt for supporting RFCDate and FancyDate formats of KDateTime are in the work; * Priority for the coming week is still the KDEUI Crumble. Action items still open: * [sandsmark] Finish the work on KPtyProcess * [steveire] Write a CMake for frameworks guideline in the wiki * [steveire] investigate why kde4support isn't usable from outside kdelibs (issue with the cmake file) If you got questions, feel free to ask. Regards. -- Kévin Ottens, http://ervin.ipsquad.net Sponsored by BlueSystems and KDAB to work on KDE Frameworks signature.asc Description: This is a digitally signed message part.
Re: Review Request 111370: system_tray: initialize uninitialized members
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111370/ --- (Updated July 2, 2013, 6:15 p.m.) Review request for kde-workspace and Kai Uwe Broulik. Description --- A member in WidgetItem is used unitialized. Fix this. See https://bugs.kde.org/show_bug.cgi?id=321828 Diffs - plasma/generic/applets/systemtray/ui/widgetitem.cpp 1f922c921c0f3763e8ffeb4466fbffb527fca2f2 Diff: http://git.reviewboard.kde.org/r/111370/diff/ Testing --- Thanks, Jiri Slaby
Review Request 111371: Fix duplicate targets in PythonMacros.cmake, unbreaking Ninja builds for Python-based KDE apps
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111371/ --- Review request for kdelibs. Description --- Currently, PYTHON_INSTALL macro in PythonMacros.cmake creates same target compile_python_files for all Python modules. This somehow works for Make-based builds, but breaks when using Ninja backend for CMake. CMake itself says that it does not guarantee that targets with same name will ever work. The patch creates unique target for each python module being pre-compiled and installed. The patch can be applied without modifications to 4.10 and 4.11, too. Diffs - cmake/modules/PythonMacros.cmake 95c9315 Diff: http://git.reviewboard.kde.org/r/111371/diff/ Testing --- OpenBSD-CURRENT, PyKDE 4.10.4 and 4.10.5. Thanks, Vadim Zhukov
Re: KF5 Update Meeting Minutes 2013-w27
On Tuesday 02 July 2013, Kevin Ottens wrote: Hello everyone, This is the minutes of the Week 27 KF5 meeting. As usual it has been held on #kde-devel at 4pm Paris time. Were present: apol, ben2367, dfaure, R_Rios, sandsmark, scarpino, sebas, wojtask9 and myself. ... * [steveire] investigate why kde4support isn't usable from outside kdelibs (issue with the cmake file) what's the issue here ? Without looking in detail, this will be a mess while we have the mixture of already-split and still-monolithic libraries. Alex
Re: KF5 Update Meeting Minutes 2013-w27
On Tuesday 02 July 2013, Kevin Ottens wrote: Hello everyone, This is the minutes of the Week 27 KF5 meeting. As usual it has been held on #kde-devel at 4pm Paris time. Were present: apol, ben2367, dfaure, R_Rios, sandsmark, scarpino, sebas, wojtask9 and myself. Topics discussed: * QFontsDatabase patch is getting ready; * Support for ctrl+enter for the OK button in Qt dialog is blocked for now, apol is waiting for Lars to take position to decide how to deal with it; * Progress on the KGlobalSettings tasks by apol; * KCrash framework is in the making by ben2367; * Still work on going with QUrl by dfaure with thiago on the review end; * KIO splitting resumed; * help:/ support for QDesktopServices to replace invokeHelp; * KPtyProcess identified problem might not be a regression, it shows up on 4.10, to be confirmed; * Buildsystem cleanup in plasma-framework is done; * KPluginFactory work is making progresses again thanks to sebas (sebas/kplugins branches); * http://wstaw.org/m/2013/07/02/plasma-desktopDY2308.png * Plasma::DataEngine resurrection is in progress; * wojtask9 is looking into kacceleratormanager move; * he also plans to port kkeymodifierkeyinfo to xcb; * scarpino is making progress on splitting XMLGUI; * Tests are green again in kdelibs-frameworks; * Still cleaning up the remains of kdecore; * KUrl is in kde4support now; * KUser and KShell moved in kcoreaddons; * Patches to Qt for supporting RFCDate and FancyDate formats of KDateTime are in the work; * Priority for the coming week is still the KDEUI Crumble. Another question: the libraries as they are now in tier1/ and staging/ are partly really tiny. kinterprocesswindowing consists of 2 cpp files, kplotting is 4 cpp files. Isn't that a bit too fine grained ? E.g. kplotting and kwidgets have the same dependencies. I wouldn't mind having kplotting as part of kwidgets. Alex
Re: Review Request 110285: Fixed warning message when user trying to set more than 8 keyboard layouts
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110285/#review35479 --- This review has been submitted with commit 7c599bae516c61365c4eda2be998c97594e3b1af by Victor Polevoy to branch master. - Commit Hook On May 3, 2013, 11 a.m., Victor Polevoy wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110285/ --- (Updated May 3, 2013, 11 a.m.) Review request for kde-workspace. Description --- If user trying to set more than 8 keyboard layouts in System Settings/Input devices/Keyboard layouts he sees a warning that he can't set more than 4 keyboard layouts but there should be 8. My patch fixes typo in the warning message. This addresses bug 285562. http://bugs.kde.org/show_bug.cgi?id=285562 Diffs - kcontrol/keyboard/kcm_keyboard_widget.cpp e513a41 kcontrol/keyboard/x11_helper.h 31f76f9 kcontrol/keyboard/x11_helper.cpp ebd09f3 Diff: http://git.reviewboard.kde.org/r/110285/diff/ Testing --- Yes, it works for me. Thanks, Victor Polevoy
Re: Review Request 110285: Fixed warning message when user trying to set more than 8 keyboard layouts
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110285/ --- (Updated July 2, 2013, 8:42 p.m.) Status -- This change has been marked as submitted. Review request for kde-workspace. Description --- If user trying to set more than 8 keyboard layouts in System Settings/Input devices/Keyboard layouts he sees a warning that he can't set more than 4 keyboard layouts but there should be 8. My patch fixes typo in the warning message. This addresses bug 285562. http://bugs.kde.org/show_bug.cgi?id=285562 Diffs - kcontrol/keyboard/kcm_keyboard_widget.cpp e513a41 kcontrol/keyboard/x11_helper.h 31f76f9 kcontrol/keyboard/x11_helper.cpp ebd09f3 Diff: http://git.reviewboard.kde.org/r/110285/diff/ Testing --- Yes, it works for me. Thanks, Victor Polevoy
Re: Review Request 111272: Correctly handle executable names typed into KOpenWithDialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111272/ --- (Updated July 3, 2013, 2:22 a.m.) Review request for kdelibs and David Faure. Changes --- Updated the previous iteration of the patch to - Remove the entryPath() from the custom KService we create so as not to confuse KRun. - If the typed executable does not contain a %U or %F, but the matching service does, append those options so that KRun won't end up using kioexec. Description --- The attached patch addresses a bug where a user enters the name of a KDE application in OpenWith dialog to open a remote file and the file is opened as if the user requested to open it with a non KDE application. That is a local copy of the file is created first. Currently this problem can be reproduced with kate because the Exec= line in its desktop file contains an additional option, -b. Note that this patch only addresses the specific condition where the user only typed in the KDE executable name. Other scenarios, like the user typing in not only the name of the KDE app but also additional command line options, will still produce this same issue. This addresses bug 222519. http://bugs.kde.org/show_bug.cgi?id=222519 Diffs (updated) - kdecore/services/kservice.h 3843bad kdecore/services/kservice.cpp e2cc86f kio/kfile/kopenwithdialog.cpp 84465cd Diff: http://git.reviewboard.kde.org/r/111272/diff/ Testing --- Try to open a remote text or source file by typing kate in the open with dialog. Thanks, Dawit Alemayehu
