- Adds two new job flags, PrivilegeExecution and NoPrivilegeExecution.
- Adds code for confirmation dialog to be shown when the slave asks for it.
- Provides connection for privilegeOperationRequested emmited by the slave.
Details
- Reviewers
dfaure - Group Reviewers
Frameworks - Maniphest Tasks
- T6561: Polkit support in KIO
- Commits
- R241:596ef4ccf1d7: Integrate new file ioslave in KIO job
Diff Detail
- Repository
- R241 KIO
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage
src/core/job.cpp | ||
---|---|---|
345 | This hardcoded 5 is awful, please use SlaveBase::Continue instead. | |
src/core/job_base.h | ||
222 | This doesn't need to be public, does it? Could be in the private class. | |
229 | This doesn't need to be public, does it? Could be in the private class. | |
src/core/job_p.h | ||
94 | Please keep all the member variables together. In the orig code it's already ugly that there are methods after the variables, but this makes it slightly worse with the enum in the middle and no empty line between vars and methods. Ideally this should be | |
195 | typo: emitted | |
src/core/simplejob.cpp | ||
151 | This is not a very good signal name. | |
346 | use QLatin1String rather than QStringLiteral for comparisons | |
354 | OK, so the data is two possible strings, and the reply is two possible strings, why not use enums instead? You can define them in SlaveBase for instance. You might have to use lambdas to avoid the enum appearing in Q_PRIVATE_SLOT in public headers (and requiring slavebase.h) |
- Separated enums, vars and functions
- Removed hardcoding
- Use the new signal privilegeOperationRequested
- Removed public methods added in previous revision. Instead of them d_func is used (which should have been the case right from the start but I overlooked it somehow)
Looks good, apart from a few minor things.
src/core/job.cpp | ||
---|---|---|
275 | its -> it's | |
src/core/job_p.h | ||
198 | Private header -> no need for @since doc, it's not part of the API. | |
src/core/simplejob.cpp | ||
346 | I'm not sure if the bool->int conversion is safe and guaranteed (AFAIK, in theory a bool can convert to 2 or anything else that is not 0, depending on how it was set in the first place) This would be safer if it said QByteArray::number(confirmed ? 1 : 0), but then that's even better written as confirmed ? "1" : "0", no need to call int->string conversion code; |
- Fixed the issues pointed out.
- Check for UnitTesting key in metadata while showing the confirmation dialog.
src/core/simplejob.cpp | ||
---|---|---|
346 | Check that this is consistent with SlaveBase... |
1.Reassigned flag values for NoPrivilegeExecution and PrivilegeExecution. I strongly feel that the latter won't be needed anymore.
2.Setting meta-data for unit test in the job itself (in tryAskPrivilegeOpConfirmation).
3.Changed some comments due to changing flags .
Please keep in mind that we should remove one of the two flags before the end of the month, after people tested this as much as possible ;)
I admit that it sucks a bit that I don't know what the default should be...
Conservatively, I think opt-in is better, since the worst case is an app that regularly does a failing copy in the background, and it would now prompt the user every time... well, maybe this doesn't happen anywhere though ;)
Anyhow, let's first get this in. Thanks for your perseverance!