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