Re: Review Request: Speed limit in ftp kio slave
On Aug. 10, 2011, 1:51 p.m., Thomas Zander wrote: kioslave/ftp/speedController.h, line 37 http://git.reviewboard.kde.org/r/102267/diff/1/?file=31277#file31277line37 I'm wondering if the function name contains a typo; maybe you meant 'calcHowMuchToRead' (notice the extra c in calc) ? acknowledged. - Tushar --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102267/#review5598 --- On Aug. 9, 2011, 7:16 p.m., Tushar Mehta wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102267/ --- (Updated Aug. 9, 2011, 7:16 p.m.) Review request for kdelibs. Summary --- - This patch contains the basic code which will put the limit on download speed of the ftp data transfer. - It is looking for speed-limit meta-data for deciding how much speed control is required. - If this meta-data is not found, code will work as it was before and no speed control related code will come into picture. - This patch is the most basic one which I have testing on my system and to the extent it is controlling the speed. - Lets say if speed limit is 30 KBps then mostly will get the avg speed around 30 to 35 KBps. - I am using QTime for measuring time elapsed between two socket read call and its precision is in millisecond. Looping is taking place in microsecond and thats why I am getting almost all the time 0 as time elapsed in between two calls. - To solve the above problem usleep is introduced to make it sync with the timer. Diffs - kioslave/ftp/CMakeLists.txt e080b02 kioslave/ftp/ftp.h 0bd375b kioslave/ftp/ftp.cpp 655524a kioslave/ftp/speedController.h PRE-CREATION kioslave/ftp/speedController.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/102267/diff Testing --- Thanks, Tushar
Re: Review Request: Speed limit in ftp kio slave
On Aug. 10, 2011, 1:07 p.m., David Faure wrote: kioslave/ftp/speedController.h, line 24 http://git.reviewboard.kde.org/r/102267/diff/1/?file=31277#file31277line24 kde_file.h isn't used in this header - move the #include to the .cpp file. I have used it for usleep. If I am not including it then it give me this: error: ‘usleep’ was not declared in this scope On Aug. 10, 2011, 1:07 p.m., David Faure wrote: kioslave/ftp/speedController.h, line 33 http://git.reviewboard.kde.org/r/102267/diff/1/?file=31277#file31277line33 trailing whitespace acknowledged. On Aug. 10, 2011, 1:07 p.m., David Faure wrote: kioslave/ftp/speedController.cpp, line 3 http://git.reviewboard.kde.org/r/102267/diff/1/?file=31278#file31278line3 Not my code :) acknowledged. On Aug. 10, 2011, 1:07 p.m., David Faure wrote: kioslave/ftp/speedController.cpp, line 31 http://git.reviewboard.kde.org/r/102267/diff/1/?file=31278#file31278line31 Make getters const, for good practice. acknowledged. On Aug. 10, 2011, 1:07 p.m., David Faure wrote: kioslave/ftp/speedController.cpp, line 55 http://git.reviewboard.kde.org/r/102267/diff/1/?file=31278#file31278line55 This doesn't seem to add anything, but to set. It replaces any existing socket. Note: the naming is wrong. m_socket looks like a member variable, while socket is the actual member variable. I would suggest to use m_ for the actual member vars, in fact -- and for sure never for function parameters. acknowledged. - Tushar --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102267/#review5593 --- On Aug. 9, 2011, 7:16 p.m., Tushar Mehta wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102267/ --- (Updated Aug. 9, 2011, 7:16 p.m.) Review request for kdelibs. Summary --- - This patch contains the basic code which will put the limit on download speed of the ftp data transfer. - It is looking for speed-limit meta-data for deciding how much speed control is required. - If this meta-data is not found, code will work as it was before and no speed control related code will come into picture. - This patch is the most basic one which I have testing on my system and to the extent it is controlling the speed. - Lets say if speed limit is 30 KBps then mostly will get the avg speed around 30 to 35 KBps. - I am using QTime for measuring time elapsed between two socket read call and its precision is in millisecond. Looping is taking place in microsecond and thats why I am getting almost all the time 0 as time elapsed in between two calls. - To solve the above problem usleep is introduced to make it sync with the timer. Diffs - kioslave/ftp/CMakeLists.txt e080b02 kioslave/ftp/ftp.h 0bd375b kioslave/ftp/ftp.cpp 655524a kioslave/ftp/speedController.h PRE-CREATION kioslave/ftp/speedController.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/102267/diff Testing --- Thanks, Tushar
Re: Review Request: Speed limit in ftp kio slave
On Aug. 10, 2011, 3:05 p.m., Thiago Macieira wrote: kioslave/ftp/CMakeLists.txt, line 11 http://git.reviewboard.kde.org/r/102267/diff/1/?file=31274#file31274line11 We usually do not use capitals in source code in kdelibs. (there are exceptions, but not in KIO). Also, it would be better if this were called ratecontroller.cpp, not speed controller. acknowledged. On Aug. 10, 2011, 3:05 p.m., Thiago Macieira wrote: kioslave/ftp/speedController.h, line 29 http://git.reviewboard.kde.org/r/102267/diff/1/?file=31277#file31277line29 Rename to RateController. acknowledged. On Aug. 10, 2011, 3:05 p.m., Thiago Macieira wrote: kioslave/ftp/speedController.h, line 37 http://git.reviewboard.kde.org/r/102267/diff/1/?file=31277#file31277line37 Suggest renaming to nextReadBlockSize(). acknowledged. On Aug. 10, 2011, 3:05 p.m., Thiago Macieira wrote: kioslave/ftp/speedController.h, line 45 http://git.reviewboard.kde.org/r/102267/diff/1/?file=31277#file31277line45 Use QElapserTimer, not QTime. acknowledged. - Tushar --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102267/#review5606 --- On Aug. 9, 2011, 7:16 p.m., Tushar Mehta wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102267/ --- (Updated Aug. 9, 2011, 7:16 p.m.) Review request for kdelibs. Summary --- - This patch contains the basic code which will put the limit on download speed of the ftp data transfer. - It is looking for speed-limit meta-data for deciding how much speed control is required. - If this meta-data is not found, code will work as it was before and no speed control related code will come into picture. - This patch is the most basic one which I have testing on my system and to the extent it is controlling the speed. - Lets say if speed limit is 30 KBps then mostly will get the avg speed around 30 to 35 KBps. - I am using QTime for measuring time elapsed between two socket read call and its precision is in millisecond. Looping is taking place in microsecond and thats why I am getting almost all the time 0 as time elapsed in between two calls. - To solve the above problem usleep is introduced to make it sync with the timer. Diffs - kioslave/ftp/CMakeLists.txt e080b02 kioslave/ftp/ftp.h 0bd375b kioslave/ftp/ftp.cpp 655524a kioslave/ftp/speedController.h PRE-CREATION kioslave/ftp/speedController.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/102267/diff Testing --- Thanks, Tushar
Re: Review Request: Speed limit in ftp kio slave
On Aug. 10, 2011, 1:07 p.m., David Faure wrote: kioslave/ftp/speedController.h, line 37 http://git.reviewboard.kde.org/r/102267/diff/1/?file=31277#file31277line37 why not just return the int, like int bytesToRead()? acknowledged. With the current design of the code we don't need pass by reference. At initial phase when I was experimenting with the code, I used this behaviour. I will correct this. - Tushar --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102267/#review5593 --- On Aug. 9, 2011, 7:16 p.m., Tushar Mehta wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102267/ --- (Updated Aug. 9, 2011, 7:16 p.m.) Review request for kdelibs. Summary --- - This patch contains the basic code which will put the limit on download speed of the ftp data transfer. - It is looking for speed-limit meta-data for deciding how much speed control is required. - If this meta-data is not found, code will work as it was before and no speed control related code will come into picture. - This patch is the most basic one which I have testing on my system and to the extent it is controlling the speed. - Lets say if speed limit is 30 KBps then mostly will get the avg speed around 30 to 35 KBps. - I am using QTime for measuring time elapsed between two socket read call and its precision is in millisecond. Looping is taking place in microsecond and thats why I am getting almost all the time 0 as time elapsed in between two calls. - To solve the above problem usleep is introduced to make it sync with the timer. Diffs - kioslave/ftp/CMakeLists.txt e080b02 kioslave/ftp/ftp.h 0bd375b kioslave/ftp/ftp.cpp 655524a kioslave/ftp/speedController.h PRE-CREATION kioslave/ftp/speedController.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/102267/diff Testing --- Thanks, Tushar
Re: Review Request: Fix missing ... in KBookmarkAction displayed text
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102262/#review5621 --- Ship it! Ah, I see. Indeed after reading QAction::iconText() it's all clear ;) But yeah this is quite obviously a bug in qaction (qt_strippedText), which should remove ... only at the end. - David On Aug. 9, 2011, 3:32 p.m., Yoann Laissus wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102262/ --- (Updated Aug. 9, 2011, 3:32 p.m.) Review request for kdelibs. Summary --- When a bookmark name, is too long, its name is truncated with ... at the middle. But, QAction strip those three dots by default. This patch solves this issue by defining imageText. Diffs - kio/bookmarks/kbookmarkmenu.cc 873f4a8 Diff: http://git.reviewboard.kde.org/r/102262/diff Testing --- It works with Konqueror and rekonq. Thanks, Yoann
Re: Review Request: Add the resource paramater in resource queries
On Wednesday 10 August 2011 15:22:51 Vishesh Handa wrote: If I push it into the 'frameworks' branch, then it won't be a part of KDE until KDE Platform 5, which is quite far away. Yes, this is why we were talking about splitting out nepomuk into its own repo, since it's already separate. But then it turns out that KIO uses nepomuk (although only in metainfo stuff which I think is an unfinished and mostly unused port) -- and worse, KParts::BrowserRun calls Nepomuk::Utils::createCopyEvent. In order to prepare for more modularity, do you see a way we could cut this dependency? Well, actually I do. We could emit a dbus signal when a download is finished, and one of the nepomuk daemons could connect to that signal. If you can do this, we can, for 4.8 already, split out nepomuk from kdelibs, which would allow you to work on nepomuk master. -- David Faure, fa...@kde.org, http://www.davidfaure.fr Sponsored by Nokia to work on KDE, incl. Konqueror (http://www.konqueror.org).
Re: Review Request: Do not terminate threads
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/#review5623 --- Good job, this is tricky code indeed. Some comments below. kio/kio/hostinfo.cpp http://git.reviewboard.kde.org/r/102179/#comment5034 The interesting question is, what if hostname was already in the m_lookups map? Either we want to detect this upfront and not create a second request, or we need a list of requests for a given hostname in the map. kio/kio/hostinfo.cpp http://git.reviewboard.kde.org/r/102179/#comment5033 reviewboard is highlighting quite a number of lines with trailing whitespace kio/kio/hostinfo.cpp http://git.reviewboard.kde.org/r/102179/#comment5035 You could also just create the worker on the stack here. kio/kio/hostinfo.cpp http://git.reviewboard.kde.org/r/102179/#comment5036 Maybe move nameLookupThread as a member of hostInfoAgentPrivate, to avoid multiplying the global statics (and therefore have more control over order of destruction)? Or does this make no sense? kio/kio/hostinfo.cpp http://git.reviewboard.kde.org/r/102179/#comment5037 Ouch, a busy loop. The standard solution is to use ... yes ... a semaphore :-) I love semaphores. Acquire the semaphore here, release it in the thread once the worker object has been created. - David On Aug. 11, 2011, 9:23 a.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 11, 2011, 9:23 a.m.) Review request for kdelibs and Dawit Alemayehu. Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs - kio/kio/hostinfo.cpp 344b1d8 Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Re: Fix kopete/kdenetwork build against Qt 4.8
On Thursday 04 August 2011 07:46:57 Jeremy Whiting wrote: Hello, Qt 4.8 has a change to moc which makes virtual inheritance from QObject no longer possible. This caused a problem in Jovie that was fixed by not virtually inheriting from QObject anymore. There is a similar compile problem in kdenetwork/kopete/libkopete at the moment and the attached patch fixes the problem. I can commit to 4.7 and trunk if it looks good to others. Looks definitely good. Virtual inheritance has only given us trouble, with things like qobjects. +1 for getting rid of 'virtual' there. -- David Faure, fa...@kde.org, http://www.davidfaure.fr Sponsored by Nokia to work on KDE, incl. Konqueror (http://www.konqueror.org).
Re: Review Request: new kded daemon to check .thumbnail directory space usage
On Thursday 04 August 2011 12:14:44 Christoph Feck wrote: * Integration with Nepomuk, so that thumbnails automatically get moved/deleted when the original file is. This can be done independently of nepomuk. When you move or delete a file using KIO, a dbus signal is emitted (see KDirNotify). Well, ok, there is nothing running all the time to listen to that, we could just handle thumbnails in the kio move/delete jobs directly (e.g. calling a static method in previewjob.cpp, to keep all the thumbnail-related code together). -- David Faure, fa...@kde.org, http://www.davidfaure.fr Sponsored by Nokia to work on KDE, incl. Konqueror (http://www.konqueror.org).
Re: Review Request: Make KSambaSharePrivate::getNetUserShareInfo less noisy
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102172/#review5627 --- Ship it! Sure, nice addition to my initial ac7fe47dc commit. - David On Aug. 1, 2011, 10:52 p.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102172/ --- (Updated Aug. 1, 2011, 10:52 p.m.) Review request for kdelibs and Rodrigo Belem. Summary --- Each time I open a open dialog i get a complain from KSambaSharePrivate::getNetUserShareInfo that says kolourpaint(25821) KSambaSharePrivate::getNetUserShareInfo: We got some errors while running 'net usershare info' kolourpaint(25821) KSambaSharePrivate::getNetUserShareInfo: net usershare: usershares are currently disabled With this patch people like me that have never configured that stop receiving the noise on the shell Diffs - kio/kio/ksambashare.cpp cd878b6 Diff: http://git.reviewboard.kde.org/r/102172/diff Testing --- The noise is gone Thanks, Albert
Re: Review Request: Do not terminate threads
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 11, 2011, 10:32 a.m.) Review request for kdelibs and Dawit Alemayehu. Changes --- Addressed David concerns Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs (updated) - kio/kio/hostinfo.cpp 344b1d8 Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Re: Review Request: Do not terminate threads
On Aug. 11, 2011, 9:41 a.m., David Faure wrote: kio/kio/hostinfo.cpp, line 236 http://git.reviewboard.kde.org/r/102179/diff/5/?file=31680#file31680line236 Maybe move nameLookupThread as a member of hostInfoAgentPrivate, to avoid multiplying the global statics (and therefore have more control over order of destruction)? Or does this make no sense? Could make that, but they seem somewhat separate things to me so i'd prefer to keep them separate unless you have a strong feeling about it On Aug. 11, 2011, 9:41 a.m., David Faure wrote: kio/kio/hostinfo.cpp, line 224 http://git.reviewboard.kde.org/r/102179/diff/5/?file=31680#file31680line224 You could also just create the worker on the stack here. Done On Aug. 11, 2011, 9:41 a.m., David Faure wrote: kio/kio/hostinfo.cpp, line 179 http://git.reviewboard.kde.org/r/102179/diff/5/?file=31680#file31680line179 reviewboard is highlighting quite a number of lines with trailing whitespace Fixed On Aug. 11, 2011, 9:41 a.m., David Faure wrote: kio/kio/hostinfo.cpp, line 266 http://git.reviewboard.kde.org/r/102179/diff/5/?file=31680#file31680line266 Ouch, a busy loop. The standard solution is to use ... yes ... a semaphore :-) I love semaphores. Acquire the semaphore here, release it in the thread once the worker object has been created. Changed. On Aug. 11, 2011, 9:41 a.m., David Faure wrote: kio/kio/hostinfo.cpp, line 177 http://git.reviewboard.kde.org/r/102179/diff/5/?file=31680#file31680line177 The interesting question is, what if hostname was already in the m_lookups map? Either we want to detect this upfront and not create a second request, or we need a list of requests for a given hostname in the map. Found out i can actually use the request id as key so we should not have this problem now. - Albert --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/#review5623 --- On Aug. 11, 2011, 10:32 a.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 11, 2011, 10:32 a.m.) Review request for kdelibs and Dawit Alemayehu. Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs - kio/kio/hostinfo.cpp 344b1d8 Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Re: Review Request: Do not terminate threads
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 11, 2011, 10:34 a.m.) Review request for kdelibs and Dawit Alemayehu. Changes --- Whitespace fixes Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs (updated) - kio/kio/hostinfo.cpp 344b1d8 Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Re: Review Request: Make KSambaSharePrivate::getNetUserShareInfo less noisy
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102172/#review5633 --- This review has been submitted with commit 75d7cc47fc071f2270492b027c01dcfb28b089b0 by Albert Astals Cid to branch KDE/4.7. - Commit On Aug. 1, 2011, 10:52 p.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102172/ --- (Updated Aug. 1, 2011, 10:52 p.m.) Review request for kdelibs and Rodrigo Belem. Summary --- Each time I open a open dialog i get a complain from KSambaSharePrivate::getNetUserShareInfo that says kolourpaint(25821) KSambaSharePrivate::getNetUserShareInfo: We got some errors while running 'net usershare info' kolourpaint(25821) KSambaSharePrivate::getNetUserShareInfo: net usershare: usershares are currently disabled With this patch people like me that have never configured that stop receiving the noise on the shell Diffs - kio/kio/ksambashare.cpp cd878b6 Diff: http://git.reviewboard.kde.org/r/102172/diff Testing --- The noise is gone Thanks, Albert
Re: Review Request: Make KSambaSharePrivate::getNetUserShareInfo less noisy
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102172/#review5634 --- This review has been submitted with commit de6bae2c9331ca2a1e9493288ca355856f4fcaad by Albert Astals Cid to branch frameworks. - Commit On Aug. 1, 2011, 10:52 p.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102172/ --- (Updated Aug. 1, 2011, 10:52 p.m.) Review request for kdelibs and Rodrigo Belem. Summary --- Each time I open a open dialog i get a complain from KSambaSharePrivate::getNetUserShareInfo that says kolourpaint(25821) KSambaSharePrivate::getNetUserShareInfo: We got some errors while running 'net usershare info' kolourpaint(25821) KSambaSharePrivate::getNetUserShareInfo: net usershare: usershares are currently disabled With this patch people like me that have never configured that stop receiving the noise on the shell Diffs - kio/kio/ksambashare.cpp cd878b6 Diff: http://git.reviewboard.kde.org/r/102172/diff Testing --- The noise is gone Thanks, Albert
Re: Review Request: Do not terminate threads
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/#review5636 --- kio/kio/hostinfo.cpp http://git.reviewboard.kde.org/r/102179/#comment5046 This class could be simplified to a simple struct. kio/kio/hostinfo.cpp http://git.reviewboard.kde.org/r/102179/#comment5047 This class isn't necessary. We can do the same with QThread alone. To quit the thread, connect the worker's destroyed() signal to the thread's quit() slot. Then just deleteLater the worker. kio/kio/hostinfo.cpp http://git.reviewboard.kde.org/r/102179/#comment5048 Remove the cache too. QHostInfo already caches. kio/kio/hostinfo.cpp http://git.reviewboard.kde.org/r/102179/#comment5049 Huh? What is the acquire/release doing here? - Thiago On Aug. 11, 2011, 10:34 a.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 11, 2011, 10:34 a.m.) Review request for kdelibs and Dawit Alemayehu. Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs - kio/kio/hostinfo.cpp 344b1d8 Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Re: Review Request: Add the resource paramater in resource queries
On Wednesday 10 August 2011 15:22:51 Vishesh Handa wrote: If I push it into the 'frameworks' branch, then it won't be a part of KDE until KDE Platform 5, which is quite far away. Yes, this is why we were talking about splitting out nepomuk into its own repo, since it's already separate. But then it turns out that KIO uses nepomuk (although only in metainfo stuff which I think is an unfinished and mostly unused port) -- and worse, Dolphin uses it and Gwenview does and maybe some more. KParts::BrowserRun calls Nepomuk::Utils::createCopyEvent. In order to prepare for more modularity, do you see a way we could cut this dependency? Well, actually I do. We could emit a dbus signal when a download is finished, and one of the nepomuk daemons could connect to that signal. ok If you can do this, we can, for 4.8 already, split out nepomuk from kdelibs, which would allow you to work on nepomuk master. I would be fine with this as long as I am not the one who has to create the tarball for 4.8 :D Cheers, Sebastian
Re: Review Request: Fix missing ... in KBookmarkAction displayed text
On Aug. 11, 2011, 9:13 a.m., David Faure wrote: Ah, I see. Indeed after reading QAction::iconText() it's all clear ;) But yeah this is quite obviously a bug in qaction (qt_strippedText), which should remove ... only at the end. It's my first patch in kdelibs, where should i push it ? In frameworks and KDE/4.7 ? - Yoann --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102262/#review5621 --- On Aug. 9, 2011, 3:32 p.m., Yoann Laissus wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102262/ --- (Updated Aug. 9, 2011, 3:32 p.m.) Review request for kdelibs. Summary --- When a bookmark name, is too long, its name is truncated with ... at the middle. But, QAction strip those three dots by default. This patch solves this issue by defining imageText. Diffs - kio/bookmarks/kbookmarkmenu.cc 873f4a8 Diff: http://git.reviewboard.kde.org/r/102262/diff Testing --- It works with Konqueror and rekonq. Thanks, Yoann
Review Request: rate control in ftp kio slave with review comments fixes
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102307/ --- Review request for kdelibs, Dawit Alemayehu, David Faure, Thiago Macieira, Thomas Zander, and Lukas Appelhans. Summary --- This patch is trying to clear the comments of the previous patch.(https://git.reviewboard.kde.org/r/102307/) Diffs - kioslave/ftp/CMakeLists.txt e080b02 kioslave/ftp/ftp.cpp 655524a kioslave/ftp/ratecontroller.h PRE-CREATION kioslave/ftp/ratecontroller.cpp PRE-CREATION kioslave/ftp/speedController.h PRE-CREATION kioslave/ftp/speedController.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/102307/diff Testing --- Thanks, Tushar
Review Request: plasma_applet_folderview - folder previews on mouse hover
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102300/ --- Review request for KDE Base Apps. Summary --- Hello, this is my attempt to resolve Bug 250703. The diff adds a gui option in the settings dialog which configures the click to view option. It was quite fun because most of the time I didn't know what I'm doing :) This addresses bug 250703. http://bugs.kde.org/show_bug.cgi?id=250703 Diffs - plasma/applets/folderview/folderview.h 35a0625 plasma/applets/folderview/folderview.cpp 6e95c40 plasma/applets/folderview/folderviewDisplayConfig.ui 961296d plasma/applets/folderview/iconview.h 4d736c5 Diff: http://git.reviewboard.kde.org/r/102300/diff Testing --- it compiles fine on kde 4.6.5 and does also work. Thanks, Greg
Re: Master or 4.7 under VirtualBox/KVM
Hi; On Wed, Aug 3, 2011 at 7:39 PM, Martin Gräßlin mgraess...@kde.org wrote: On Sunday 31 July 2011 12:58:05 Michael Jansen wrote: Did we manage to break kde under virtual machines? Or is the problem here? Has someone a recent kde running in a vm? For the record: I just found the commit which broke it: 6aa2b5caf625a142e92bf4e7c99452bfad968a8c which basically removed the check that OpenGL compositing does not get enabled with software rasterization. How should distributions proceed here, shall we just revert the faulty commit? Regards, ismail
Re: Formal complaint concerning the use of the name System Settings by GNOME
On Wed, 2011-08-10 at 13:47 +0200, todd rme wrote: On Wed, Aug 10, 2011 at 1:42 PM, Richard Hughes hughsi...@gmail.com wrote: On 4 August 2011 07:27, George Spelvin li...@horizon.com wrote: I think what is needed is a series of more specific alternate names in a .desktop file, with more levels than the current GenericName and Name. I think the KDE system settings desktop file just needs an addition of: OnlyShowIn=KDE; Richard. It has already been explained why this is not sufficient. System settings is needed to configure many aspects of KDE programs. Doing this will leave Gnome users unable to configure any KDE programs they use. I already pointed out a solution that makes it System Settings in KDE and KDE System Settings in other desktops. The KDE developers seemed to agree to this. The problem is solved. Please let's end this thread and get back to writing great free software. Thanks, Shaun
Re: Review Request: Do not terminate threads
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/#review5653 --- kio/kio/hostinfo.cpp http://git.reviewboard.kde.org/r/102179/#comment5065 Ah, the wifi lost my comment here: the name Petition surprised me a lot, I couldn't see what this was about. I had to use an english dictionary to see that this was a synonym of Request, which is a much more common name for this. How about renaming to NameLookupRequest? kio/kio/hostinfo.cpp http://git.reviewboard.kde.org/r/102179/#comment5066 m_lookups.insert(lookupId, petition); is faster (as recommended by Effective C++ iirc). - David On Aug. 11, 2011, 10:34 a.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 11, 2011, 10:34 a.m.) Review request for kdelibs and Dawit Alemayehu. Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs - kio/kio/hostinfo.cpp 344b1d8 Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Re: Review Request: Do not terminate threads
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 11, 2011, 10:10 p.m.) Review request for kdelibs and Dawit Alemayehu. Changes --- Rename Petition to Request Use QMap::insert instead of [] = Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs (updated) - kio/kio/hostinfo.cpp 344b1d8 Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Review Request: [KDE/Windows] Do not set cursorFlashTime, but respect Control Panel setting
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102313/ --- Review request for kdelibs and kdewin. Summary --- The bad thing with this bug is that it affects even non-KDE applications. This addresses bug 279882. http://bugs.kde.org/show_bug.cgi?id=279882 Diffs - kdeui/kernel/kglobalsettings.cpp b77b7b0 Diff: http://git.reviewboard.kde.org/r/102313/diff Testing --- None. I don't have KDE/Windows. Thanks, Christoph
Re: Review Request: Avoid terminating the thread in kio/kio/hostinfo.cpp
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102238/ --- (Updated Aug. 12, 2011, 3:45 a.m.) Review request for kdelibs. Changes --- New approach to solving this problem. Summary (updated) --- The attached patch is an alternate approach to address the issue of crashes that arise from terminating an active thread than the one proposed at https://git.reviewboard.kde.org/r/102179/. With this patch the function QHostInfo::lookupHost(QString, int) avoids the use of QThread::terminate with the following fairly simple changes: - Connect its finished signal to its parent deleteLater slot in the ctor so that the thread is automatically deleted later. - Store the looked up DNS info in the global cache to avoid unnecessary queries for the same request. - Check for cached DNS information and avoid doing reverse look ups before resorting to performing DNS queries in a separate thread. Diffs (updated) - kio/kio/hostinfo.cpp 344b1d8 Diff: http://git.reviewboard.kde.org/r/102238/diff Testing (updated) --- Local unit testing code. Tested both failing and working DNS lookup scenarios. Thanks, Dawit
Re: Review Request: Do not terminate threads
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/#review5654 --- #1. Doesn't this approach have similar issues to the one that forced me to change the previous QtConcurrent::run based implementation ? That is, doesn't the use of a single thread expose this function to lookup requests backlog and hence cause noticable delays ? #2. Isn't all this complexity a bit too much for such a small functionality ? Wouldn't a much simpler approach work just as well ? For example, - Keep the current look up thread, but connect its finished signal to its parent deleteLater signal in its ctor. [Automatic cleanup of the lookup thread]. - Make the lookup thread cache the looked up information in the global cache instead of storing it locally. [No need to store this inform 2x]. - Move all the preemtive lookup logic from the thread's run function to HostInfo::lookupHost. [No need to create a thread when DNS is already cached]. - Create the lookup thread on the heap and let it run until it finishes. When it is done it will clean itself up. See a patch that implements the above approach at https://git.reviewboard.kde.org/r/102238/ - Dawit On Aug. 11, 2011, 10:10 p.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 11, 2011, 10:10 p.m.) Review request for kdelibs and Dawit Alemayehu. Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs - kio/kio/hostinfo.cpp 344b1d8 Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
