[WIP] add initial wsdiscovery support
Needs ReviewPublic

Authored by sitter on Mon, Dec 2, 1:50 PM.

Details

Reviewers
dfaure
Group Reviewers
Frameworks
Dolphin
Summary

needs kdsoap >= 1.8.50 (current master).
builds a static variant of kdsoap-ws-discovery-client but can also
use system's version if available.

also reinvents how discovery works:
there are now Discoverers for dnssd and wsd that get started/stopped.
the Discoverers discover servers and emit them as Discoveries. Discoveries
are then converted to udsentries for kio.

todo:

  • cleanup pragmas/warnings
  • cleanup qdebugs and/or categorize
  • split the browse function to ease readability
  • cleanup includes
  • get rid of icon overlays
  • figure out a way to get better context menus for non-file entries. this is a general problem with KIO. because it only understands unix dirent types the context menus for artificial things as appearing in discovery slaves (smb and network for instance) are lackluster

BUG: 392447
FIXED-IN: 20.04.0

Test Plan

discover all the things!

Diff Detail

Repository
R320 KIO Extras
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 19620
Build 19638: arc lint + arc unit
sitter created this revision.Mon, Dec 2, 1:50 PM
Restricted Application added projects: Dolphin, Frameworks. · View Herald TranscriptMon, Dec 2, 1:50 PM
Restricted Application added subscribers: kfm-devel, kde-frameworks-devel. · View Herald Transcript
sitter requested review of this revision.Mon, Dec 2, 1:50 PM
ngraham edited the summary of this revision. (Show Details)Mon, Dec 2, 2:27 PM

Why do we need to mirror this dsoap-ws-discovery-client lib that seems to be copied from somewhere?

Why do we need to mirror this dsoap-ws-discovery-client lib that seems to be copied from somewhere?

Testing convenience until @caspermeijn makes a release mostly. The library was,or maybe even still is, in flux as API was being shuffled up to kdsoap.

ngraham added a subscriber: ngraham.Mon, Dec 2, 3:22 PM

OMG it works!!! I can find my windows Samba share in Dolphin again!

Why do we need to mirror this dsoap-ws-discovery-client lib that seems to be copied from somewhere?

Testing convenience until @caspermeijn makes a release mostly. The library was,or maybe even still is, in flux as API was being shuffled up to kdsoap.

Cool than my library can be used for this as well! The interface to KDSoap is in master now, so that only waits for a release of KDSoap. However I want to change some things in the kdsoap-ws-discovery-client API.

Please be aware of the license of the WSDL files. It explicitly states "this document itself may not be modified in any way", so I am not sure what this will to with the license of the binary.

dfaure requested changes to this revision.Sat, Dec 7, 12:27 AM

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 :-)

smb/discovery.cpp
22

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.

smb/discovery.h
3

What happened to the '>' character?

[repeats]

41

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.

52

const?

smb/dnssddiscoverer.cpp
47

u.setScheme(QStringLiteral("smb")) would be slightly faster; you're constructing a URL from its parts, no need to trigger parsing.

72

qAsConst

104

qAsConst

smb/dnssddiscoverer.h
42

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)

smb/kdsoap-ws-discovery-client/.gitlab-ci.yml
3

(is this meant to go into kde git? just wondering)

smb/kdsoap-ws-discovery-client/CMakeLists.txt.user
3 ↗(On Diff #70750)

Now this one I'm sure, should NOT go into git.

205 ↗(On Diff #70750)

... because it's about your config :)

smb/kdsoap-ws-discovery-client/src/wsdiscoverytargetservice.cpp
39

const

(this will avoid detaching m_typeList)

49

same

smb/kio_smb_browse.cpp
484

lol

499

isEmpty() is more readable.

508

With my suggestion to return a udsentry, this whole lambda becomes

list.append(discovery->toEntry())

(another move, no copy)

516

const QList.... to avoid detaching in range-for below

524

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++.

smb/wsdiscoverer.cpp
117

OK. But maybe an else then ?
Not much point in using response.childValues() on a fault, i.e. iterating over the fault details.

129

Needs a const local var to avoid a detach. Yes, range-for is annoying for Qt containers.

161

unused

210

qAsConst

auto & ?

253

then rewrite this code to .at(0) ?

258

especially with the data races.

(service->endpointReference() reads data written by another thread, without mutex)

281

Obviously unfinished :-)

smb/wsdiscoverer.h
49

(same question about virtual)

This revision now requires changes to proceed.Sat, Dec 7, 12:27 AM

With regards to the Docker/Gitlab CI part, please use the images under kdeorg/ on Dockerhub rather than personally maintained images as the wider community has no access to your namespace on Gitlab.com

(Additionally please note that Fedora is not permitted to be used on our infrastructure)

sitter marked 26 inline comments as done.Mon, Dec 9, 3:34 PM

Yep, still looking for primarily design input.

Thanks for your comments, David!

smb/discovery.h
41

Good point. We used to pass it around before, but it indeed doesn't make much sense.

smb/dnssddiscoverer.h
42

Left over from something I tried, not actually necessary indeed.

smb/kdsoap-ws-discovery-client/.gitlab-ci.yml
3

No. Well, I mean, it's a copy of the upstream repo with only the actually necessary delta applied (STATIC internal build as opposed to SHARED).

smb/kdsoap-ws-discovery-client/CMakeLists.txt.user
3 ↗(On Diff #70750)

Yeah. Slipped into git add.

smb/kdsoap-ws-discovery-client/src/wsdiscoverytargetservice.cpp
39

Forwarded to upstream repo.

smb/wsdiscoverer.cpp
210

waitForFinished is not const. I've made the container qAsConst though.

253

That's the plan, I need to read up on when exactly there'd be more than one xaddr though. While we need only one I haven't read up if any xaddr will work all the time.

258

promoted the comment to a warning for now so I don't forget to deal with it before landing

281

Yep. Only there to easily see which service produced which device entry for debugging ;)

sitter updated this revision to Diff 71121.Mon, Dec 9, 3:35 PM
sitter marked 9 inline comments as done.

various fixes suggested by dfaure