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