<div dir="ltr">Hi Thiago,<div><br></div><div>Thanks for making time to look over the patches!</div><div><br></div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> Subject: [PATCH 03/17] Use only the BTH address to check if the device was<br>
> already added<br>
><br>
> There are moments when the name of the device cannot be obtained.<br>
> Therefore we should use only the Bluetooth address to identify if<br>
> the discovered device was already added to the list.<br>
<br>
</span>> - QList<QListWidgetItem *> itemsWithSameSignature =<br>
> ui->discoveredDevicesList->findItems(deviceLabel, Qt::MatchStartsWith);<br>
> + QList<QListWidgetItem *> itemsWithSameSignature =<br>
> ui->discoveredDevicesList->findItems(remoteDeviceInfo.address().toString(),<br>
> Qt::MatchContains);<br>
<br>
This is a little confusing. I understand that the device is added to the list<br>
first as a MAC address only and then replaced by the label. But I'm not sure if<br>
the fix is correct... where is the corresponding addition to the list in the<br>
first place? How can we be sure that the change to a label hasn't happened?<br></blockquote><div><br></div><div>Sorry for the confusion but after some discussions with Dirk I removed that commit.</div><div>All the new patches were included in my latest e-mail from this thread. Also you can</div><div>find them in this branch [1].</div><div><br></div><div>The only difference is that patch number 3 was replaced with this one [2] and </div><div>I added a new one [3].</div><div><br></div><div>Now I clear the list with the discovered devices when a new device lookup is </div><div>initiated.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> Subject: [PATCH 05/17] Add skeleton for Bluetooth custom serial<br>
> implementation<br>
> on Windows platforms<br>
<br>
This patch, by itself, doesn't do anything, except:<br>
<br>
> BtDeviceSelectionDialog::~BtDeviceSelectionDialog()<br>
> {<br>
> delete ui;<br>
><br>
> +#if defined(Q_OS_WIN)<br>
> + // Terminate the use of Winsock 2 DLL<br>
> + WSACleanup();<br>
> +#else<br>
<br>
This will mess up the winsock DLL internal refcounting, so it could<br>
accidentally shut down all sockets in Subsurface... I know the matching<br>
WSAStartup is in patch 12, but this means that patches 5 through 11 are not<br>
testable as they (potentially) break Winsock2. I'm not talking about testing<br>
the Bluetooth code (the initializeDiscoveryAgent code isn't added until patch<br>
17). This is about the whole Subsurface. The WSACleanup bit should be moved to<br>
patch 12.<br></blockquote><div><br></div><div>Ok, I will move the WSACleanup call in the same patch where WSAStartup is used.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Please initialise the stopped and running variables. In patch 11, when you<br>
actually use them, the stopped variable is used uninitialised, in:<br>
<br>
while (result == SUCCESS && !stopped){<br>
<br>
As for the running variable, the isActive function may race against the<br>
initialisation, so please initialise it too.<br></blockquote><div><br></div><div>I had that in mind but I totally forgot :). I will do the initialization. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> Subject: [PATCH 06/17] Add implementation for BTH custom serial open method<br>
> on<br>
> Windows platforms<br>
<br>
> @@ -51,7 +51,49 @@ static int qt_serial_open(serial_t **out, dc_context_t<br>
> *context, const char* dev<br>
> + char *address = strdup(devaddr);<br>
<br>
There doesn't seem to be any need for strdup here.<br></blockquote><div><br></div><div>Yes, you are right.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> Subject: [PATCH 10/17] Add internal method which returns a pretty message<br>
> about the last error on Windows<br>
<br>
Hint: you could have used qt_error_string(). That does exactly what you did in<br>
your code.<br></blockquote><div><br></div><div>Thanks for the hint! I will remove the internal function and use the one you </div><div>suggested.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> Subject: [PATCH 11/17] Add implementation for our custom BTH device<br>
> discovery<br>
> service<br>
<br>
> + WSAQUERYSET *pResults = (WSAQUERYSET*)&buffer;<br>
[...]<br>
> + if (WSAAddressToStringA((LPSOCKADDR)<br>
> socketBthAddress,<br>
[...]<br>
> + QString deviceName =<br>
> QString(pResults->lpszServiceInstanceName);<br>
> + QString deviceAddress = QString(addressAsString);<br>
<br>
I'm not going to hold you to this, so this is just learning for next time.<br>
<br>
It's usually a good idea to use the W functions on Windows instead of the A<br>
ones. First of all, they are faster, since they are the real native functions<br>
-- the A functions need to convert from wchar_t to char back and forth. Not to<br>
mention that this conversion is lossy, so the A functions cannot represent all<br>
possibilities.<br>
<br>
Second, they are supported on Windows CE, whereas the A functions aren't. Not<br>
really important to us, but it gets you in the right habit.<br>
<br>
And third, you would avoid an UTF-8 decode in those two QString creations --<br>
instead, you could have used QString::fromWCharArray or QString::fromUtf16,<br>
which are simple memcpy. Or, in the case of addressAsString, you could have<br>
used QString itself as your buffer:<br>
<br>
QString deviceAddress(BTH_ADDR_STR_LEN, Qt::Uninitialized);<br>
if (WSAAddressToString(....<br>
reinterpret_cast<wchar_t*>(deviceAddress.data()),<br>
...<br>
deviceAddress.truncate(addressSize);<br></blockquote><div><br></div><div>Thank you for your detailed explanation! I didn't know about the difference </div><div>between the W functions on Windows instead of the A ones.</div><div>Since I will create a new set of patches I will try to add all the things you </div><div>suggested.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Please merge as much of the code as you can. The first two (or more) lines of<br>
the two branches of the #if are identical, so it's a good idea to keep them<br>
together to avoid code duplication.<br></blockquote><div><br></div><div>I know that there is code duplication but I thought that it would be easier </div><div>to follow the implementation :).</div><div><br></div><div>Have a nice day,</div><div>Claudiu </div></div></div><div class="gmail_extra"><br></div><div class="gmail_extra">[1] - <a href="https://github.com/claudiuolteanu/subsurface/commits/bth_windows">https://github.com/claudiuolteanu/subsurface/commits/bth_windows</a></div><div class="gmail_extra">[2] - <a href="https://github.com/claudiuolteanu/subsurface/commit/9ae338fdfe28c6707e98442ebe587a4b22d6809c">https://github.com/claudiuolteanu/subsurface/commit/9ae338fdfe28c6707e98442ebe587a4b22d6809c</a></div><div class="gmail_extra">[3] - <a href="https://github.com/claudiuolteanu/subsurface/commit/08d3c3a6ebfa69c41810d2af158fb2808a0c0a11">https://github.com/claudiuolteanu/subsurface/commit/08d3c3a6ebfa69c41810d2af158fb2808a0c0a11</a></div></div>