Review Request: Make KAuth ready for frameworks + API Changes
7 views
Skip to first unread message
Dario Freddi
Mar 18, 2012, 6:25:52 PM3/18/12
to Kevin Ottens, David Faure, Alexander Neundorf, kdelibs, Dario Freddi
| This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104337/ |
|
Review request for kdelibs, Kevin Ottens, David Faure, and Alexander Neundorf.
By Dario Freddi.
Description
Testing
Diffs
|
Stephen Kelly
Mar 18, 2012, 6:55:30 PM3/18/12
to Kevin Ottens, David Faure, Alexander Neundorf, kdelibs, Stephen Kelly, Dario Freddi
| This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104337/ |
Given that it does so many things, I wonder if it should be split into multiple patches. (I didn't fetch the branch - maybe you already have it in multiple patches) For examples, renaming the enums in one commit, making Action implicitly shared in another, ID ->id in another etc. That would make the 'meat' of your work reviewable. Did you investigate how much the removed API is used?
- Stephen
On March 18th, 2012, 10:25 p.m., Dario Freddi wrote:
|
Review request for kdelibs, Kevin Ottens, David Faure, and Alexander Neundorf.
By Dario Freddi.
|
Updated March 18, 2012, 10:25 p.m. |
Dario Freddi
Mar 18, 2012, 6:59:09 PM3/18/12
to Kevin Ottens, David Faure, Alexander Neundorf, kdelibs, Stephen Kelly, Dario Freddi
| This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104337/ |
On March 18th, 2012, 10:55 p.m., Stephen Kelly wrote:
Given that it does so many things, I wonder if it should be split into multiple patches. (I didn't fetch the branch - maybe you already have it in multiple patches) For examples, renaming the enums in one commit, making Action implicitly shared in another, ID ->id in another etc. That would make the 'meat' of your work reviewable. Did you investigate how much the removed API is used?
As I said in the description, the branch has indeed separate commits with more elaborate descriptions where needed and it's even split in a further patchset between api changes and unit tests. The removed APIs (multiple actions) is used nowhere, the semantics of execute() do not obviously apply as they changed anyway and can be replaced easily (no features are lost). The reason why I didn't split the review in RB in multiple patches is because it's already quite difficult to submit a patch since you have to give it the as a parent a full diff of frameworks vs. master, which takes a reasonable amount of time and makes it extremely hard to update the patch afterwards.
- Dario
Stephen Kelly
Mar 18, 2012, 7:04:41 PM3/18/12
to Kevin Ottens, David Faure, Alexander Neundorf, kdelibs, Stephen Kelly, Dario Freddi
| This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104337/ |
Nice, thanks and sorry for the noise, and thanks for making the branch.
- Stephen
On March 18th, 2012, 10:25 p.m., Dario Freddi wrote:
|
Review request for kdelibs, Kevin Ottens, David Faure, and Alexander Neundorf.
By Dario Freddi.
|
Updated March 18, 2012, 10:25 p.m. Description |
Dario Freddi
Mar 18, 2012, 7:06:28 PM3/18/12
to Kevin Ottens, David Faure, Alexander Neundorf, kdelibs, Stephen Kelly, Dario Freddi
| This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104337/ |
On March 18th, 2012, 11:04 p.m., Stephen Kelly wrote:
Nice, thanks and sorry for the noise, and thanks for making the branch.
Np, hope you'll be able to have a quick look at it as well, it would be great :)
- Dario
Dario Freddi
Mar 18, 2012, 8:30:44 PM3/18/12
to Kevin Ottens, David Faure, Alexander Neundorf, kdelibs, Dario Freddi
| This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104337/ |
On March 19th, 2012, 12:17 a.m., Henry Miller wrote:
In kauth/autotests/HelperTest.cpp The comment on line 57-68 should be reworded. In general when someone is told not to touch some lines they won't. You should be clear on why the code is that way. However saying "you don't want to touch this code" is a bad thing. It gives someone permission to not look close, even when in the future their change would break things. Yes what you are doing is subtle, but that is no excuse for someone to not understand it.
Well, the comment was indeed meant to be ironic and funny, and I can indeed rephrase it if it's not desirable. Though, there is all the needed insights into that trick, which is:
"Qt's local loop optimizations at the moment make it impossible to stream an async request to a process
living on the same thread. So that's what we do: we instantiate a separate helperProxy and move it to
a different thread - afterwards we can do everything as if we were in a separate process.
If you are wondering if this means we'll have two helper proxies, you are right my friend. But please
remember that helperProxy acts both as a client and as a server, so it makes total sense."
But I can understand if the remaining part does not look appropriate - it's just that I usually don't like reading boring comments ;)
Henry Miller
Mar 18, 2012, 8:17:14 PM3/18/12
to Kevin Ottens, David Faure, Alexander Neundorf, kdelibs, Dario Freddi
| This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104337/ |
In kauth/autotests/HelperTest.cpp The comment on line 57-68 should be reworded. In general when someone is told not to touch some lines they won't. You should be clear on why the code is that way. However saying "you don't want to touch this code" is a bad thing. It gives someone permission to not look close, even when in the future their change would break things. Yes what you are doing is subtle, but that is no excuse for someone to not understand it.
- Henry
On March 18th, 2012, 10:25 p.m., Dario Freddi wrote:
|
Review request for kdelibs, Kevin Ottens, David Faure, and Alexander Neundorf.
By Dario Freddi.
|
Updated March 18, 2012, 10:25 p.m. Description |
Stephen Kelly
Mar 19, 2012, 4:58:52 PM3/19/12
to Kevin Ottens, David Faure, Alexander Neundorf, kdelibs, Stephen Kelly, Dario Freddi
| This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104337/ |
On March 18th, 2012, 11:04 p.m., Stephen Kelly wrote:
Nice, thanks and sorry for the noise, and thanks for making the branch.
On March 18th, 2012, 11:06 p.m., Dario Freddi wrote:
Np, hope you'll be able to have a quick look at it as well, it would be great :)
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
Mar 19, 2012, 5:04:54 PM3/19/12
to Kevin Ottens, David Faure, Alexander Neundorf, kdelibs, Stephen Kelly, Dario Freddi
| This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104337/ |
On March 18th, 2012, 11:04 p.m., Stephen Kelly wrote:
Nice, thanks and sorry for the noise, and thanks for making the branch.
On March 18th, 2012, 11:06 p.m., Dario Freddi wrote:
Np, hope you'll be able to have a quick look at it as well, it would be great :)
On March 19th, 2012, 8:58 p.m., 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?
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
Mar 20, 2012, 1:04:10 PM3/20/12
to Kevin Ottens, David Faure, Alexander Neundorf, kdelibs, Stephen Kelly, Dario Freddi
| This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104337/ |
On March 18th, 2012, 11:04 p.m., Stephen Kelly wrote:
Nice, thanks and sorry for the noise, and thanks for making the branch.
On March 18th, 2012, 11:06 p.m., Dario Freddi wrote:
Np, hope you'll be able to have a quick look at it as well, it would be great :)On March 19th, 2012, 8:58 p.m., 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?
On March 19th, 2012, 9:04 p.m., 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.
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.
- Kevin
Dario Freddi
Mar 20, 2012, 8:20:08 PM3/20/12
to Kevin Ottens, David Faure, Alexander Neundorf, kdelibs, Stephen Kelly, Dario Freddi
| This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104337/ |
On March 18th, 2012, 11:04 p.m., Stephen Kelly wrote:
Nice, thanks and sorry for the noise, and thanks for making the branch.
On March 18th, 2012, 11:06 p.m., Dario Freddi wrote:
Np, hope you'll be able to have a quick look at it as well, it would be great :)On March 19th, 2012, 8:58 p.m., 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?On March 19th, 2012, 9:04 p.m., 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.
On March 20th, 2012, 5:04 p.m., 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.
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.
- Dario
Alexander Neundorf
Mar 21, 2012, 4:54:13 PM3/21/12
to Kevin Ottens, David Faure, Alexander Neundorf, kdelibs, Dario Freddi
| This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104337/ |
I did not yet have time to look through the git branch... So, kauth, we have a bunch of cmake macros related to this in kdelibs/cmake/KDE4Macros.cmake, right ? What about them ?
- Alexander
On March 18th, 2012, 10:25 p.m., Dario Freddi wrote:
|
Review request for kdelibs, Kevin Ottens, David Faure, and Alexander Neundorf.
By Dario Freddi.
|
Updated March 18, 2012, 10:25 p.m. Description |
Stephen Kelly
Mar 21, 2012, 6:52:52 PM3/21/12
to Kevin Ottens, David Faure, Alexander Neundorf, kdelibs, Stephen Kelly, Dario Freddi
| This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104337/ |
On March 21st, 2012, 8:54 p.m., Alexander Neundorf wrote:
I did not yet have time to look through the git branch... So, kauth, we have a bunch of cmake macros related to this in kdelibs/cmake/KDE4Macros.cmake, right ? What about them ?
They are already in ${CMAKE_SOURCE_DIR}/staging/kauth/cmake/KAuthMacros.cmake
- Stephen
Dario Freddi
Mar 21, 2012, 11:35:37 PM3/21/12
to Kevin Ottens, David Faure, Alexander Neundorf, kdelibs, Stephen Kelly, Dario Freddi
| This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104337/ |
On March 21st, 2012, 8:54 p.m., Alexander Neundorf wrote:
I did not yet have time to look through the git branch... So, kauth, we have a bunch of cmake macros related to this in kdelibs/cmake/KDE4Macros.cmake, right ? What about them ?
On March 21st, 2012, 10:52 p.m., Stephen Kelly wrote:
They are already in ${CMAKE_SOURCE_DIR}/staging/kauth/cmake/KAuthMacros.cmake
This change didn't affect them at all, since their sole purpose is handling helper creation and backend selection - this didn't change. The cmake check was mostly to verify I did the correct bits for KCoreAddons (given I simply mentioned the binary name when linking), but Stephen already confirmed it's ok. Of course, having you double-checking the macros just in case would be awesome if you have a couple spare minutes :)
- Dario
Kevin Ottens
Mar 22, 2012, 12:04:34 PM3/22/12
to Kevin Ottens, David Faure, Alexander Neundorf, kdelibs, Stephen Kelly, Dario Freddi
| This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104337/ |
On March 18th, 2012, 11:04 p.m., Stephen Kelly wrote:
Nice, thanks and sorry for the noise, and thanks for making the branch.
On March 18th, 2012, 11:06 p.m., Dario Freddi wrote:
Np, hope you'll be able to have a quick look at it as well, it would be great :)On March 19th, 2012, 8:58 p.m., 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?On March 19th, 2012, 9:04 p.m., 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.
On March 20th, 2012, 5:04 p.m., 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.
On March 21st, 2012, 12:20 a.m., 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.
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
Reply all
Reply to author
Forward
0 new messages