> On March 30, 2012, 1:14 p.m., Dario Freddi wrote: > > April is upon us. If no objections arise in the time being, these changes > > will be merged on Sunday, after which I'll ask Kevin to move KAuth back to > > tier2/ for prime time. > > Stephen Kelly wrote: > Thanks for making this happen! :) > > Instead of merging, please rebase your work like this: > > git fetch origin > git checkout kauth-unit-tests > git rebase origin/frameworks > git checkout frameworks > git rebase kauth-unit-tests > gitk # Make sure it still looks ok. > git push > > > This will instead replay your changes on top of the tip of the frameworks > branch instead of creating a merge, which is not needed. > > Dario Freddi wrote: > I always do this - nice to know that others love rebased branches :D
Great. Good to know. You did use the word 'merge' though, which triggered my alarm. :) Thanks, - Stephen ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104337/#review12013 ----------------------------------------------------------- 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 > >