Review Request: Make KAuth ready for frameworks + API Changes
Dario Freddi
drf at kde.org
Fri Mar 23 00:17:01 GMT 2012
> On March 18, 2012, 11:04 p.m., Stephen Kelly wrote:
> > Nice, thanks and sorry for the noise, and thanks for making the branch.
>
> Dario Freddi wrote:
> Np, hope you'll be able to have a quick look at it as well, it would be great :)
>
> Stephen Kelly wrote:
> Mostly it looks fine.
>
> The enums are not named consistently though. Sometimes you make it <enum name><enum value> (eg StatusDenied) and sometimes you use <enum value><enum name> (eg HelperBusyError).
> The Qt way would be <enum value><enum name>.
>
> Also, you replied that removed APIs are used nowhere. Are the enums used nowhere too? Is it worth keeping the backward compatibility enum names? Assuming your branch builds (I didn't try it) nothing else inside of kdelibs seems to need those enum values at least, so maybe it's not a big deal.
>
> I'm also generally impressed with how the commits are written to do one thing at a time, so thanks for that. I wonder if the fixes to ExecuteJob should be squashed though as well as porting it to KJob? It's not clear to me if those commits are separate because of something in an intermediate commit? Not very important either way. I don't mind if you change them or not.
>
> Some of the files in the unit tests appear to be old. Are they copied from somewhere? Could they be moved instead?
>
> Stephen Kelly wrote:
> Regarding what you asked about:
>
> * Consistency of the enums. StatusInvalid vs. ExecuteMode vs. AuthorizationDeniedError. While the semantic seems correct to me, I'd like to have some feedback on whether consistency is valuable in the ordering of <type><value> vs. <value><type> and which one should be preferred in case.
>
> I guess I prefer the Qt style.
>
> * Whether to deprecate static accessors such as static const ActionReply SuccessReply(). I strongly favor this.
>
> If you know that they are not much used or easily portable, I'd say go for it.
>
> * Whether the new dependency of kcoreaddons for the sake of using KJob is reasonable or I should go for a different alternative.
>
> I think it's an ok dependency. I still hope that class will move to a different framework at some point if we can find other classes that it would belong with ('base-asyncronous APIs'?). 'addons' is a forbidden name if the result of Randa is followed.
>
> * CMake sanity for the new dependency of kcoreaddons.
>
> That's fine, yes.
>
>
> Kevin Ottens wrote:
> Result pretty much aligns with what I was expecting as outcome from our previous private discussion. And so, apart from the points Stephen already raised I see nothing outstanding now.
>
> Dario Freddi wrote:
> Enums: will change all of them to be <value><name> (InvalidStatus, ExecuteMode, AuthorizationDeniedError).
>
> Static accessors: they are easily portable (one should use SuccessReply() compared to previous SuccessReply) and quite used widely. I'd like people to use ActionReply(ActionReply::SuccessType) instead, although they are indeed widely used in helpers. Quite torn on this one - now they're safe to use though, so I guess that besides adding lots of symbols for the sake of nothing, they won't hurt. What's your take?
>
> Squashing ExecuteJob's commits might be a good idea now that it's clear we're going to use KJob as a base class. Will also take care of amending other commits for the sake of clarity.
>
> The enums are not used exactly because the static accessors are used instead. The only enums that might be used around are the error ones, but again it's not something as big to justify the need for having to keep SC with those.
>
> Old files in unit tests - Disclaimer I should have put: I forgot to update copyrights and documentation, so those will come later. Files in the autotests directory are slight modifications of existent files which basically hijack KAuth's backend loading making it possible to use a different testing backend. Will probably write a more detailed documentation on how autotesting is achieved in the future, not anything strictly urgent unless somebody plans to hack on the core testing suite soon (very unlikely).
>
> Will fix enums in my branch (I won't update the review since it's too much of a hassle) and will wait for further feedback from you about the other points before amending & merging.
>
> Kevin Ottens wrote:
> Regarding the static accessors I think you're right, they won't hurt. If that really bothers you though you could at least mark them deprecated, that'll motivate people to port away from them, and you'll be in a good position to remove them at the next great ABI breakage. :-)
Kevin: exactly what I intended to do :) Will do this in my branch then
- Dario
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104337/#review11596
-----------------------------------------------------------
On March 18, 2012, 10:25 p.m., Dario Freddi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104337/
> -----------------------------------------------------------
>
> (Updated March 18, 2012, 10:25 p.m.)
>
>
> Review request for kdelibs, Kevin Ottens, David Faure, and Alexander Neundorf.
>
>
> Description
> -------
>
> Preamble - sorry for having to name-call people but apparently we still don't have a frameworks way for reviewing code (which sucks). And sorry for the long summary, but it's worth reading. However.
>
> This huge patchsets brings KAuth in the marvelous world of Frameworks. If you dislike ReviewBoard's way of displaying diffs or simply want to see a commit list, please refer to the URL in "Branch".
>
> First of all, I pulled in a dependency on KJob after a chat with Kevin. This makes KAuth tier2, but shouldn't be a big issue.
>
> Then there's the hard part: source compatibility is reasonably broken here. The changes I had to do were mostly for the sake of revamping the internal workflow of the library. The main problem KAuth had was the fact it was completely synchronous, leading to a multitude of problems. After these changes it's fully asynchronous instead (reason for pulling in KJob), the API was simplified, and some unused features like multiple action execution have been removed.
>
> The main changes at a glance:
>
> * Some renaming to the enums
> * Moving Action & ActionReply to be implicitly shared
> * Removing ActionWatcher (now useless due to the new semantics of execute
> * Removing some useless APIs from Action, namely executeActions, execute(helper)
> * execute() now returns a KJob
> * helperID() -> helperId()
> * Static action replies are now static accessors returning a new instance. This was a complete mistake in the first place, but it's still there with a different semantic to ease porting. The main use case for changing this is a failure to handle implicitly shared classes in multithreaded environments with that approach.
>
> Of course, while it would be awesome to have all the code reviewed, I understand it's a very big change so I'd like at least some feedback on the following points:
>
> * General sanity of the new API
> * Consistency of the enums. StatusInvalid vs. ExecuteMode vs. AuthorizationDeniedError. While the semantic seems correct to me, I'd like to have some feedback on whether consistency is valuable in the ordering of <type><value> vs. <value><type> and which one should be preferred in case.
> * Whether to deprecate static accessors such as static const ActionReply SuccessReply(). I strongly favor this.
> * Whether the new dependency of kcoreaddons for the sake of using KJob is reasonable or I should go for a different alternative.
> * CMake sanity for the new dependency of kcoreaddons.
>
> The code is pretty much unit-tested and it should have a decent coverage, even if I had no way to check this. For unit tests, I had to create a fake authorization backend for testing purposes, whereas I managed to reuse the dbus backend for helper communication, so that I could even test that. For running the helper and the client in the same process, in the unit test I am resorting to making the dbus service of the helper live in a separate thread, to prevent asynchronous DBus calls from failing due to QDBus' local-loop optimization. The test is also run on the session bus.
>
>
> Diffs
> -----
>
> staging/kauth/CMakeLists.txt PRE-CREATION
> staging/kauth/autotests/BackendsManager.h PRE-CREATION
> staging/kauth/autotests/BackendsManager.cpp PRE-CREATION
> staging/kauth/autotests/CMakeLists.txt PRE-CREATION
> staging/kauth/autotests/HelperTest.cpp PRE-CREATION
> staging/kauth/autotests/SetupActionTest.cpp PRE-CREATION
> staging/kauth/autotests/TestBackend.h PRE-CREATION
> staging/kauth/autotests/TestBackend.cpp PRE-CREATION
> staging/kauth/autotests/TestHelper.h PRE-CREATION
> staging/kauth/autotests/TestHelper.cpp PRE-CREATION
> staging/kauth/src/AuthBackend.h PRE-CREATION
> staging/kauth/src/CMakeLists.txt PRE-CREATION
> staging/kauth/src/HelperProxy.h PRE-CREATION
> staging/kauth/src/backends/dbus/DBusHelperProxy.h PRE-CREATION
> staging/kauth/src/backends/dbus/DBusHelperProxy.cpp PRE-CREATION
> staging/kauth/src/backends/dbus/org.kde.auth.xml PRE-CREATION
> staging/kauth/src/backends/fake/FakeBackend.cpp PRE-CREATION
> staging/kauth/src/backends/fakehelper/FakeHelperProxy.h PRE-CREATION
> staging/kauth/src/backends/fakehelper/FakeHelperProxy.cpp PRE-CREATION
> staging/kauth/src/backends/mac/AuthServicesBackend.cpp PRE-CREATION
> staging/kauth/src/backends/policykit/PolicyKitBackend.cpp PRE-CREATION
> staging/kauth/src/backends/polkit-1/Polkit1Backend.cpp PRE-CREATION
> staging/kauth/src/kauthaction.h PRE-CREATION
> staging/kauth/src/kauthaction.cpp PRE-CREATION
> staging/kauth/src/kauthactionreply.h PRE-CREATION
> staging/kauth/src/kauthactionreply.cpp PRE-CREATION
> staging/kauth/src/kauthactionwatcher.h PRE-CREATION
> staging/kauth/src/kauthactionwatcher.cpp PRE-CREATION
> staging/kauth/src/kauthexecutejob.h PRE-CREATION
> staging/kauth/src/kauthexecutejob.cpp PRE-CREATION
>
> Diff: http://git.reviewboard.kde.org/r/104337/diff/
>
>
> Testing
> -------
>
> New unit tests pass 100%
>
>
> Thanks,
>
> Dario Freddi
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20120323/9cae5f43/attachment.htm>
More information about the kde-core-devel
mailing list