Review Request: KWallet::Wallet::openWallet or will it?

[David Edmundson]

> On July 14, 2012, 9:19 a.m., George Kiagiadakis wrote:
> > KTp/pending-wallet-operation.cpp, line 11
> > <http://git.reviewboard.kde.org/r/105566/diff/1/?file=72583#file72583line11>
> >
> >     What if it fails...?

If it fails, the developer should continue using the WalletInterface as though everything is fine. The attempt to open it succeeded. Code in all the internal methods here will check if it can load/save passwords and act appropriately.

If it fails it potentially means the user cancelled entering their wallet password and we should /not/ try again.


> On July 14, 2012, 9:19 a.m., George Kiagiadakis wrote:
> > KTp/wallet-interface.cpp, lines 52-53
> > <http://git.reviewboard.kde.org/r/105566/diff/1/?file=72585#file72585line52>
> >
> >     Good thing that you thought about threads here, but the rest of the API is still not thread-safe.
> >     
> >     Solution #1 would be to have one global mutex and lock it in every method.
> >     
> >     Solution #2 would be to make the class non-singleton so that you can claim the api is re-entrant and be done with it.
> >     
> >     I would personally prefer the second solution as the first solution would only make things slower and still unsafe. By unsafe I mean that if there are two different parts of an application trying to access the wallet and one part assumes that the other part has opened the wallet, but then something happens and this assertion is no longer true, then you have a bug that is quite difficult to trace.
> >     
> >     The solution would be to have a non-singleton WalletInterface with a private constructor and the openWallet() method returning a KTp::PendingWallet, which has a wallet() method returning the pointer to the interface (just like other PendingOperations in tp-qt). This way, to get access to the wallet you must go through the opening process, which ensures you are not going to do any mistake.
> >

We need the call to KWallet::Wallet::openWallet to be in a singleton somewhere, otherwise we go back to a bug we had in 0.3 days where the KWallet requests to be opened lots of times and that causes a mess on setups where people have the wallet to "prompt every time" or "close when app stops using it". I need to always use the same instance of the KWallet::Wallet.

I did try (the alternate review) a version something similar to what you said...but given the wallet usages throughout the auth handler and accounts KCM it became a much more annoying change to implement. I have to put in pendingOperations to open the wallet all over the place, even though it's probably already tried being opened.


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105566/#review15836
-----------------------------------------------------------


On July 14, 2012, 12:50 a.m., David Edmundson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105566/
> -----------------------------------------------------------
> 
> (Updated July 14, 2012, 12:50 a.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Description
> -------
> 
> Adds a method to asynchronously open the wallet. This must be called (and completed) by the application if they want to use any other methods.
> API is a bit horrendous, but that's mostly because of KWallet.
> 
> Wallet opening appears as a Tp::PendingOperation so that it can be used in a composite job.
> 
> 
> Diffs
> -----
> 
>   KTp/CMakeLists.txt 61da66d 
>   KTp/pending-wallet-operation.h PRE-CREATION 
>   KTp/pending-wallet-operation.cpp PRE-CREATION 
>   KTp/wallet-interface.h c43fb38 
>   KTp/wallet-interface.cpp d2dc2cb 
> 
> Diff: http://git.reviewboard.kde.org/r/105566/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20120714/31b9f964/attachment-0001.html>