Re: Review Request: Avoid creating an empty .tbcache for bookmarks
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106563/#review19561 --- Ship it! Ah, thanks, now I understand. Please commit. - David Faure On Sept. 28, 2012, 8:30 p.m., Stefan Brüns wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106563/ --- (Updated Sept. 28, 2012, 8:30 p.m.) Review request for kdelibs and David Faure. Description --- Currently, an empty tbcache file is created when the toolbar bookmarks are the full bookmarks, i.e. no entries/folders with toolbar attribute. Diffs - kio/bookmarks/kbookmarkmanager.cc d8a9cb7 Diff: http://git.reviewboard.kde.org/r/106563/diff/ Testing --- Thanks, Stefan Brüns
Re: Review Request: Avoid creating an empty .tbcache for bookmarks
On Sept. 29, 2012, 8:25 a.m., David Faure wrote: Ah, thanks, now I understand. Please commit. The speedup idea is good too, feel free to switch to that solution. - David --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106563/#review19561 --- On Sept. 28, 2012, 8:30 p.m., Stefan Brüns wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106563/ --- (Updated Sept. 28, 2012, 8:30 p.m.) Review request for kdelibs and David Faure. Description --- Currently, an empty tbcache file is created when the toolbar bookmarks are the full bookmarks, i.e. no entries/folders with toolbar attribute. Diffs - kio/bookmarks/kbookmarkmanager.cc d8a9cb7 Diff: http://git.reviewboard.kde.org/r/106563/diff/ Testing --- Thanks, Stefan Brüns
Requiring cmake 2.8.9 for kdelibs 4.10 ?
Hi, since May 2010 kdelibs requires CMake 2.6.4 for building. This version is quite old in the meantime, and we are missing on new CMake features and also run into problems with some cmake modules where we have an own copy, which is not forward compatible to the new versions coming with CMake. So, I'd like to raise the required minimum version of CMake for kdelibs 4.10 to 2.8.9 (released beginning of August). I know this will cause some effort, because I guess only few distributions already come with CMake 2.8.9, but doing this once again after 2 1/2 years seems acceptable for me. Objections ? Alex
Re: Requiring cmake 2.8.9 for kdelibs 4.10 ?
Am Samstag 29 September 2012, 10:36:55 schrieb Alexander Neundorf: Hi, since May 2010 kdelibs requires CMake 2.6.4 for building. This version is quite old in the meantime, and we are missing on new CMake features and also run into problems with some cmake modules where we have an own copy, which is not forward compatible to the new versions coming with CMake. So, I'd like to raise the required minimum version of CMake for kdelibs 4.10 to 2.8.9 (released beginning of August). I know this will cause some effort, because I guess only few distributions already come with CMake 2.8.9, but doing this once again after 2 1/2 years seems acceptable for me. Objections ? None from my side, but I don't think that surprises anyone. 4.10 will likely require some other updated stuff which may cause deeper trouble in the system (e.g. some updated libs). Changing the CMake version will not change anything. It's not even required at runtime, so I don't think that will cause trouble to anyone. Eike signature.asc Description: This is a digitally signed message part.
Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106503/#review19563 --- konqueror/src/konqsessionmanager.cpp http://git.reviewboard.kde.org/r/106503/#comment15537 You moved this to the init list, shouldn't it be removed from here? konqueror/src/konqsessionmanager.cpp http://git.reviewboard.kde.org/r/106503/#comment15543 This code would be better with a KDialog subclass, rather than with all the code in a function -- and then you need a slot in the calling class, and an ugly qobject_castwindow()... If you split this up into a proper class, then it can have its own slots and member variables, and this will increase modularity and reusability. (and readability). Thanks. konqueror/src/konqsessionmanager.cpp http://git.reviewboard.kde.org/r/106503/#comment15542 Doesn't this lose ItemIsSelectable? I guess item-flags() | Qt::ItemIsUserCheckable would be better. konqueror/src/konqsessionmanager.cpp http://git.reviewboard.kde.org/r/106503/#comment15541 A QTreeWidgetItemIterator might make this code simpler (one loop instead of two nested loops) konqueror/src/konqsessionmanager.cpp http://git.reviewboard.kde.org/r/106503/#comment15539 This hash of hash makes the code rather complicated. I think the goal is to associate data with each item in the list -- for this you can just use QTreeWidgetItem::setData(0, Qt::UserRole, the_internal_string) and retrieve that again here, rather than a lookup based on the user-visible text (which could have duplicates) konqueror/src/konqsessionmanager.cpp http://git.reviewboard.kde.org/r/106503/#comment15538 Why did you remove the const? I don't see that you added code that changed that list. Probably a leftover, please revert. konqueror/src/konqsessionmanager.cpp http://git.reviewboard.kde.org/r/106503/#comment15540 Word puzzles are a big no no, in translated strings. Use i18nc(..., Window %1, indexOf+1); - David Faure On Sept. 28, 2012, 4:45 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106503/ --- (Updated Sept. 28, 2012, 4:45 p.m.) Review request for KDE Base Apps and David Faure. Description --- The attached patch fixes one of those pet peeve bugs that infurate me from time to time by allowing me to unselect the sessions I do not want to be restored when Konqueror's restore session dialog pops up. This addresses bug 260282. http://bugs.kde.org/show_bug.cgi?id=260282 Diffs - konqueror/src/konqsessionmanager.h ee629e4 konqueror/src/konqsessionmanager.cpp 68a003f Diff: http://git.reviewboard.kde.org/r/106503/diff/ Testing --- * Unselected sessions should not be restored. * If all available sessions are selected (the default), the behavior should remain the same as it is today. * If all available sessions are unselected, disable the Restore Session button. Screenshots --- old restore dialog http://git.reviewboard.kde.org/r/106503/s/729/ new restore dialog http://git.reviewboard.kde.org/r/106503/s/731/ new restore dialog v2 http://git.reviewboard.kde.org/r/106503/s/739/ Thanks, Dawit Alemayehu
Re: Review Request: In Konqueror's session restore dialog, allow user to chose which sessions to restore
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106503/#review19565 --- konqueror/src/konqsessionmanager.cpp http://git.reviewboard.kde.org/r/106503/#comment15547 i18n: what do these filter-... contexts mean? konqueror/src/konqsessionmanager.cpp http://git.reviewboard.kde.org/r/106503/#comment15544 KIcon icon(dialog-warning); is enough, pixmap() should load the correct icon. konqueror/src/konqsessionmanager.cpp http://git.reviewboard.kde.org/r/106503/#comment15545 i18n: remove the space before the question mark, please - Pino Toscano On Sept. 28, 2012, 4:45 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106503/ --- (Updated Sept. 28, 2012, 4:45 p.m.) Review request for KDE Base Apps and David Faure. Description --- The attached patch fixes one of those pet peeve bugs that infurate me from time to time by allowing me to unselect the sessions I do not want to be restored when Konqueror's restore session dialog pops up. This addresses bug 260282. http://bugs.kde.org/show_bug.cgi?id=260282 Diffs - konqueror/src/konqsessionmanager.h ee629e4 konqueror/src/konqsessionmanager.cpp 68a003f Diff: http://git.reviewboard.kde.org/r/106503/diff/ Testing --- * Unselected sessions should not be restored. * If all available sessions are selected (the default), the behavior should remain the same as it is today. * If all available sessions are unselected, disable the Restore Session button. Screenshots --- old restore dialog http://git.reviewboard.kde.org/r/106503/s/729/ new restore dialog http://git.reviewboard.kde.org/r/106503/s/731/ new restore dialog v2 http://git.reviewboard.kde.org/r/106503/s/739/ Thanks, Dawit Alemayehu
Re: Requiring cmake 2.8.9 for kdelibs 4.10 ?
On Saturday 29 September 2012, André Wöbbeking wrote: Hi Alex, On Saturday 29 September 2012 10:36:55 Alexander Neundorf wrote: I know this will cause some effort, because I guess only few distributions already come with CMake 2.8.9, but doing this once again after 2 1/2 years seems acceptable for me. Do you really need 2.8.9 or would 2.8.8 from April also be sufficient? The older version would probably cause less trouble?!? Actually I'd prefer 2.8.10, to be released beginning of November, which might even still make it before the dependency freeze :-) Absolute minimum is 2.8.8: http://www.cmake.org/pipermail/cmake/2012-April/049955.html This is 2.8.9: http://permalink.gmane.org/gmane.comp.kde.devel.general/64919 It additionally has the new POSITION_INDEPENDENT_CODE target property, generating man pages for different sections, and the usual set of bug fixes :-) To make it short, when upgrading CMake, and causing some pain by this, I'd like to have the maximum outcome of it, i.e. the most recent version possible. Alex
Re: Requiring cmake 2.8.9 for kdelibs 4.10 ?
Hi Alex, On Saturday 29 September 2012 10:36:55 Alexander Neundorf wrote: I know this will cause some effort, because I guess only few distributions already come with CMake 2.8.9, but doing this once again after 2 1/2 years seems acceptable for me. Do you really need 2.8.9 or would 2.8.8 from April also be sufficient? The older version would probably cause less trouble?!? Cheers, André
Re: Requiring cmake 2.8.9 for kdelibs 4.10 ?
Am Samstag 29 September 2012, 11:48:16 schrieb André Wöbbeking: Hi Alex, On Saturday 29 September 2012 10:36:55 Alexander Neundorf wrote: I know this will cause some effort, because I guess only few distributions already come with CMake 2.8.9, but doing this once again after 2 1/2 years seems acceptable for me. Do you really need 2.8.9 or would 2.8.8 from April also be sufficient? The older version would probably cause less trouble?!? What trouble would it cause? Eike signature.asc Description: This is a digitally signed message part.
Re: Requiring cmake 2.8.9 for kdelibs 4.10 ?
On Saturday 29 September 2012 11:59:04 Rolf Eike Beer wrote: Am Samstag 29 September 2012, 11:48:16 schrieb André Wöbbeking: Hi Alex, On Saturday 29 September 2012 10:36:55 Alexander Neundorf wrote: I know this will cause some effort, because I guess only few distributions already come with CMake 2.8.9, but doing this once again after 2 1/2 years seems acceptable for me. Do you really need 2.8.9 or would 2.8.8 from April also be sufficient? The older version would probably cause less trouble?!? What trouble would it cause? Well it's one more dependency to fulfill before you can build KDE. Probably no problem for most people here but maybe for distributions or users.
Re: Requiring cmake 2.8.9 for kdelibs 4.10 ?
Am Samstag 29 September 2012, 12:12:43 schrieb André Wöbbeking: On Saturday 29 September 2012 11:59:04 Rolf Eike Beer wrote: Am Samstag 29 September 2012, 11:48:16 schrieb André Wöbbeking: Hi Alex, On Saturday 29 September 2012 10:36:55 Alexander Neundorf wrote: I know this will cause some effort, because I guess only few distributions already come with CMake 2.8.9, but doing this once again after 2 1/2 years seems acceptable for me. Do you really need 2.8.9 or would 2.8.8 from April also be sufficient? The older version would probably cause less trouble?!? What trouble would it cause? Well it's one more dependency to fulfill before you can build KDE. Probably no problem for most people here but maybe for distributions or users. If distros go and upgrade their KDE tarballs to get the new version, what more hassle is it to just drop in a new CMake tarball before? CMake doesn't have any dependency changes in the last time I know of and also ships everything needed inside it's own tarball. So you are right that this is one more package, but I bet that KDE SC 4.10 would require other things to be upgraded too. But CMake will be the easiest thing to upgrade IMHO. Eike signature.asc Description: This is a digitally signed message part.
Re: Re: Requiring cmake 2.8.9 for kdelibs 4.10 ?
On Saturday 29 September 2012 11:48:03 Alexander Neundorf wrote: On Saturday 29 September 2012, André Wöbbeking wrote: Hi Alex, On Saturday 29 September 2012 10:36:55 Alexander Neundorf wrote: I know this will cause some effort, because I guess only few distributions already come with CMake 2.8.9, but doing this once again after 2 1/2 years seems acceptable for me. Do you really need 2.8.9 or would 2.8.8 from April also be sufficient? The older version would probably cause less trouble?!? Actually I'd prefer 2.8.10, to be released beginning of November, which might even still make it before the dependency freeze :-) please don't. Debian testing and the next Kubuntu release is currently at 2.8.9 (Debian in fact at 2.8.9~rc1-1) and given that both are frozen there is no possibility to get in a new version. Using a later version means quite a high entry level for hacking on KDE. Cheers Martin signature.asc Description: This is a digitally signed message part.
Re: Requiring cmake 2.8.9 for kdelibs 4.10 ?
please don't. Debian testing and the next Kubuntu release is currently at 2.8.9 (Debian in fact at 2.8.9~rc1-1) and given that both are frozen there +1 Cheerio, Ivan signature.asc Description: This is a digitally signed message part.
Re: Requiring cmake 2.8.9 for kdelibs 4.10 ?
On Saturday 29 September 2012, Martin Gräßlin wrote: On Saturday 29 September 2012 11:48:03 Alexander Neundorf wrote: On Saturday 29 September 2012, André Wöbbeking wrote: Hi Alex, On Saturday 29 September 2012 10:36:55 Alexander Neundorf wrote: I know this will cause some effort, because I guess only few distributions already come with CMake 2.8.9, but doing this once again after 2 1/2 years seems acceptable for me. Do you really need 2.8.9 or would 2.8.8 from April also be sufficient? The older version would probably cause less trouble?!? Actually I'd prefer 2.8.10, to be released beginning of November, which might even still make it before the dependency freeze :-) please don't. Debian testing and the next Kubuntu release is currently at 2.8.9 (Debian in fact at 2.8.9~rc1-1) Do you mean they ship cmake 2.8.9 rc1 ? They should not do that, this can break all kinds of stuff. and given that both are frozen there is no possibility to get in a new version. Using a later version means quite a high entry level for hacking on KDE. I know that. Installing cmake is as simple as wget'ing the file und untarring it to /opt/. But how does that differ from requiring other very recent packages, including Qt ? Beside, we didn't upgrade cmake since more than 2 years... But ok, I did not seriously propose to require CMake 2.8.10. Alex
Re: Requiring cmake 2.8.9 for kdelibs 4.10 ?
On Saturday 29 September 2012 13:47:16 Thomas Lübking wrote: Am 29.09.2012, 13:19 Uhr, schrieb Alexander Neundorf neund...@kde.org: Do you mean they ship cmake 2.8.9 rc1 ? Seems so. http://packages.debian.org/search?keywords=cmakesearchon=namessuite=testin gsection=all or http://release.debian.org/migration/testing.pl?package=cmake
Re: Re: Requiring cmake 2.8.9 for kdelibs 4.10 ?
On Saturday 29 September 2012 13:19:30 you wrote: But how does that differ from requiring other very recent packages, including Qt ? we don't do that. We still require only Qt 4.7 and there is one difference in build system vs. some depending library. Depending library is nicely handled by systems like kdesrc-build. The example kdesrc-buildrc comes with all the pieces already there, so if there is really the need I uncomment a part and let it build. For cmake on the other hand I have no idea how to install it without reading through the documentation and then I have to adjust all my build scripts to take this custom cmake and I have to remember to remove it again once it's in the distro. Beside, we didn't upgrade cmake since more than 2 years... which doesn't mean we have to update to the latest cmake in town. Given that kdelibs is feature frozen anyway I'm personally surprised by the request for an updated cmake (though I don't mind as long as it's in distro packages). But ok, I did not seriously propose to require CMake 2.8.10. :-) Cheers Martin signature.asc Description: This is a digitally signed message part.
Re: Requiring cmake 2.8.9 for kdelibs 4.10 ?
On Saturday 29 September 2012, Martin Gräßlin wrote: On Saturday 29 September 2012 13:19:30 you wrote: But how does that differ from requiring other very recent packages, including Qt ? we don't do that. We still require only Qt 4.7 and there is one difference in build system vs. some depending library. Depending library is nicely handled by systems like kdesrc-build. The example kdesrc-buildrc comes with all the pieces already there, so if there is really the need I uncomment a part and let it build. For cmake on the other hand I have no idea how to install it without $ wget http://www.cmake.org/files/v2.8/cmake-2.8.9-Linux-i386.tar.gz $ su $ cd /opt $ tar -zxvf path/to/cmake-2.8.9-Linux-i386.tar.gz ... $ /opt/cmake-2.8.9-Linux-i386/bin/cmake ...all your arguments Alex
Re: Requiring cmake 2.8.9 for kdelibs 4.10 ?
On Saturday, September 29, 2012 12:12:43 André Wöbbeking wrote: On Saturday 29 September 2012 11:59:04 Rolf Eike Beer wrote: Am Samstag 29 September 2012, 11:48:16 schrieb André Wöbbeking: Hi Alex, On Saturday 29 September 2012 10:36:55 Alexander Neundorf wrote: I know this will cause some effort, because I guess only few distributions already come with CMake 2.8.9, but doing this once again after 2 1/2 years seems acceptable for me. Do you really need 2.8.9 or would 2.8.8 from April also be sufficient? The older version would probably cause less trouble?!? What trouble would it cause? Well it's one more dependency to fulfill before you can build KDE. Probably no problem for most people here but maybe for distributions or users. I've recently had to add support for building CMake to kdesrc-build for this reason. This isn't a complaint per se as the work is already done, but the price of closely tracking the latest stable release of a dependency is pretty much always that it hampers development of KDE itself. 2.8.8 at least has a shot of having packages available in most distros, that would obviously not be as true for 2.8.10 (and 2.8.9 would be a question as well). I guess the point is that if we're going to bump the dependency to something that isn't broadly available from distro packages then we might as well bump the requirement to the latest release. But hopefully we only make these bumps when there are clear advantages. Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: Review Request: Fix Konqueror's MMB click to close tab option
On Sept. 29, 2012, 6:48 a.m., David Faure wrote: Doesn't this break moving a tab with MMB then? The KTabBar mousePress/mouseRelease code won't be called anymore. If this is the case, then what we really have is two incompatible features... so we could just remove the moving with MMB code in ktabbar and this eventFilter wouldn't be needed. Why remove a feature other application might want to have or was the moving a tab with MMB specifically added for Konqueror's sake ? Anyhow, this patch does not really break moving a tab with MMB unless the user checks the Middle-click on a tab to close it option. Otherwise, the MMB click on a tab will continue to behave as it does now. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106615/#review19560 --- On Sept. 27, 2012, 6:11 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106615/ --- (Updated Sept. 27, 2012, 6:11 p.m.) Review request for KDE Base Apps and David Faure. Description --- The attached patch fixes the bug where clicking on a tab with the MMB while the Middle-click on a tab to close it option is checked results in the tab gaining the focus first before being closed instead of being closed in the background. That results in the active tab being changed when a user activates the aforementioned option and clicks on a background tab with the MMB. For the record the reason why this really happens is KTabBar's treatment of the MMB clicks when the setMovable is set to TRUE. In addition to emitting a mouseMiddleClick signal which Konqueror currently relies on to close the tab, it also eats the MMB click event and generates a new LMB click event in its place. Apparently that was done for compatibility sake so that the user can move tabs using either the LMB or MMB. Anyhow, this patch addresses the problem by installing an event filter to intercept the mouse events from the tabbar and handle MMB clicks on its own when the aforementioned option is checked so that we do not receive unnecessary bogus events. This addresses bug 264058. http://bugs.kde.org/show_bug.cgi?id=264058 Diffs - konqueror/src/konqtabs.h 4b6f1f1 konqueror/src/konqtabs.cpp 611659f Diff: http://git.reviewboard.kde.org/r/106615/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request: Fix for CTRL+Tab not switcing tabs in Konqueror when the active tab is a Dolphin filemanagement part
On Sept. 28, 2012, 5:42 p.m., David Faure wrote: dolphin/src/kitemviews/kitemlistview.cpp, line 871 http://git.reviewboard.kde.org/r/106569/diff/2/?file=87267#file87267line871 This strikes me as wrong. Why should this widget's event(QEvent*) change the default state of ALL events ?!? No widgets do that, anywhere. David Faure wrote: For instance, this would break closing such a widget (if it was a toplevel). ignore() all key events, OK (like QWidget::keyPressEvent does). But not *all* events. Dawit Alemayehu wrote: Right. That is why I just fixed it. See http://commits.kde.org/kde-baseapps/b3789357335cbbb100b4a089ee7723078e5d219f. David Faure wrote: Much better, thanks. (Sorry for the race condition between reading k-c-d and kde-commits ;) Didn't happen in the old days before reviewboard ;) Frank Reininghaus wrote: Thanks Dawit and David for noticing that this must be fixed in a better way! It seems that my knowledge of Qt's event handling is even more limited than I thought. I thought that ignoring an event is the way to go if we don't handle it inside the class, and I can't see right now why the first commit broke things like dragdrop, but maybe I should think about it again tomorrow after getting some sleep :-) Anyway, the 4.9.2 packages unfortunately contain the bad commit. I'll send a message to the release-team and packagers mailing lists to inform them about the issue. Thanks again for your help. That shows me that I should probably add a couple more unit tests to cover the basic things... David Faure wrote: Some events are accepted by default, some are ignored by default (= before being sent to the widget). Propagation happens [for the type of events that do propagate] if the event is ignored and event() returns true. Thanks for explaining, David! Just for the record: the 4.9.2 packages were updated with Dawit's fix, so nobody will suffer from the temporary regression. - Frank --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106569/#review19524 --- On Sept. 26, 2012, 12:53 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106569/ --- (Updated Sept. 26, 2012, 12:53 p.m.) Review request for Dolphin and KDE Base Apps. Description --- This patch fixes a bug where pressing CTRL+Tab does not switch tabs when active tab in Konqueror is a Dolphin's filemanagement part. It happens because DolphinView installs an event filter and does not call event-ignore() or event-setAccepted(false) to allow those events to be propagated up the event chain. This addresses bug 302329. http://bugs.kde.org/show_bug.cgi?id=302329 Diffs - dolphin/src/kitemviews/kitemlistview.cpp 54a8cbf Diff: http://git.reviewboard.kde.org/r/106569/diff/ Testing --- Thanks, Dawit Alemayehu
Review Request: Fix whitespace related bugs when listing directories in kio_ftp
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106636/ --- Review request for kdelibs and David Faure. Description --- The attached patch fixes a regression caused by a commit to fix bug# 88575. Namely, it fixed a problem where filenames with whitespaces in them were not handled correctly by kio_ftp. That is because the filenames were automatically trimmed when read from the directory. However, the fix then re-introduced the original bug and the reason why names were automatically trimmed in the first place. Some ftp servers add bogus whitespace between the date and filename in their listings. Hence, we need need to fix both of these opposing issues without breaking the other. This patch tries to do just that by actually validating each name entry that starts with a whitespace. That way the correct name is sent to the client. This addresses bug 300988. http://bugs.kde.org/show_bug.cgi?id=300988 Diffs - kioslave/ftp/ftp.h 2465a4b kioslave/ftp/ftp.cpp 26be283 Diff: http://git.reviewboard.kde.org/r/106636/diff/ Testing --- Thanks, Dawit Alemayehu
