Fix logic error in kioclient noninteractive argument handling
ClosedPublic

Authored by davidedmundson on Dec 18 2018, 12:57 PM.

Details

Summary

If noninteractive /is/ set s_interactive should be false.
When s_interactive is true we show dialog and kuiserver notifications.

This was a porting bug.

Given fixing would result in a behavioural change, the default behaviour
is swapped to remain the same as the previous broken version and an
explicit --interactive flag is added.

Test Plan

Ran a test that should produce an error without passing --noninteractive
It produced an error notification.

Diff Detail

Repository
R126 KDE CLI Utilities
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 6129
Build 6147: arc lint + arc unit
Restricted Application added a project: Plasma. · View Herald TranscriptDec 18 2018, 12:57 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
davidedmundson requested review of this revision.Dec 18 2018, 12:57 PM

Given this has been broken for more than a decade (original commit from 2006 already had this bug), I think all users of this binary rely on the fact that calling kioclient5 with no arguments is non-interactive and doesn't cause and blocking or waiting for user input/confirmation.

Should we instead turn this around to be an explicit "interactive" flag?

That's a good point. So we would have an interactive flag and a working noninteractive flag, but change the default to be non-interactive so that the no args case remains the same?

I'll tag David Faure (given his plane didn't crash after 39f61b5114ef835230eda9d36e85dc4e91c49731) and see what he says.

No, the commit in 2006 wasn't buggy. There was special logic in KCmdLineArgs for the case of the "no" prefix (note how it said "ninteractive", the "no" was toggling the value). This is a porting error when going from KCmdLineArgs to QCommandLineParser.
So it's "only" buggy since commit 48a9f87 in 2014 :-)

I like the idea of adding --interactive to avoid surprising existing users.

There's a new twist!

Even in non interactive mode we still got dialogs for file conflicts. See https://phabricator.kde.org/D17703

Because of this the current state isn't non-interactive. It's a horrible mess of both.
Whatever we do will result in a behavioural change for users.

Good find. However for users who didn't want a progress dialog, and who never got a conflict (e.g. if they were copying into an empty dir), kioclient was non-interactive, so I think our previous reasoning still holds.

note how it said "ninteractive", the "no" was toggling the value).

I assumed it was a typo. Yay :)

Make non-interactive default

Looks good except for the TODO comment. I think the comment that is actually needed is one about why we have a "noninteractive" option and it has no effect whatsoever ;)

kioclient/kioclient.cpp
276

Why would we want to break compatibility yet again? ;-)

Why would we want to break compatibility yet again? ;-)

It's the better default.
I've removed the TODO, but I've deliberately not said which is default in the --help output which hopefully will encourage people to still type --noninteractive if they genuinely want that which allows us to keep our options open.

davidedmundson edited the summary of this revision. (Show Details)

update

dfaure accepted this revision.Dec 20 2018, 11:44 PM

I'm not sure anymore what the best default is. There are two different use cases, that's all.

This revision is now accepted and ready to land.Dec 20 2018, 11:44 PM
This revision was automatically updated to reflect the committed changes.