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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
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
1541

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

src/core/slavebase.h
1042

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
1042

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
273

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
1026

, 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
273

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
1557

#if KIOCORE_BUILD_DEPRECATED_SINCE(5, 65)

(notice BUILD, not ENABLE)

src/core/slavebase.h
1029

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

1030

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

src/core/slaveinterface.h
89 ↗(On Diff #70914)

unrelated/unneeded anymore

src/core/slaveinterface_p.h
55

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
379

two spaces after =, remove one

381

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

src/widgets/jobuidelegate.h
156

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
55

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
156

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
291

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
260–271

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

1012

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
260–271

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.