Re: Kdelibs Coding Style vs. preparations for Qt5
Hello, On Saturday 29 December 2012 02:47:13 Friedrich W. H. Kossebau wrote: --- 8 --- Qt Includes If you add #includes for Qt classes, use both the module and class name. This allows library code to be used by applications without excessive compiler include paths. Example: // wrong #include QString // correct #include QtCore/QString --- 8 --- From http://techbase.kde.org/Policies/Kdelibs_Coding_Style#Qt_Includes Since I've been skimming quite a bit through kdelibs codebase recently, needless to say this rule is in practice void as it's clearly not followed. Of course the current Qt Coding Style, which is the base of the kdelibs one, does not mention anything about that, given that its about their own headers. Yep. Now the Porting from Qt 4 to Qt 5 guide from KDAB recommends this: --- 8 --- Fixing up includes [...] Or more portably (Which works in Qt 4 and Qt 5): #include QWidget --- 8 --- From http://www.kdab.com/porting-from-qt-4-to-qt-5/ So what to do about this? I *personally* prefer the fully qualified style with the module name. That said, since we don't have per module namespaces to go with it and the like it's rather cosmetic more than anything. Now add to that the fact that the fully qualified style will generate more work when you port (for classes moving from one module to another) it sounds like it's the right move to drop them completely and use only the class name for includes[*]. Will kde-frameworks be Qt5-only, so not need to support both Qt4 and Qt5 and thus to use module-less Qt includes? For now it supports both, but it's very likely that by the end of january it will be Qt5 only. Or will the includes be if-def'ed? So will projects which refer to the Kdelibs Coding Style need to add an exception to their rules for the includes, if they want to prepare for Qt5? Or does the rule need adaption? With what I wrote above I think the natural conclusion is to adapt the rule. We should push to use the class name only includes I think. Also, contributing to Qt itself more, I noticed some differences in style between both (namely: we ignore space around operators, and we for braces around single line statements). Your question makes me wonder if we should update our own KDE Frameworks style to be closer of the Qt one. The aim here would be to be consistent accross the whole stack. Actually looking at both documents now, I see we should already follow the spacing around operators today... I think the intent of our coding style was to be the Qt style with extra exceptions but somewhere in the wording this gets lost. Maybe we should do a better job at it... If we go toward being closer to the Qt style maybe the proper way out would be to shorten the document quite a bit saying: use the Qt style + includes section + astyle section + emacs vim section. Right now by duplicating some but not all of the Qt style we're diluting the messaging it seems. Opinions? Thanks for bringing it up though, it's definitely the right time for that. Regards. PS: Please, before hitting reply think twice! This type of thread can easily degenerate in bikeshedding. So put your personal preferences at the door as I tried to, it's really not the point. PPS: My PS might sound obvious and unneeded to some of you, but I got burnt enough times with coding styles discussions to be extra cautious now. It's really easy to loose perspective and I might end up doing the mistake myself, the reminder is for me too. :-) [*] Note that, History might prove me wrong there at some point if Qt6 introduces classes with the same name in different modules... if that's the case I hope it'll come with consistent namespacing though. :-) -- Kévin Ottens, http://ervin.ipsquad.net KDAB - proud patron of KDE, http://www.kdab.com signature.asc Description: This is a digitally signed message part.
Re: Kdelibs Coding Style vs. preparations for Qt5
Kevin Ottens wrote: We should push to use the class name only includes I think. I agree. We have a buildsystem that is good enough that we can specify the directories to look for the 'class name' headers in, and it is in the buildsystem that that stuff belongs. See the kinds of practical problems 'module includes' create: http://thread.gmane.org/gmane.comp.programming.tools.cmake.user/44780/focus=44893 Thanks, Steve.
Re: Kdelibs Coding Style vs. preparations for Qt5
On Saturday 29 December 2012 02:47:13 Friedrich W. H. Kossebau wrote: Will kde-frameworks be Qt5-only, so not need to support both Qt4 and Qt5 and thus to use module-less Qt includes? At some point it will be, yes. Right now it supports both, so a bunch of QtGui/ was removed in #include statements. So will projects which refer to the Kdelibs Coding Style need to add an exception to their rules for the includes, if they want to prepare for Qt5? Or does the rule need adaption? Well, for frameworks that intend to be as close to Qt as possible they should do the same (for the convenience of developers who don't use qmake/cmake but set up their project configuration in their IDE by hand, for instance Visual Studio). This means re-adding the missing QtWidgets/ in public headers once Qt5 is required in KF5. But for sure this doesn't apply to projects which refer to the kdelibs coding style. There's no good reason for apps to do this, only porting trouble comes from that. -- David Faure, fa...@kde.org, http://www.davidfaure.fr Working on KDE, in particular KDE Frameworks 5
Review Request: Port kdelibs to feature_summary
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108002/ --- Review request for Build System, kdelibs and Alexander Neundorf. Description --- This patch replaces all the macro_log_feature entries with feature_summary. Diffs - CMakeLists.txt 0030b30 dnssd/CMakeLists.txt 4a44acf kdecore/CMakeLists.txt b147f22 kdecore/auth/ConfigureChecks.cmake ff17ff0 kdecore/compression/ConfigureChecks.cmake 8edd173 kdeui/tests/proxymodeltestsuite/CMakeLists.txt 0c9a129 kdewebkit/CMakeLists.txt 21b2c5c kdoctools/CMakeLists.txt 26a9231 khtml/CMakeLists.txt 99034cc kimgio/CMakeLists.txt 26329c0 kioslave/http/CMakeLists.txt 95ed9cf kjs/CMakeLists.txt ef3c5e8 mimetypes/CMakeLists.txt 003951e solid/solid/CMakeLists.txt b7dcc97 sonnet/plugins/CMakeLists.txt 9d69005 Diff: http://git.reviewboard.kde.org/r/108002/diff/ Testing --- No issue detected. CMake detects the same dependencies as before. Thanks, Christophe Giboudeaux
Re: Kdelibs Coding Style vs. preparations for Qt5
El Dissabte, 29 de desembre de 2012, a les 10:48:48, Kevin Ottens va escriure: Hello, On Saturday 29 December 2012 02:47:13 Friedrich W. H. Kossebau wrote: --- 8 --- Qt Includes If you add #includes for Qt classes, use both the module and class name. This allows library code to be used by applications without excessive compiler include paths. Example: // wrong #include QString // correct #include QtCore/QString --- 8 --- From http://techbase.kde.org/Policies/Kdelibs_Coding_Style#Qt_Includes Since I've been skimming quite a bit through kdelibs codebase recently, needless to say this rule is in practice void as it's clearly not followed. Of course the current Qt Coding Style, which is the base of the kdelibs one, does not mention anything about that, given that its about their own headers. Yep. Now the Porting from Qt 4 to Qt 5 guide from KDAB recommends this: --- 8 --- Fixing up includes [...] Or more portably (Which works in Qt 4 and Qt 5): #include QWidget --- 8 --- From http://www.kdab.com/porting-from-qt-4-to-qt-5/ So what to do about this? I *personally* prefer the fully qualified style with the module name. That said, since we don't have per module namespaces to go with it and the like it's rather cosmetic more than anything. Now add to that the fact that the fully qualified style will generate more work when you port (for classes moving from one module to another) it sounds like it's the right move to drop them completely and use only the class name for includes[*]. Will kde-frameworks be Qt5-only, so not need to support both Qt4 and Qt5 and thus to use module-less Qt includes? For now it supports both, but it's very likely that by the end of january it will be Qt5 only. Or will the includes be if-def'ed? So will projects which refer to the Kdelibs Coding Style need to add an exception to their rules for the includes, if they want to prepare for Qt5? Or does the rule need adaption? With what I wrote above I think the natural conclusion is to adapt the rule. We should push to use the class name only includes I think. Also, contributing to Qt itself more, I noticed some differences in style between both (namely: we ignore space around operators, and we for braces around single line statements). Your question makes me wonder if we should update our own KDE Frameworks style to be closer of the Qt one. The aim here would be to be consistent accross the whole stack. Actually looking at both documents now, I see we should already follow the spacing around operators today... I think the intent of our coding style was to be the Qt style with extra exceptions but somewhere in the wording this gets lost. Maybe we should do a better job at it... If we go toward being closer to the Qt style maybe the proper way out would be to shorten the document quite a bit saying: use the Qt style + includes section + astyle section + emacs vim section. Right now by duplicating some but not all of the Qt style we're diluting the messaging it seems. Opinions? Thanks for bringing it up though, it's definitely the right time for that. Would there be any chance to have the style check done by a pre-commit hook? Or at least have a command-line tool that checks it for me? Personally I find it is quite hard to remember all the different coding styles of the bazillion projects I controbiute to and since i can't remember they in my head i have to find the page that describes the coding style and check it manually each time i want to contribute to the respective projects, and to be honest that is quite boring and lowers my morale. Cheers, Albert Regards. PS: Please, before hitting reply think twice! This type of thread can easily degenerate in bikeshedding. So put your personal preferences at the door as I tried to, it's really not the point. PPS: My PS might sound obvious and unneeded to some of you, but I got burnt enough times with coding styles discussions to be extra cautious now. It's really easy to loose perspective and I might end up doing the mistake myself, the reminder is for me too. :-) [*] Note that, History might prove me wrong there at some point if Qt6 introduces classes with the same name in different modules... if that's the case I hope it'll come with consistent namespacing though. :-)
Re: Kdelibs Coding Style vs. preparations for Qt5
On Saturday, December 29, 2012 04:25:54 PM Albert Astals Cid wrote: Would there be any chance to have the style check done by a pre-commit hook? Or at least have a command-line tool that checks it for me? Wouldn't that typically be a Krazy thing to check (and you can run Krazy checks from the command-line)? It'd be post-hoc, but at least the style issues can then be dealt with by those folks who run around fixing up Krazy things. [ade]
Re: Re: Kdelibs Coding Style vs. preparations for Qt5
On Saturday 29 December 2012 16:25:54 Albert Astals Cid wrote: Would there be any chance to have the style check done by a pre-commit hook? Or at least have a command-line tool that checks it for me? yeah that should be possible. At my old day-job I created a pre-commit hook to basically grep for some common coding style violations. Only problem: it checks for all files, so it would e.g. also check QML files which follow a different coding style. Assuming we can perfectly check the coding style with astyle it would be quite nice to have that as a pre-commit hook. Would be very nice to have it in the repos as it also moves away the nitpicks from code reviews. And no, Krazy check is for that too late as it's after commit. Cheers Martin signature.asc Description: This is a digitally signed message part.
Re: Kdelibs Coding Style vs. preparations for Qt5
On Saturday 29 December 2012 07:11:17 PM Martin Gräßlin wrote: On Saturday 29 December 2012 16:25:54 Albert Astals Cid wrote: Would there be any chance to have the style check done by a pre-commit hook? Or at least have a command-line tool that checks it for me? yeah that should be possible. At my old day-job I created a pre-commit hook to basically grep for some common coding style violations. Only problem: it checks for all files, so it would e.g. also check QML files which follow a different coding style. Assuming we can perfectly check the coding style with astyle it would be quite nice to have that as a pre-commit hook. Would be very nice to have it in the repos as it also moves away the nitpicks from code reviews. And no, Krazy check is for that too late as it's after commit. If you have the Krazy command line tool installed (https://gitorious.org/krazy) then you could write a pre-commit hook that runs 'krazy2 --style' on the files being committed. Keeping in mind that I haven't tested the Krazy kdelibs style checker in a long time. But I am accepting bug reports and patches. -Allen
Re: Kdelibs Coding Style vs. preparations for Qt5
On Saturday 29 December 2012 19:11:17 Martin Gräßlin wrote: On Saturday 29 December 2012 16:25:54 Albert Astals Cid wrote: Would there be any chance to have the style check done by a pre-commit hook? Or at least have a command-line tool that checks it for me? yeah that should be possible. At my old day-job I created a pre-commit hook to basically grep for some common coding style violations. Only problem: it checks for all files, so it would e.g. also check QML files which follow a different coding style. Assuming we can perfectly check the coding style with astyle it would be quite nice to have that as a pre-commit hook. Would be very nice to have it in the repos as it also moves away the nitpicks from code reviews. And no, Krazy check is for that too late as it's after commit. Note that something like that would be very welcome for me in KDevelop related projects as well. I've investigated it a few times already and it is sadly not yet perfectly feasable in my opinion. My approach was to format the new about- to-be-committed file and compare it to the unformatted file - if they both are equal everything is fine and we can continue. Otherwise some style guideline was violated and the commit was denied. While it was fairly trivial to write this up, the tool fails due to the imperfection of astyle and uncrustify. Both have still problems when formatting a few files, especially when it comes to macro usages or similar. Furthermore I was not yet able to write a perfect rule set for either astyle or uncrustify. Generally though, I would very much welcome any effort in that direction. If we could at least catch a few common issues it should help a lot. Cheers -- Milian Wolff m...@milianw.de http://milianw.de signature.asc Description: This is a digitally signed message part.
Re: Review Request: Fix for stale permissions information in properties dialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103555/ --- (Updated Dec. 29, 2012, 8:31 p.m.) Review request for kdelibs and David Faure. Changes --- Added testing done. Description --- If you open a directory that contains other directories in Konqueror or Dolphin, change the permission of one of these directories from outside, say the command line, and right click on the same directory to look at the permission tab in the properties dialog, you will see that the permission change has not been updated. This patch addresses that bug. This addresses bug 173733. http://bugs.kde.org/show_bug.cgi?id=173733 Diffs - kio/kio/kdirlister.cpp ec3d622 Diff: http://git.reviewboard.kde.org/r/103555/diff/ Testing (updated) --- 1. In konsole, create a test directory within another test directory: mkdir -p test/test1 2. Open Dolphin or Konqueror and navigate to the top newly created directory, i.e. test. 3. In konsole, cd into the first test directory: cd test 4. In konsole, change the permission of 'test1' from konsole. For example, chmod go-rx 5. In the open Dolphin or Konqueror, right click on test1, select properties and click on permission tab. 6. Validate whether or not the permission shown in the GUI matches what you get in the command line. Thanks, Dawit Alemayehu
Re: Kdelibs Coding Style vs. preparations for Qt5
On Sat, Dec 29, 2012 at 7:50 PM, Allen Winter win...@kde.org wrote: If you have the Krazy command line tool installed ( https://gitorious.org/krazy) then you could write a pre-commit hook that runs 'krazy2 --style' on the files being committed. ./krazy2 --style Unknown option: style Besides, it would be nice to get something automatically fixed when possible (i.e. not just a simple run as krazy usually does). Laszlo
Re: Kdelibs Coding Style vs. preparations for Qt5
On Saturday 29 December 2012 16:25:54 Albert Astals Cid wrote: El Dissabte, 29 de desembre de 2012, a les 10:48:48, Kevin Ottens va escriure: Hello, On Saturday 29 December 2012 02:47:13 Friedrich W. H. Kossebau wrote: --- 8 --- Qt Includes If you add #includes for Qt classes, use both the module and class name. This allows library code to be used by applications without excessive compiler include paths. Example: // wrong #include QString // correct #include QtCore/QString --- 8 --- From http://techbase.kde.org/Policies/Kdelibs_Coding_Style#Qt_Includes Since I've been skimming quite a bit through kdelibs codebase recently, needless to say this rule is in practice void as it's clearly not followed. Of course the current Qt Coding Style, which is the base of the kdelibs one, does not mention anything about that, given that its about their own headers. Yep. Now the Porting from Qt 4 to Qt 5 guide from KDAB recommends this: --- 8 --- Fixing up includes [...] Or more portably (Which works in Qt 4 and Qt 5): #include QWidget --- 8 --- From http://www.kdab.com/porting-from-qt-4-to-qt-5/ So what to do about this? I *personally* prefer the fully qualified style with the module name. That said, since we don't have per module namespaces to go with it and the like it's rather cosmetic more than anything. Now add to that the fact that the fully qualified style will generate more work when you port (for classes moving from one module to another) it sounds like it's the right move to drop them completely and use only the class name for includes[*]. Will kde-frameworks be Qt5-only, so not need to support both Qt4 and Qt5 and thus to use module-less Qt includes? For now it supports both, but it's very likely that by the end of january it will be Qt5 only. Or will the includes be if-def'ed? So will projects which refer to the Kdelibs Coding Style need to add an exception to their rules for the includes, if they want to prepare for Qt5? Or does the rule need adaption? With what I wrote above I think the natural conclusion is to adapt the rule. We should push to use the class name only includes I think. Also, contributing to Qt itself more, I noticed some differences in style between both (namely: we ignore space around operators, and we for braces around single line statements). Your question makes me wonder if we should update our own KDE Frameworks style to be closer of the Qt one. The aim here would be to be consistent accross the whole stack. Actually looking at both documents now, I see we should already follow the spacing around operators today... I think the intent of our coding style was to be the Qt style with extra exceptions but somewhere in the wording this gets lost. Maybe we should do a better job at it... If we go toward being closer to the Qt style maybe the proper way out would be to shorten the document quite a bit saying: use the Qt style + includes section + astyle section + emacs vim section. Right now by duplicating some but not all of the Qt style we're diluting the messaging it seems. Opinions? Thanks for bringing it up though, it's definitely the right time for that. Would there be any chance to have the style check done by a pre-commit hook? Or at least have a command-line tool that checks it for me? If anyone is willing to put the time and effort I would definitely welcome that! But it clearly need some more investigation (use krazy or astyle? how much setup for dev? how reliable? etc.) and experimentation. Personally I find it is quite hard to remember all the different coding styles of the bazillion projects I controbiute to and since i can't remember they in my head i have to find the page that describes the coding style and check it manually each time i want to contribute to the respective projects, and to be honest that is quite boring and lowers my morale. Yep, that's one more reason for trying to be closer to the Qt style as I mentioned in my previous email. But I'm waiting to see if someone has practical problems with such a move to have a clearer opinion. Regards. -- Kévin Ottens, http://ervin.ipsquad.net KDAB - proud patron of KDE, http://www.kdab.com signature.asc Description: This is a digitally signed message part.
Re: Kdelibs Coding Style vs. preparations for Qt5
On Saturday 29 December 2012 11:06:30 David Faure wrote: Well, for frameworks that intend to be as close to Qt as possible they should do the same (for the convenience of developers who don't use qmake/cmake but set up their project configuration in their IDE by hand, for instance Visual Studio). This means re-adding the missing QtWidgets/ in public headers once Qt5 is required in KF5. Out of curiosity, is it something which got documented in the Qt project? Or that's more a custom? (doesn't make it less valid, I'm being curious here and couldn't find the info) Regards. -- Kévin Ottens, http://ervin.ipsquad.net KDAB - proud patron of KDE, http://www.kdab.com signature.asc Description: This is a digitally signed message part.
Re: Kdelibs Coding Style vs. preparations for Qt5
Thiago Macieira wrote: On sábado, 29 de dezembro de 2012 22.58.49, Kevin Ottens wrote: On Saturday 29 December 2012 11:06:30 David Faure wrote: Well, for frameworks that intend to be as close to Qt as possible they should do the same (for the convenience of developers who don't use qmake/cmake but set up their project configuration in their IDE by hand, for instance Visual Studio). This means re-adding the missing QtWidgets/ in public headers once Qt5 is required in KF5. Out of curiosity, is it something which got documented in the Qt project? Or that's more a custom? (doesn't make it less valid, I'm being curious here and couldn't find the info) syncqt complains if public headers have Qt includes that aren't in the QtModule/ClassName or QtModule/classname.h form. syncqt is a Qt-internal tool. It is relevant to the headers of Qt itself, but not relevant outside it. Thanks, Steve.
Re: Review Request: Prevent activation of tabs opened in the background
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107048/ --- (Updated Dec. 30, 2012, 1:37 a.m.) Review request for KDE Base Apps and David Faure. Changes --- Updated patch based on dfaure's feedback. Note that this change requires addition of new API to KParts::PartManager. See https://git.reviewboard.kde.org/r/108017. Description --- The attached patch is an attempt to fix the bug where opening certain types of documents in a background tab results in the new tab being activated inadvertently. The issue can be reproduced by simply attempting to open a PDF document using either the MMB or the CTRL+LMB button in Konqueror. If you have a local PDF file simply navigate to the location of the file and click on it using the MMB. Otherwise, see the aforementioned bug report for a site with PDF links in it. If the Open new tabs in background option is checked when you clicked on the PDF document, then the PDF document is opened embedded in Konqueror tab in the background. However, this background tab should not be activated when the aforementioned option is enabled. Unfortunately, in the currently implementation the background tab will get activated if the part used to handle the resource (PDF file in this case) issues a FocusIn event, for example by calling setFocus. That is because KPart::PartManager installs an event filter on all the KParts it manages, listens for FocusIn events, and invokes KParts::PartManager::setActivatePart. This addresses bug 306417. http://bugs.kde.org/show_bug.cgi?id=306417 Diffs (updated) - konqueror/src/konqmainwindow.cpp 630dd46 konqueror/src/konqviewmanager.cpp 948a2a9 Diff: http://git.reviewboard.kde.org/r/107048/diff/ Testing --- - Tested opening a PDF file when the Open new tabs in background option is checked. - Tested opening a PDF file when the Open new tabs in background option is UNchecked. - Tested opening a PDF link from a webpage using both the MMB and CTRL+LMB. Thanks, Dawit Alemayehu
