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

[George Kiagiadakis]

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



KTp/CMakeLists.txt
<http://git.reviewboard.kde.org/r/105566/#comment12514>

    You don't really want to install this header



KTp/pending-wallet-operation.h
<http://git.reviewboard.kde.org/r/105566/#comment12511>

    missing license



KTp/pending-wallet-operation.cpp
<http://git.reviewboard.kde.org/r/105566/#comment12512>

    missing license



KTp/pending-wallet-operation.cpp
<http://git.reviewboard.kde.org/r/105566/#comment12513>

    What if it fails...?



KTp/wallet-interface.cpp
<http://git.reviewboard.kde.org/r/105566/#comment12515>

    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.
    


- George Kiagiadakis


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/9e751b45/attachment.html>