D25682: [WIP] add initial wsdiscovery support
David Faure
noreply at phabricator.kde.org
Sat Dec 7 00:27:52 GMT 2019
dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.
I guess you were expecting a higher-level review, but I don't know anything about these protocols.
I'm glad to see my KDSoap library being useful in KDE too though :-)
INLINE COMMENTS
> discovery.cpp:21
> +
> +#include "discovery.h"
Not sure this .cpp file serves any purpose right now ;)
But actually the virtual destructors could be implemented here out-of-line to avoid being generated in every user of the class. The `=default` syntax would work here too.
> discovery.h:2
> +/*
> + Copyright 2019 Harald Sitter <sitter at kde.org
> +
What happened to the '>' character?
[repeats]
> discovery.h:40
> + virtual ~Discovery() = default;
> + virtual void toEntry(KIO::UDSEntry &entry) = 0;
> +};
Why don't these `toEntry()` methods *return* a KIO::UDSEntry instead?
It's always empty on incoming anyway.
And UDSEntry supports moving so the "return" statement won't make a copy.
> discovery.h:51
> + virtual void stop() = 0;
> + virtual bool isFinished() = 0;
> +
const?
> dnssddiscoverer.cpp:46
> + // that assumption will be true all the time. ~sitter, 2018
> + QUrl u(QStringLiteral("smb://"));
> + u.setHost(m_service->hostName());
`u.setScheme(QStringLiteral("smb"))` would be slightly faster; you're constructing a URL from its parts, no need to trigger parsing.
> dnssddiscoverer.cpp:71
> + // RemoteService::Ptr is useless here.
> + for (const auto &it : m_services) {
> + if (*service == *it) {
qAsConst
> dnssddiscoverer.cpp:103
> + m_browser.disconnect();
> + for (auto service : m_services) {
> + service->resolve(); // Blocks until resolution happened. Our signal handle then jumps in.
qAsConst
> dnssddiscoverer.h:41
> +
> +class DNSSDDiscoverer : public QObject, public virtual Discoverer
> +{
Why `virtual`? I thought this only mattered for diamond-shaped inheritance.
(OTOH I remember it leads to strange order of ctors being called, so I avoid it as much as possible)
> .gitlab-ci.yml:2
> +fedora:
> + image: registry.gitlab.com/caspermeijn/docker-images/fedora-build-onvifviewer:latest
> + stage: build
(is this meant to go into kde git? just wondering)
> CMakeLists.txt.user:3
> +<!DOCTYPE QtCreatorProject>
> +<!-- Written by QtCreator 4.9.1, 2019-08-07T17:08:05. -->
> +<qtcreator>
Now this one I'm sure, should NOT go into git.
> CMakeLists.txt.user:205
> + </valuelist>
> + <value type="QString" key="ProjectExplorer.BuildConfiguration.BuildDirectory">/home/me/src/git/soapy/build-kdsoap-ws-discovery-client-Desktop-Release-with-Debug-Information</value>
> + <valuemap type="QVariantMap" key="ProjectExplorer.BuildConfiguration.BuildStepList.0">
... because it's about your config :)
> wsdiscoverytargetservice.cpp:38
> +
> +bool WSDiscoveryTargetService::isMatchingType(const KDQName &matchingType)
> +{
const
(this will avoid detaching m_typeList)
> wsdiscoverytargetservice.cpp:48
> +
> +bool WSDiscoveryTargetService::isMatchingScope(const QUrl &matchingScope)
> +{
same
> kio_smb_browse.cpp:479
> + if (normalizedUrl.path().isEmpty()) {
> +#pragma message "refactor this entire function into less of a long pile of madness"
> + qDebug() << "Trying modern discovery (dnssd/wsdiscovery)";
lol
> kio_smb_browse.cpp:494
> + connect(&sendTimer, &QTimer::timeout, this, [&] {
> + if (list.size() < 1) {
> + return;
isEmpty() is more readable.
> kio_smb_browse.cpp:503
> + auto enterDiscovery = [&](Discovery::Ptr discovery) {
> + discovery->toEntry(udsentry);
> + list.append(udsentry);
With my suggestion to return a udsentry, this whole lambda becomes
`list.append(discovery->toEntry())`
(another move, no copy)
> kio_smb_browse.cpp:511
> +
> + QList<Discoverer *> discoverers {&d, &w};
> +
const QList.... to avoid detaching in range-for below
> kio_smb_browse.cpp:519
> + }
> + allFinished &= discoverer->isFinished();
> + }
never use &= for booleans, it's not meant for that
(it won't work for '1' and '2' which are both 'true' for booleans)
`allFinished = allFinished && discoverer->isFinished()`
Unfortunately there's no &&= in C++.
> wsdiscoverer.cpp:116
> + qWarning() << response.faultAsString();
> + // No return! We'd disqualify systems that do not implement pbsd.
> + }
OK. But maybe an `else` then ?
Not much point in using response.childValues() on a fault, i.e. iterating over the fault details.
> wsdiscoverer.cpp:128
> + QString computer;
> + for (auto section : response.childValues()) {
> + computer = section
Needs a const local var to avoid a detach. Yes, range-for is annoying for Qt containers.
> wsdiscoverer.cpp:160
> +
> + const QHostAddress address(m_endpointUrl.toString());
> + qDebug() << "++++++++++++++++++++++++++++++++++++++++++++";
unused
> wsdiscoverer.cpp:209
> + // We do set a suitable timeout in the resolver so this doesn't take forever.
> + for (auto future : m_futures) {
> + future.waitForFinished();
qAsConst
auto & ?
> wsdiscoverer.cpp:252
> + addr = xAddr;
> +#warning only get first val we only need one addr
> + }
then rewrite this code to `.at(0)` ?
> wsdiscoverer.cpp:257
> +
> + // Probably should just qobject this. Hardly worth the threading.
> + m_futures << QtConcurrent::run([=] {
especially with the data races.
(service->endpointReference() reads data written by another thread, without mutex)
> wsdiscoverer.cpp:280
> + entry.fastInsert(KIO::UDSEntry::UDS_ICON_NAME, "network-server");
> + entry.fastInsert(KIO::UDSEntry::UDS_ICON_OVERLAY_NAMES, "/home/me/.local/share/icons/Windows10Icons/32x32/places/ubuntu-logo.png");
> +
Obviously unfinished :-)
> wsdiscoverer.h:48
> +
> +class WSDiscoverer : public QObject, public virtual Discoverer
> +{
(same question about virtual)
REPOSITORY
R320 KIO Extras
REVISION DETAIL
https://phabricator.kde.org/D25682
To: sitter, dfaure, #frameworks, #dolphin
Cc: ngraham, caspermeijn, davidedmundson, kde-frameworks-devel, kfm-devel, pberestov, iasensio, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, andrebarros, bruns, emmanuelp, mikesomov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20191207/cf373523/attachment-0001.html>
More information about the Kde-frameworks-devel
mailing list