Re: Re: Review request for KSecretsService components
A Dimarts, 8 de novembre de 2011, Valentin Rusu vàreu escriure: Hello Again, The freeze will come in less thant two days now and I'd like to know if anyone reviewed these components. Thanks, On 10/31/2011 11:48 PM, Valentin Rusu wrote: Hello, Please be advised three repostories need review before integration into the next release: 1. /kdereview/ksecretservice 2. kdelibs branch ksecretsservice 3. /kdereview/ksecrets *1. /kdereview/ksecretservice* The first one has several subdirectories. The only relevant one is the daemon subdirectory. Other directories contents was already moved to other repositories (see below). This daemon directory contains the sources of the future kde-runtime/ksecretsserviced It needs testing but it's quite usable. *2. kdelibs branch ksecretsservice* kdelibs/kdeui/ksecretsservice This is the API KDE applications will be supposed to use instead of KWallet class. Tools listed below already use this API. This is scary, last time i used kwallet, i had to add a single line, and now there are like a billion of classes? Or is this API for more complex apps like kwalletmanager? My comments on the kdelibs API: - ksecretsservice/ksecretsservicecodec.h * Seems to be installed but has no documentation at all * Uses references for output parameters when KDE/Qt usually uses pointers * Does not use d-pointers thus maintaining ABI is going to be difficult - ksecretsservice/ksecretsservicecollection.h * I miss a note saying who belongs the ownsership of all those job that come as result of the getters. Do I have to delete them? * createItem falls in the let's use a bool instead of an enum because it just have two values trap for the replace parameter * There are a few of spacing glitches, e.g. const QVariantMap collectionProperties = QVariantMap(), const WId promptParentWindowId =0 ); * There are a few not implemented signals - ksecretsservice/ksecretsservicedbustypes.h Contains a plain struct and some inline functions, what if those have to change? - ksecretsservice/ksecretsserviceitem.h * Same question about ownership of jobs - ksecretsservice/ksecretsserviceitemjobs.h * Without having at all a clue on what it is used for, i miss a note saying to who belongs the ownership of the SecretItem passed to the constructor and returned in the secretItem() function, i.e. do i have to delete it myself? * SecretItemJob does not have a d-pointer * void finished( ItemJobError, const QString msg =); should probably be void finished( ItemJobError, const QString msg = QString()); - ksecretsservice/ksecretsservicesecret.h Is installed and has no documentation Albert kdelibs/kdeui/util/kwallet.cpp Contains code depending on a configuration flag that directs calls to ksecretsserviced instead of kwalletd, via the new API. *3. /kdereview/ksecrets* Contains several tools in a less or more mature state: a. kwl2kss - tool to import kwallet files to ksecretsservice, b. ksecrets - tool to list the contents of a ksecretsservice collection (e.g. wallet), c. kio - KIO slave in a just started state, intended to show collections in konqueror or dolphin, d. secretsync - this was the tool I initially wanted to do for KWallet, but drought me into ksecretsservice :-) It's half way implemented. The mandatory components for next release would be 1, 2, 3 (a, b), the others may wait, but releasing them may cause no harm if communication is done right (I'll take care of that). Thanks for your comments (any comments), -- Valentin Rusu (IRC valir, KDE vrusu) KSecretsService (former KSecretService, KWallet replacement)
Re: Review request for KSecretsService components
Hello Albert, Thanks for the thourough review. On 11/09/2011 03:26 PM, Albert Astals Cid wrote: A Dimarts, 8 de novembre de 2011, Valentin Rusu vàreu escriure: Hello Again, The freeze will come in less thant two days now and I'd like to know if anyone reviewed these components. Thanks, On 10/31/2011 11:48 PM, Valentin Rusu wrote: Hello, Please be advised three repostories need review before integration into the next release: 1. /kdereview/ksecretservice 2. kdelibs branch ksecretsservice 3. /kdereview/ksecrets *1. /kdereview/ksecretservice* The first one has several subdirectories. The only relevant one is the daemon subdirectory. Other directories contents was already moved to other repositories (see below). This daemon directory contains the sources of the future kde-runtime/ksecretsserviced It needs testing but it's quite usable. *2. kdelibs branch ksecretsservice* kdelibs/kdeui/ksecretsservice This is the API KDE applications will be supposed to use instead of KWallet class. Tools listed below already use this API. This is scary, last time i used kwallet, i had to add a single line, and now there are like a billion of classes? Welcome to the asynchronous jobs world. I had the same opinion at start. And, to be honest, that's why it takes so long to implement, as I'm not working on it only during my spare time. Or is this API for more complex apps like kwalletmanager? Well, the secrets service infrastructure, specified with GNOME's Stef Walter is intended to do more things thant KWallet was. For instance, the KWallet API won't let me do the synchronization tool I wanted without significant improvement. My comments on the kdelibs API: - ksecretsservice/ksecretsservicecodec.h * Seems to be installed but has no documentation at all It's indeed installed but it's an @internal class - I added the appropriate documentation. * Uses references for output parameters when KDE/Qt usually uses pointers * Does not use d-pointers thus maintaining ABI is going to be difficult Once again, it's an internal class, no breakage risk here as I'll maintain the API and the daemon together. - ksecretsservice/ksecretsservicecollection.h * I miss a note saying who belongs the ownsership of all those job that come as result of the getters. Do I have to delete them? Oh, I thought that'll not be needed. Usually jobs autodelete themselves. Note added. * createItem falls in the let's use a bool instead of an enum because it just have two values trap for the replace parameter Well, I don't agree. When presenting a new item to the collection, you should specify to replace existing item or not. Yes/No is boolean for me. * There are a few of spacing glitches, e.g. const QVariantMap collectionProperties = QVariantMap(), const WIdpromptParentWindowId =0 ); Added a space before the 0. * There are a few not implemented signals Yes, but they'll not be done before tommorrow, unfortunately. - ksecretsservice/ksecretsservicedbustypes.h Contains a plain struct and some inline functions, what if those have to change? Marked as @internal - ksecretsservice/ksecretsserviceitem.h * Same question about ownership of jobs Same note added. - ksecretsservice/ksecretsserviceitemjobs.h * Without having at all a clue on what it is used for, i miss a note saying to who belongs the ownership of the SecretItem passed to the constructor and returned in the secretItem() function, i.e. do i have to delete it myself? This class was marked as @internal and corresponding comment was added. * SecretItemJob does not have a d-pointer Oops! Added. * void finished( ItemJobError, const QString msg =); should probably be void finished( ItemJobError, const QString msg = QString()); Right! - ksecretsservice/ksecretsservicesecret.h Is installed and has no documentation Documentation added. Albert kdelibs/kdeui/util/kwallet.cpp Contains code depending on a configuration flag that directs calls to ksecretsserviced instead of kwalletd, via the new API. *3. /kdereview/ksecrets* Contains several tools in a less or more mature state: a. kwl2kss - tool to import kwallet files to ksecretsservice, b. ksecrets - tool to list the contents of a ksecretsservice collection (e.g. wallet), c. kio - KIO slave in a just started state, intended to show collections in konqueror or dolphin, d. secretsync - this was the tool I initially wanted to do for KWallet, but drought me into ksecretsservice :-) It's half way implemented. The mandatory components for next release would be 1, 2, 3 (a, b), the others may wait, but releasing them may cause no harm if communication is done right (I'll take care of that). Thanks for your comments (any comments), -- Valentin Rusu (IRC valir, KDE vrusu) KSecretsService (former KSecretService, KWallet replacement) -- Valentin Rusu (IRC valir, KDE vrusu) KSecretsService (former KSecretService, KWallet replacement)
Re: Re: Review request for KSecretsService components
A Dijous, 10 de novembre de 2011, Valentin Rusu vàreu escriure: Hello Albert, Thanks for the thourough review. On 11/09/2011 03:26 PM, Albert Astals Cid wrote: This is scary, last time i used kwallet, i had to add a single line, and now there are like a billion of classes? Welcome to the asynchronous jobs world. I had the same opinion at start. And, to be honest, that's why it takes so long to implement, as I'm not working on it only during my spare time. Are you saying you don't have anything as simple as this in the new API? QString walletName = KWallet::Wallet::NetworkWallet(); KWallet::Wallet *wallet = KWallet::Wallet::openWallet(walletName, parentwid); if ( !wallet-hasFolder( KPdf ) ) wallet-createFolder( KPdf ); wallet-setFolder( KPdf ); wallet-readPassword( walletKey, retrievedPass ) * createItem falls in the let's use a bool instead of an enum because it just have two values trap for the replace parameter Well, I don't agree. When presenting a new item to the collection, you should specify to replace existing item or not. Yes/No is boolean for me. That is the problem, that a boolean is Yes/No, and then you end up with a call that says foo-createItem(label, attributes, mySecret, false); And it seems you do not want to create the item :D while foo-createItem(label, attributes, mySecret, DoNotReplace); is much more obvious. More on my side at http://wiki.qt-project.org/API_Design_Principles#The_Boolean_Parameter_Trap and http://ariya.ofilabs.com/2011/08/hall-of-api-shame-boolean-trap.html Of course that is a minor thing and won't complain too much if you decide to disagree with me ;-) Albert
Re: Review request for KSecretsService components
On 11/10/2011 12:48 AM, Albert Astals Cid wrote: A Dijous, 10 de novembre de 2011, Valentin Rusu vàreu escriure: Hello Albert, Thanks for the thourough review. On 11/09/2011 03:26 PM, Albert Astals Cid wrote: This is scary, last time i used kwallet, i had to add a single line, and now there are like a billion of classes? Welcome to the asynchronous jobs world. I had the same opinion at start. And, to be honest, that's why it takes so long to implement, as I'm not working on it only during my spare time. Are you saying you don't have anything as simple as this in the new API? QString walletName = KWallet::Wallet::NetworkWallet(); KWallet::Wallet *wallet = KWallet::Wallet::openWallet(walletName, parentwid); if ( !wallet-hasFolder( KPdf ) ) wallet-createFolder( KPdf ); wallet-setFolder( KPdf ); wallet-readPassword( walletKey, retrievedPass ) Yes, your code isn't asynchrounous ! (boo) Well, I don't fully agree with this point of view, but in Randa they managed to convince me about the benefits. Take a look inside kwallet.cpp class to see how this new API should be used. Another example is the ksecrets API inside kdereview/ksecrets repository. * createItem falls in the let's use a bool instead of an enum because it just have two values trap for the replace parameter Well, I don't agree. When presenting a new item to the collection, you should specify to replace existing item or not. Yes/No is boolean for me. That is the problem, that a boolean is Yes/No, and then you end up with a call that says foo-createItem(label, attributes, mySecret, false); And it seems you do not want to create the item :D while foo-createItem(label, attributes, mySecret, DoNotReplace); is much more obvious. More on my side at http://wiki.qt-project.org/API_Design_Principles#The_Boolean_Parameter_Trap and http://ariya.ofilabs.com/2011/08/hall-of-api-shame-boolean-trap.html Ok, I see. I'll change that. Of course that is a minor thing and won't complain too much if you decide to disagree with me ;-) Albert -- Valentin Rusu (IRC valir, KDE vrusu) KSecretsService (former KSecretService, KWallet replacement)
Re: Review request for KSecretsService components
On 11/10/2011 01:00 AM, Valentin Rusu wrote: On 11/10/2011 12:48 AM, Albert Astals Cid wrote: * createItem falls in the let's use a bool instead of an enum because it just have two values trap for the replace parameter Well, I don't agree. When presenting a new item to the collection, you should specify to replace existing item or not. Yes/No is boolean for me. That is the problem, that a boolean is Yes/No, and then you end up with a call that says foo-createItem(label, attributes, mySecret, false); And it seems you do not want to create the item :D while foo-createItem(label, attributes, mySecret, DoNotReplace); is much more obvious. More on my side at http://wiki.qt-project.org/API_Design_Principles#The_Boolean_Parameter_Trap and http://ariya.ofilabs.com/2011/08/hall-of-api-shame-boolean-trap.html Ok, I see. I'll change that. Well, In fact I changed my mind, as the enum must be a member of Collection class, but required to be used in the create job and nested enums cannot be forward declared. So that'll require to move the enum outside of the Collection class, leading to even uglier code. So I'll stay with the boolean, wich also corresponds to the freedesktop.org dbus interfaces spec, btw. -- Valentin Rusu (IRC valir, KDE vrusu) KSecretsService (former KSecretService, KWallet replacement)
Re: Re: Review request for KSecretsService components
A Dijous, 10 de novembre de 2011, Valentin Rusu vàreu escriure: On 11/10/2011 12:48 AM, Albert Astals Cid wrote: A Dijous, 10 de novembre de 2011, Valentin Rusu vàreu escriure: Hello Albert, Thanks for the thourough review. On 11/09/2011 03:26 PM, Albert Astals Cid wrote: This is scary, last time i used kwallet, i had to add a single line, and now there are like a billion of classes? Welcome to the asynchronous jobs world. I had the same opinion at start. And, to be honest, that's why it takes so long to implement, as I'm not working on it only during my spare time. Are you saying you don't have anything as simple as this in the new API? QString walletName = KWallet::Wallet::NetworkWallet(); KWallet::Wallet *wallet = KWallet::Wallet::openWallet(walletName, parentwid); if ( !wallet-hasFolder( KPdf ) ) wallet-createFolder( KPdf ); wallet-setFolder( KPdf ); wallet-readPassword( walletKey, retrievedPass ) Yes, your code isn't asynchrounous ! (boo) Yeah well, kparts gives you virtual bool openUrl( const KUrl url ); With that signature you are forced to either use synchronous code or create a nested event loop, pick your poison. Well, I don't fully agree with this point of view, but in Randa they managed to convince me about the benefits. Take a look inside kwallet.cpp class to see how this new API should be used. Another example is the ksecrets API inside kdereview/ksecrets repository. This answer makes me really scared in that it is not going to be 5 lines :-/ Ok, now i've looked at the API and I am not sure i like it, particulary nonQtlike seems the way of searching things by passing a map with a key/value structure of what you want to find. I can see that it is very flexible, but string based api end up with people writing key instead of Key and they lose days debugging why it fails. Also i'm a bit confused about the docu of SearchCollectionItemsJob * searchItems( const StringStringMap attributes ); it says * KSecretsService uses overall the following standard attributes: * Label : item's or collection's label so i understand that Label is the only key of the map that you can use, and then the example says * StrinStringMap attrs; * attrs[Key] = ; and uses Key ? Does this mean Key is a valid attribute too and should be documented? Or should it say Label? Or i totally misunderstood the documentation? Also note it says StrinStringMap, missing g. Albert
Re: Re: Review request for KSecretsService components
A Dijous, 10 de novembre de 2011, Valentin Rusu vàreu escriure: On 11/10/2011 01:00 AM, Valentin Rusu wrote: On 11/10/2011 12:48 AM, Albert Astals Cid wrote: * createItem falls in the let's use a bool instead of an enum because it just have two values trap for the replace parameter Well, I don't agree. When presenting a new item to the collection, you should specify to replace existing item or not. Yes/No is boolean for me. That is the problem, that a boolean is Yes/No, and then you end up with a call that says foo-createItem(label, attributes, mySecret, false); And it seems you do not want to create the item :D while foo-createItem(label, attributes, mySecret, DoNotReplace); is much more obvious. More on my side at http://wiki.qt-project.org/API_Design_Principles#The_Boolean_Parameter _Trap and http://ariya.ofilabs.com/2011/08/hall-of-api-shame-boolean-trap.html Ok, I see. I'll change that. Well, In fact I changed my mind, as the enum must be a member of Collection class, but required to be used in the create job and nested enums cannot be forward declared. So that'll require to move the enum outside of the Collection class, leading to even uglier code. There is a solution for this, the CreateCollectionItemJob constructor should accept a CreateCollectionItemJobPrivate instead of all the params (since i understand it does not make sense for me creating one directly and I will only get one thought Collection::createItem (reason to make it private too?)). This way you do not need a replace member in the constructor since Collection passes the CreateCollectionItemJobPrivate directly. Probably the same argument for making the constructor private for all the other jobs applies too. Albert So I'll stay with the boolean, wich also corresponds to the freedesktop.org dbus interfaces spec, btw. -- Valentin Rusu (IRC valir, KDE vrusu) KSecretsService (former KSecretService, KWallet replacement)
Re: Review request for KSecretsService components
Hello Again, The freeze will come in less thant two days now and I'd like to know if anyone reviewed these components. Thanks, On 10/31/2011 11:48 PM, Valentin Rusu wrote: Hello, Please be advised three repostories need review before integration into the next release: 1. /kdereview/ksecretservice 2. kdelibs branch ksecretsservice 3. /kdereview/ksecrets *1. /kdereview/ksecretservice* The first one has several subdirectories. The only relevant one is the daemon subdirectory. Other directories contents was already moved to other repositories (see below). This daemon directory contains the sources of the future kde-runtime/ksecretsserviced It needs testing but it's quite usable. *2. kdelibs branch ksecretsservice* kdelibs/kdeui/ksecretsservice This is the API KDE applications will be supposed to use instead of KWallet class. Tools listed below already use this API. kdelibs/kdeui/util/kwallet.cpp Contains code depending on a configuration flag that directs calls to ksecretsserviced instead of kwalletd, via the new API. *3. /kdereview/ksecrets* Contains several tools in a less or more mature state: a. kwl2kss - tool to import kwallet files to ksecretsservice, b. ksecrets - tool to list the contents of a ksecretsservice collection (e.g. wallet), c. kio - KIO slave in a just started state, intended to show collections in konqueror or dolphin, d. secretsync - this was the tool I initially wanted to do for KWallet, but drought me into ksecretsservice :-) It's half way implemented. The mandatory components for next release would be 1, 2, 3 (a, b), the others may wait, but releasing them may cause no harm if communication is done right (I'll take care of that). Thanks for your comments (any comments), -- Valentin Rusu (IRC valir, KDE vrusu) KSecretsService (former KSecretService, KWallet replacement)
