[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-frameworks-devel
Subject: Re: Review Request 129394: [filenamesearch] Fix huge ram usage in kded module
From: Emmanuel Pescosta <emmanuelpescosta099 () gmail ! com>
Date: 2016-11-22 7:13:53
Message-ID: 20161122071353.32069.53547 () mimi ! kde ! org
[Download RAW message or body]
--===============7502430230127958372==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
> On Nov. 21, 2016, 9:34 a.m., David Faure wrote:
> > filenamesearch/kded/filenamesearchmodule.cpp, line 84
> > <https://git.reviewboard.kde.org/r/129394/diff/1/?file=485431#file485431line84>
> >
> > Well, if dirUrl looks like "filenamesearch:?search=file&url=file:///path/to/file" \
> > then dirUrl.path() is empty, and this code is incorrect (it should use the query \
> > item "url", not the path). What am I missing?
>
> Anthony Fieroni wrote:
> This is a big misunderstanding mainly by me. Emitted url should contains query with \
> new path ? for (const QString &file : files) {
> const QUrl url(file);
> if (!url.isLocalFile()) {
> continue;
> }
> const QString urlPath = url.path();
> for (const QUrl &dirUrl : m_searchUrls) {
> QUrlQuery urlQuery(dirUrl);
> QString str = urlQuery.queryItemValue(QStringLiteral("url"));
> if (urlPath.startsWith(QUrl(str).path())) {
> QUrl temp;
> temp.setScheme(QStringLiteral("filenamesearch"));
> urlQuery.removeQueryItem(QStringLiteral("url");
> urlQuery.addQueryItem(QStringLiteral('url"), url);
> temp.setQuery(urlQuery);
> fileList << temp;
> }
> }
> }
>
> David Faure wrote:
> Maybe, but I'm still in the dark about something. How can KDirLister cope with \
> listing such URLs? It wants a directory URL and files inside that directory. Such a \
> filenamesearch URL doesn't look like it's a file inside a directory, in terms of \
> paths. Ideally I would look into the code to understand what is being done but I'm \
> short on time.
> Does kio_filenamesearch really return items from listDir(), which have an empty \
> path too, just like the listed directory? I would assume this breaks many things in \
> KDirLister.
> Please clarify with the dolphin people (or whoever wrote the filenamesearch KIO) \
> about the URL structure, then it will be straightforward to do the URL conversions \
> in this code.
> Anthony Fieroni wrote:
> I'm invited Emmanuel, who knows? \
> https://github.com/KDE/dolphin/blob/1710304e9ba926d2aec4226d00974b826f9bcbc0/src/kitemviews/kfileitemmodel.cpp#L123 \
> url("filenamesearch:?search=file&url=file:///path/to/file") in slot \
> https://github.com/KDE/dolphin/blob/1710304e9ba926d2aec4226d00974b826f9bcbc0/src/kitemviews/kfileitemmodel.cpp#L77 \
> comes results
Peter wrote the filenamesearch slave.
Filenamesearch URLs are only used to initiate a search. The URL contains the search \
start-folder, the pattern used for matching and an optional 'check contents' query \
item which can be yes or no. The filenamesearch ioslave uses all this query items to \
perform the search. It recursively opens each folder via KCoreDirLister (starting \
with the start-folder of the filenamesearch URL) and iterates trough the item list of \
the directory, every matching item is then listed via `listEntry` by using the \
UDSEntry of the matching item (see 1). So kio_filenamesearch can only return items \
with an empty path if the underlying ioslave (local, smb, ftp, ...) returns items \
with an empty path.
[1] https://github.com/KDE/kio-extras/blob/master/filenamesearch/kio_filenamesearch.cpp#L103
- Emmanuel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129394/#review101007
-----------------------------------------------------------
On Nov. 14, 2016, 12:44 p.m., Anthony Fieroni wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129394/
> -----------------------------------------------------------
>
> Review request for KDE Frameworks, Anthony Fieroni, David Faure, and Emmanuel \
> Pescosta.
>
> Repository: kio-extras
>
>
> Description
> -------
>
> Bug is introduced in https://git.reviewboard.kde.org/r/129297/
> When is fixed new kio-extras realease is needed for 16.08 branch.
>
>
> Diffs
> -----
>
> filenamesearch/kded/filenamesearchmodule.cpp 3f9f582
>
> Diff: https://git.reviewboard.kde.org/r/129394/diff/
>
>
> Testing
> -------
>
> No big ram usage but still not works as expected.
> 1. Perform search in Dolphin
> 2. Delete one result item
> 3. View must be update, but it's not
>
>
> Thanks,
>
> Anthony Fieroni
>
>
--===============7502430230127958372==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 7bit
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 \
solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;"> \
<tr> <td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/129394/">https://git.reviewboard.kde.org/r/129394/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On November 21st, 2016, 9:34 a.m. CET, <b>David \
Faure</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;"> <thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;"> <a \
href="https://git.reviewboard.kde.org/r/129394/diff/1/?file=485431#file485431line84" \
style="color: black; font-weight: bold; text-decoration: \
underline;">filenamesearch/kded/filenamesearchmodule.cpp</a> <span \
style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">84</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> \
<span class="k">if</span> <span class="p">(</span><span class="n">urlPath</span><span \
class="p">.</span><span class="n">startsWith</span><span class="p">(</span><span \
class="n">dirUrl</span><span class="p">.</span><span class="n">path</span><span \
class="p">()))</span> <span class="p">{</span></pre></td> </tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Well, if dirUrl looks \
like "filenamesearch:?search=file&url=file:///path/to/file" then \
dirUrl.path() is empty, and this code is incorrect (it should use the query item \
"url", not the path). What am I missing?</pre> </blockquote>
<p>On November 21st, 2016, 10:45 p.m. CET, <b>Anthony Fieroni</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This \
is a big misunderstanding mainly by me. Emitted url should contains query with new \
path ? for (const QString &file : files) {
const QUrl url(file);
if (!url.isLocalFile()) {
continue;
}
const QString urlPath = url.path();
for (const QUrl &dirUrl : m_searchUrls) {
QUrlQuery urlQuery(dirUrl);
QString str = urlQuery.queryItemValue(QStringLiteral("url"));
if (urlPath.startsWith(QUrl(str).path())) {
QUrl temp;
temp.setScheme(QStringLiteral("filenamesearch"));
urlQuery.removeQueryItem(QStringLiteral("url");
urlQuery.addQueryItem(QStringLiteral('url"), url);
temp.setQuery(urlQuery);
fileList << temp;
}
}
}</p></pre>
</blockquote>
<p>On November 22nd, 2016, 12:01 a.m. CET, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">Maybe, but I'm still in the dark about something. How can KDirLister cope \
with listing such URLs? It wants a directory URL and files inside that directory. \
Such a filenamesearch URL doesn't look like it's a file inside a directory, in terms \
of paths. Ideally I would look into the code to understand what is being done but I'm \
short on time.</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">Does kio_filenamesearch really return \
items from listDir(), which have an empty path too, just like the listed directory? I \
would assume this breaks many things in KDirLister.</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">Please clarify with the dolphin people (or whoever wrote the filenamesearch \
KIO) about the URL structure, then it will be straightforward to do the URL \
conversions in this code.</p></pre> </blockquote>
<p>On November 22nd, 2016, 6:45 a.m. CET, <b>Anthony Fieroni</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm \
invited Emmanuel, who knows? \
https://github.com/KDE/dolphin/blob/1710304e9ba926d2aec4226d00974b826f9bcbc0/src/kitemviews/kfileitemmodel.cpp#L123 \
url("filenamesearch:?search=file&url=file:///path/to/file") in slot \
https://github.com/KDE/dolphin/blob/1710304e9ba926d2aec4226d00974b826f9bcbc0/src/kitemviews/kfileitemmodel.cpp#L77 \
comes results</p></pre> </blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">Peter wrote the filenamesearch slave.</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">Filenamesearch URLs are only used to initiate a search. The URL contains \
the search start-folder, the pattern used for matching and an optional 'check \
contents' query item which can be yes or no. The filenamesearch ioslave uses all this \
query items to perform the search. It recursively opens each folder via \
KCoreDirLister (starting with the start-folder of the filenamesearch URL) and \
iterates trough the item list of the directory, every matching item is then listed \
via <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: \
normal;margin: 0;line-height: inherit;">listEntry</code> by using the UDSEntry of the \
matching item (see 1). So kio_filenamesearch can only return items with an empty path \
if the underlying ioslave (local, smb, ftp, ...) returns items with an empty \
path.</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">[1] \
https://github.com/KDE/kio-extras/blob/master/filenamesearch/kio_filenamesearch.cpp#L103</p></pre>
<br />
<p>- Emmanuel</p>
<br />
<p>On November 14th, 2016, 12:44 p.m. CET, Anthony Fieroni wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: \
1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; \
-webkit-border-radius: 6px;"> <tr>
<td>
<div>Review request for KDE Frameworks, Anthony Fieroni, David Faure, and Emmanuel \
Pescosta.</div> <div>By Anthony Fieroni.</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kio-extras
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Bug is introduced in \
https://git.reviewboard.kde.org/r/129297/ When is fixed new kio-extras realease is \
needed for 16.08 branch.</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">No big ram usage but still not works as expected. 1. \
Perform search in Dolphin 2. Delete one result item
3. View must be update, but it's not</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>filenamesearch/kded/filenamesearchmodule.cpp <span style="color: \
grey">(3f9f582)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/129394/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--===============7502430230127958372==--
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic