Re: Review Request: Fix missing "..." in KBookmarkAction displayed text

[David Faure]

---
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: Do not terminate threads

[Albert Astals Cid]

---
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.


Changes
---

Add new patch based on dfaure and thiago's suggestions:

Thiago and I came up with a design like this:
* a single thread that makes async lookups. It's never blocked, it can always 
almost immediately read new requests from a queue and start them.
* each request is encapsulated in a class that has the QString
but also a QSemaphore.
* the main thread can block using a timeout (QSemaphore::tryAcquire(timeout))
* the request class is stored using a shared pointer, so that it's only 
deleted when neither the main thread nor the worker thread need it anymore
[otherwise the deletion is a problem, in the timeout case vs the normal case]

I hope i did what you guys had in mind :D


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: Plan to transition to KDE Frameworks

[David Faure]

On Wednesday 10 August 2011 11:49:29 Scott Kitterman wrote:
>  My concern isn't minor API extensions that might be needed to fix 
> a 4.7 bug, but with backporting new features to kdelibs 4.7 to support
> (rest  of the SC) 4.8 development.

Right, we don't want that. The idea is there will not be new features in 
kdelibs, since the people working on kdelibs are working on 'frameworks' 
instead.

-- 
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: Add the resource paramater in resource queries

[David Faure]

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

[David Faure]

---
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


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


reviewboard is highlighting quite a number of lines with trailing whitespace



kio/kio/hostinfo.cpp


You could also just create the worker on the stack here.



kio/kio/hostinfo.cpp


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


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

[David Faure]

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

[David Faure]

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

[David Faure]

---
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

[Albert Astals Cid]

---
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

[Albert Astals Cid]

> On Aug. 11, 2011, 9:41 a.m., David Faure wrote:
> > kio/kio/hostinfo.cpp, line 236
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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

[Albert Astals Cid]

---
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

[Commit Hook]

---
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

[Commit Hook]

---
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

[Thiago Macieira]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102179/#review5636
---



kio/kio/hostinfo.cpp


This class could be simplified to a simple struct.



kio/kio/hostinfo.cpp


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


Remove the cache too. QHostInfo already caches.



kio/kio/hostinfo.cpp


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: Do not terminate threads

[Albert Astals Cid]

> On Aug. 11, 2011, 11:11 a.m., Thiago Macieira wrote:
> > kio/kio/hostinfo.cpp, line 118
> > 
> >
> > This class could be simplified to a simple struct.

Yeah, but what's the benefit?


> On Aug. 11, 2011, 11:11 a.m., Thiago Macieira wrote:
> > kio/kio/hostinfo.cpp, line 264
> > 
> >
> > Remove the cache too. QHostInfo already caches.

As explained in the other request our cache has more "features" than yours, so 
can't do it.


> On Aug. 11, 2011, 11:11 a.m., Thiago Macieira wrote:
> > kio/kio/hostinfo.cpp, line 272
> > 
> >
> > Huh? What is the acquire/release doing here?

Making sure the worker has been created and the thread is ready to get 
petitions the way David asked me to do it.


> On Aug. 11, 2011, 11:11 a.m., Thiago Macieira wrote:
> > kio/kio/hostinfo.cpp, line 201
> > 
> >
> > 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.

Can you explain how do i create the worker in the qthread or move it to the 
thread without having my own class? Don't see how to do it.


- Albert


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102179/#review5636
---


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

[Sebastian Trueg]

> 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

[Yoann Laissus]

> 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

[Tushar Mehta]

---
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

[Greg T]

---
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

[İsmail Dönmez]

Hi;

On Wed, Aug 3, 2011 at 7:39 PM, Martin Gräßlin  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

[Shaun McCance]

On Wed, 2011-08-10 at 13:47 +0200, todd rme wrote:
> On Wed, Aug 10, 2011 at 1:42 PM, Richard Hughes  wrote:
> > On 4 August 2011 07:27, George Spelvin  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: Master or 4.7 under VirtualBox/KVM

[Martin Gräßlin]

On Thu, 11 Aug 2011 13:45:41 +0200, İsmail Dönmez  
wrote:

Hi;

On Wed, Aug 3, 2011 at 7:39 PM, Martin Gräßlin  
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?

NO!

I already fixed it in both master and branch. See cf2f572 in KDE/4.7 
and 2991187 in master.


Cheers
Martin


Regards,
ismail

Re: Review Request: Do not terminate threads

[David Faure]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102179/#review5653
---



kio/kio/hostinfo.cpp


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


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

[Albert Astals Cid]

---
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

[Christoph Feck]

---
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

[Dawit Alemayehu]

---
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

[Dawit Alemayehu]

---
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
> 
>