<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/127386/">https://git.reviewboard.kde.org/r/127386/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 17th, 2016, 5:26 p.m. UTC, <b>Dominik Haumann</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;">The only real difference to the original version is that the line</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%">int isMtpDevice = LIBMTP_Check_Specific_Device(solidBusNum, solidDevNum);
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">is removed. This somehow implies that this check is too restrictive. If this indeed is the case, then this might very well be a bug in libmtp.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I haved tried to find something in the net, especially since other mtp-clients (from other DEs) are said to work, but couldn't really find something useful for now...</p></pre>
</blockquote>
<p>On March 17th, 2016, 5:35 p.m. UTC, <b>Dominik Haumann</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;">Found something dating back to 2011: https://sourceforge.net/p/libmtp/mailman/libmtp-discuss/?viewmonth=201101&viewday=21&style=flat
Here it says:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%">One more thing. the mtp-probe did not work on older ubuntu's than 10.10.
Further investigating showed that it fails at
LIBMTP_Check_Specific_Device () only because libusb does not populate
the correct dev->devnum (i.e. usb device number.
However if I manually run mtp-probe, later after a few seconds of device
plugged in, it works properly.
Looks like libusb takes a while to get the correct device number. Is
this a known libusb issue or something else?
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Might it be that we are hit by this issue? According to the link, newer versions of libusb should probably not have this issue.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Anyone familiar with this?</p></pre>
</blockquote>
<p>On March 17th, 2016, 6:48 p.m. UTC, <b>Dominik Haumann</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;">More on this, grepping the entire git repo of gvfs (https://github.com/GNOME/gvfs.git) shows that gvfs does not use LIBMTP_Check_Specific_Device() at all.
Similarly, grepping through simple-mtp-fs (https://github.com/phatina/simple-mtpfs) also shows no usage of LIBMTP_Check_Specific_Device().
However, both solutions use the other LIBMTP calls similarly to how kio-mtp does.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">What if we really just omit this check. The worst thing that can happen is that we iterate over all device numbers and get no hit, which just takes a bit longer.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Question would be: Do we get too many devices, as Kai was saying? No idea, but this really solves the issue for many users, then this might well be worth a try.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Opinions?</p></pre>
</blockquote>
<p>On March 19th, 2016, 7:13 p.m. UTC, <b>Dominik Haumann</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 asked ok libmtp-discuss (https://sourceforge.net/p/libmtp/mailman/message/34948820/) and the answer is as follows:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%">This <span style="color: #008000; font-weight: bold">function</span> probes an USB device <span style="color: #008000; font-weight: bold">for</span> MTP specific properties, <span style="color: #008000; font-weight: bold">in</span> some form
of MTP property auto detection. This auto<span style="color: #666666">-</span>detection does not necessarily work <span style="color: #008000; font-weight: bold">for</span> all MTP devices,
as it is was never well defined or documemted.
It also does not check against our existing USB ID database.
If you have a device that is <span style="color: #008000; font-weight: bold">in</span> our USB ID database already,
LIBMTP_Detect_Raw_Devices will <span style="color: #008000; font-weight: bold">do</span> the by<span style="color: #666666">-</span>id detection (and you <span style="color: #008000; font-weight: bold">do</span> that later <span style="color: #008000; font-weight: bold">in</span> the code).
Only as fallback you should <span style="color: #008000; font-weight: bold">try</span> LIBMTP_Check_Specific_Device().
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In other words, following this reasoning, we should not use LIBMTP_Check_Specific_Device() at all as an initial check/filter, meaning that he patch is correct.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't think I will find more info about this issue without much more investigation. But given the infos we have, I would like to commit - thoungs?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">@Kai: You were talking about 3 devices that are listed, only one of these working. Can you test or comment on this again?</p></pre>
</blockquote>
</blockquote>
<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 got another reply, stating</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%">That call is in libmtp for one single reason, and it is for the udev helper
program mtp-detect to probe a device to see if it is MTP if all other
checks (like against the existing device database) have failed.
Using it in any other code is not adviced, use the detection for raw
devices as Marcus points out.
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In other words: This patch should be correct:
- it adds a nullptr check (probably not requried, but won't hurt)
- it removes the buggy call of LIBMTP_Check_Specific_Device().</p></pre>
<br />
<p>- Dominik</p>
<br />
<p>On March 16th, 2016, 4:41 p.m. UTC, Dominik Haumann 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, David Faure and Philipp Schmidt.</div>
<div>By Dominik Haumann.</div>
<p style="color: grey;"><i>Updated March 16, 2016, 4:41 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=325335">325335</a>
</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;">Right now, Windows Phones (Nokia HW) is not properly recognized by kio-mtp.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It seems the device is listed but dropped for some reason. This issues exists for quite a long time (also in KDE 4.x), and some people provided this patch (from bug #325335).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I personally cannot test this patch, as I don't own a Windows Phone.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">However, given this patch works successfully for at least some users (see also https://www.reddit.com/r/kde/comments/4aggg0/error_with_kio_while_trying_to_open_lumia_device/), I think we at least should give it a try.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Adding group 'kdeframeworks' to reach a broader audience.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Would be cool to get a proper review for this patch, or even someone who owns such a device!</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;">Compiles...</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>mtp/devicecache.cpp <span style="color: grey">(8c5c7cf)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/127386/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>