Show more details in warning dialog shown before starting a privileged operation
ClosedPublic

Authored by chinmoyr on Jun 13 2019, 4:13 PM.

Details

Summary

Currently it uses the dialog from D21782 to provide the current action being performed, the source file and any
action specific detail.

The initial plan was to show all the files needing elevated privileges but due to KIO jobs processing one file at
a time only limited info could be provided. There's also the option for determing this during the stat phase but it
caused my VM to slow down to a crawl. Any more ideas here?

Diff Detail

Repository
R241 KIO
Branch
arcpatch-D21783
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17705
Build 17723: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a project: Frameworks. · View Herald TranscriptJun 13 2019, 4:13 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
chinmoyr requested review of this revision.Jun 13 2019, 4:13 PM

Thanks! Is this for T8075?

Thanks! Is this for T8075?

Yes it is.

Totally awesome :)

chinmoyr planned changes to this revision.Jun 15 2019, 8:08 AM

TODO

  • Change the UI of the warning dialog
  • We still need more details
dfaure requested changes to this revision.Jun 22 2019, 8:25 AM
dfaure added inline comments.
src/core/slavebase.cpp
1511

INF is for one-way information, isn't it? Why isn't this MSG_ like the one above?

src/core/slavebase.h
972

Missing "m" prefix like the other members, but wait.... isn't this BIC? Adding a new member to an exported class certainly is. This needs to go into the SlaveBasePrivate class instead.

src/ioslaves/file/file_unix.cpp
85

all those { ... } braces are unnecessary in this switch, which doesn't define any new variables

chinmoyr updated this revision to Diff 67955.Oct 15 2019, 9:43 AM
chinmoyr marked 3 inline comments as done.

Addressed the comments.

chinmoyr added inline comments.Oct 15 2019, 9:44 AM
src/core/slavebase.h
972

Since we need details from the ioslave somehow, I deprecated requestPrivilegeOperation() in favor of an overload that accepts details as argument.

dfaure requested changes to this revision.Oct 16 2019, 8:58 PM
dfaure added inline comments.
src/core/jobuidelegateextension.h
279

Sorry, you can't add a new virtual method to an exported+installed class.

BIC rules (where "rules" is a noun, not a verb...) ;-)

Possible solutions:

  • creating a new, separate, type of extension, say JobUiDelegatePrivilegeExtension. This requires adding another defautFoo() and setFoo() method like this file has at the bottom.
  • creating a "JobUiDelegateExtensionV2", derived from JobUiDelegateExtension, with a KF6 TODO to merge it with JobUiDelegateExtension. Then you can dynamic_cast the result of defaultJobUiDelegateExtension() to V2 to see if the extension implements V2.
src/core/slavebase.h
956

, use requestPrivilegeOperation(QString)

src/ioslaves/file/file_unix.cpp
81

const QVariantList &

87

no space before ':' in English

91

same

121

weird indentation

This revision now requires changes to proceed.Oct 16 2019, 8:58 PM
chinmoyr updated this revision to Diff 70914.Dec 4 2019, 5:34 PM
chinmoyr marked 4 inline comments as done.

Addressed the issues.

ngraham accepted this revision as: VDG.Dec 4 2019, 5:36 PM

UI looks fine to me. @dfaure?

chinmoyr added inline comments.Dec 4 2019, 6:15 PM
src/core/jobuidelegateextension.h
279

Sorry, you can't add a new virtual method to an exported+installed class.

BIC rules (where "rules" is a noun, not a verb...) ;-)

LOL! Either way it's true.

Possible solutions:

*creating a new, separate, type of extension, say JobUiDelegatePrivilegeExtension. This requires adding another defautFoo() and setFoo() >method like this file has at the bottom.
*creating a "JobUiDelegateExtensionV2", derived from JobUiDelegateExtension, with a KF6 TODO to merge it with JobUiDelegateExtension. >Then you can dynamic_cast the result of defaultJobUiDelegateExtension() to V2 to see if the extension implements V2.

IMO trying to pass job details from SlaveBase to KMessageBox without breaking compatibility would result in an unnecessarily large patch.
So I went ahead with reusing sslMetaData which was hacky but the resulting patch wasn't an eye-sore. I hope we can keep this and later add proper API in KF6.

