D24887: WIP slave command behavior assertion system

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  After much delay.... some replies.... but there are yet more questions in here....

INLINE COMMENTS

> commands_p.h:27
>  {
> +#pragma message "this is awkward since this causes symbols on the public KIO entity"
> +Q_NAMESPACE

parse error

> slavebase.cpp:96
> +
> +#pragma message "pretty sure this isn't safe WRT static init order"
> +/**

why? does another global ctor use it?

It's safe. It's just costly -- happens in each and every process that links to KIO.

Maybe this should be all in NDEBUG anyhow?

> slavebase.cpp:128
> +        //   * position lacks any documentation
> +        // * trash listdir calls totalsize() (via listroot)
> +        // * applications listdir calls totalsize()

In order to get progress information when listing very large directories, the idea used to be "emit totalSize(number of files) from listDir()".

Then kdelibs4 commit 23ba25b6505dc7 <https://phabricator.kde.org/R446:23ba25b6505dc7f46f8a3a7a11088411216c8c00> changed the logic for the batching in KIO and this information is no longer necessary.

I just removed outdated docu from slavebase.h in commit 2261f2045c0a <https://phabricator.kde.org/R241:2261f2045c0a5a33c528bdc0deaf35790a410867> (the kdelibs4 commit removed most of it).

I'm wondering how ListJob can emit percent though, as comments over there say, without knowing about the totalsize. Something seems amiss here.
I can see how batching (based on time) doesn't need to know the total number of items, but progress information surely does...

And in fact AFAICS this information does end up in ListJob, and is used there to calculate progress information.
So I was wrong in today's commit, and that kdelibs4 commit was wrong in removing that docu too.
This is useful, and slaves should emit it.

The porting to KJob turned a simple int into something explicitly called "Bytes" though, so it looks even more like a hack than it did before (see SimpleJobPrivate::slotTotalSize, also called from ListJob). 
ListJob should have its own slot, and use Files instead of Bytes.

kio_file should also emit totalSize, 1) for slow network-mounted directories, 2) so that all this can be unittested.

One question, 3 TODOs, this is going to be a long patch to review ;-)

> slavebase.cpp:131
> +        // * settings listdir calls totalsize()
> +        // * desktop listdir calls processedsize()
> +        // * remote listDir calls totalSize()

Sounds wrong (i.e. but g in kio_desktop), listjob takes care of that.

> slavebase.cpp:137
> +#pragma message "deal with not codified expectations"
> +        // * during get mimetype should be emitted before data (req: mimetype mustn't be after data?)
> +        // * canResume has two variants that should be called at the beginning of get/put (req: mustn't be after data/written?)

Right. This allows to pick the app based on mimetype, and it will get all the data.

> slavebase.cpp:139
> +        // * canResume has two variants that should be called at the beginning of get/put (req: mustn't be after data/written?)
> +        // * listDir should call listEntry --> not really something to warn about, a dir may be empty
> +        // * stat should call statEntry --> unless error?

right

> slavebase.cpp:140
> +        // * listDir should call listEntry --> not really something to warn about, a dir may be empty
> +        // * stat should call statEntry --> unless error?
> +

right, on error it won't call statEntry

> slavebase.cpp:185
> +#pragma message "deal with commands I don't know expectations for"
> +        case CMD_SLAVE_CONNECT:
> +        case CMD_SLAVE_HOLD:

That's internal, doesn't get to SlaveBase.

> slavebase.cpp:186
> +        case CMD_SLAVE_CONNECT:
> +        case CMD_SLAVE_HOLD:
> +        case CMD_NONE:

same

> slavebase.cpp:187
> +        case CMD_SLAVE_HOLD:
> +        case CMD_NONE:
> +        case CMD_TESTDIR:

That's a reply (slave->app). No expectations, since it's the other way around.

> slavebase.cpp:188
> +        case CMD_NONE:
> +        case CMD_TESTDIR:
> +        case CMD_SPECIAL:

unused, should be removed in KF6

> slavebase.cpp:189
> +        case CMD_TESTDIR:
> +        case CMD_SPECIAL:
> +        case CMD_REPARSECONFIGURATION:

depends on the subcommand, as you documented in slavebase.h

> slavebase.cpp:190
> +        case CMD_SPECIAL:
> +        case CMD_REPARSECONFIGURATION:
> +        case CMD_META_DATA:

fire-and-forget command app->slave, no expectations.

> slavebase.cpp:191
> +        case CMD_REPARSECONFIGURATION:
> +        case CMD_META_DATA:
> +        case CMD_SUBURL:

handled internally in slavebase.cpp, doesn't get to the SlaveBase subclass

> slavebase.cpp:192
> +        case CMD_META_DATA:
> +        case CMD_SUBURL:
> +        case CMD_MESSAGEBOXANSWER:

Obsolete. There's much cleanup to be done around that stuff in KF6. Unless someone feels like reviving the support for chaining kioslaves using suburls... but IMHO this has all been dead for long.

> slavebase.cpp:193
> +        case CMD_SUBURL:
> +        case CMD_MESSAGEBOXANSWER:
> +        case CMD_RESUMEANSWER:

app->slave reply to the slave calling messageBox(). No expectations.

> slavebase.cpp:194
> +        case CMD_MESSAGEBOXANSWER:
> +        case CMD_RESUMEANSWER:
> +        case CMD_CONFIG:

also an app->slave reply to the slave asking

> slavebase.cpp:195
> +        case CMD_RESUMEANSWER:
> +        case CMD_CONFIG:
> +        case CMD_HOST_INFO:

handled internally, no expectations from kioslaves

> slavebase.cpp:196
> +        case CMD_CONFIG:
> +        case CMD_HOST_INFO:
> +            return {};

app->slave reply

> slavebase.cpp:844
>  {
> +#pragma message "worth adding verification to? -- also see expectations in forCommand()"
>      //qDebug() << "STUB";

there's nothing here.... This method should be removed IMHO

> slavebase.h:399
> +     *
> +     * @attention Must NOT call any conclusion signals (finished(), error()...)
>       */

All the documentation changes to slavebase.h are good to go, they could land independently from the rest of this commit...

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D24887

To: sitter, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200414/a8baf29a/attachment-0001.html>