Review Request: add autocomplete for skype account names

[David Edmundson]

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


I like what it's doing. It's a very clever idea in terms of usability.
I have a few comments with regards to one line of code.


haze/skype-main-options-widget.cpp
<http://git.reviewboard.kde.org/r/100775/#comment1443>

    If you ever write "new" you should check it is deleted or has a parent. In this case it gets leaked (bad!).
    
    You may as well keep the QDir on the stack. Putting things on the stack is faster, and they get automatically 'deleted' when the function finishes.
    
    i.e
    QDir home(QString(%1/.Skype).arg(QDir::homePath())));
    
    You only need to write "new FooBar" if you need the lifespan of the object to last longer than the method.
    
    ----
    
    "home" isn't the best name for this variable, you're not in the home directory. You're in a Skype config folder. 
    
    ----
    
    Also is this clearer to read, rather than using string manipulation?:
    QDir skypeConfigDir(QDir::home().filePath(".Skype");
    
    


- David


On March 1, 2011, 7:18 p.m., Florian Reinhard wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100775/
> -----------------------------------------------------------
> 
> (Updated March 1, 2011, 7:18 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Summary
> -------
> 
> * add autocomplete for skype account names based on foldernames in $HOME/.Skype/
> 
> 
> Diffs
> -----
> 
>   haze/skype-main-options-widget.cpp 572804f 
> 
> Diff: http://git.reviewboard.kde.org/r/100775/diff
> 
> 
> Testing
> -------
> 
> * works with two accounts in ~/.Skype/
> * does nothing if there is no ~/.Skype/ 
> 
> 
> Thanks,
> 
> Florian
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-telepathy/attachments/20110303/35b4cb5f/attachment.htm