dfaure requested changes to this revision.Dec 6 2019, 8:19 AM
dfaure added inline comments.
src/core/slavebase.cpp
1528

#if KIOCORE_BUILD_DEPRECATED_SINCE(5, 65)

(notice BUILD, not ENABLE)

src/core/slavebase.h
959

The modern way is #if KIOCORE_ENABLE_DEPRECATED_SINCE(5, 65)

960

KIOCORE_DEPRECATED_VERSION(5, 65, "Pass QString action to requestPrivilegeOperation")

src/core/slaveinterface.h
89–90

unrelated/unneeded anymore

src/core/slaveinterface_p.h
56 ↗(On Diff #70914)

Please explain how, for when the time comes (in case you're not around to do it).

src/ioslaves/file/file_unix.cpp
81

marked as done, but not done

84

missing space after switch (same rule as if, for etc.)

src/widgets/jobuidelegate.cpp
375

two spaces after =, remove one

377

I don't think you need the KMessageBox::Options( around the value?

src/widgets/jobuidelegate.h
155

what would be the proper API you have in mind?

This revision now requires changes to proceed.Dec 6 2019, 8:19 AM
chinmoyr updated this revision to Diff 71025.Dec 6 2019, 4:44 PM
chinmoyr marked 10 inline comments as done.

Addressed the issues.
Annotated code to be removed in KF6.
My build env is outdated so I couldn't compile the new depracation macro. Copied as it is.

chinmoyr added inline comments.Dec 6 2019, 4:46 PM
src/core/slaveinterface_p.h
56 ↗(On Diff #70914)

Removing each of its occurence (of which there are 3) and the surrounding if. I will add comments where its needed to be removed.

src/widgets/jobuidelegate.h
155

I think adding a QString parameter for operation details should be sufficient. Then the API should be kept in sync with SlaveBase, SalveInterface, and JobUiDelegateExtension.

dfaure requested changes to this revision.Dec 6 2019, 11:41 PM

I'm concerned that you didn't compile this (because of dependency issues, from what I gather), which means it's not tested either.

I tried to compile it but it doesn't cleanly apply to git master. Can you rebase it?

src/core/slaveinterface.cpp
295

This fails to build for me...

slaveinterface.cpp:291:55: error: ‘QString::QString(const char*)’ is private within this context

It needs QStringLiteral like the previous one.

src/ioslaves/file/file_unix.cpp
87

Not done

91

same

121

still there

This revision now requires changes to proceed.Dec 6 2019, 11:41 PM
chinmoyr updated this revision to Diff 71041.Dec 7 2019, 2:49 AM

Rebased, compiled and tested. Changes work as expected.
Double checked file_unix.cpp. Would be a shame if it happens yet again.

dfaure requested changes to this revision.Dec 14 2019, 11:09 PM
dfaure added inline comments.
src/core/slavebase.h
262

Does this break API docs generation? Suddenly the /** ... */ comment is separated from the enum.

942

I was too slow, it's 5.66 now. Please change it everywhere.

Well by waiting we didn't break the message freeze with all these new i18n calls.

This revision now requires changes to proceed.Dec 14 2019, 11:09 PM
chinmoyr updated this revision to Diff 71754.Dec 18 2019, 6:54 AM

updated kf5 version.

chinmoyr added inline comments.Dec 18 2019, 7:00 AM
src/core/slavebase.h
262

The comment didn't affect the doc generation. But the placement was awkward so I changed it.

dfaure accepted this revision.Dec 18 2019, 9:15 PM

Thanks!

This revision is now accepted and ready to land.Dec 18 2019, 9:15 PM

(remember to remove the [WIP] from the commit log, if you still have it locally)

ngraham retitled this revision from [WIP]Show more details in warning dialog shown before starting a privileged operation to Show more details in warning dialog shown before starting a privileged operation.Dec 21 2019, 8:44 PM
This revision was automatically updated to reflect the committed changes.

This changes FTBFS - see https://build.kde.org/job/Administration/job/Dependency%20Build%20Applications%20kf5-qt5%20SUSEQt5.12/47/
There are currently a large number of Dependency Builds underway to correct changes in the tree as a consequence of Grantlee changing to be a first party repository, so this needs to be corrected urgently.