Review Request: Make the 'Now listening..' presence string configurable

[Daniele Elmo Domenichelli]

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


Very nice, thanks!
It would be almost a "ship it", but since you added drag and drop, now you get comments also on that part :)


config/nowplaying-widgets.h
<http://git.reviewboard.kde.org/r/104314/#comment9411>

    Please, try to keep the names of the classes in a file the more similar possible to the name of the file itself, as it makes a lot easier to find where an class is.
    If you want to make the class "generic" rename the files .cpp and .h otherwise rename the classes to NowPlayingXxxx



config/nowplaying-widgets.h
<http://git.reviewboard.kde.org/r/104314/#comment9415>

    By looking at the video, the QListWidget doesn't look very nice, there is a large blank space on the right and before you click on it I could not say that it is draggable. Perhaps adding some icons (like amarok does), and resizing it so that it takes exactly the space that is needed, could make it look better.
    



config/telepathy-kded-config.cpp
<http://git.reviewboard.kde.org/r/104314/#comment9419>

    The localized tag names should have a comment to explain the "%" and what does "title" represent (i18nc instead of i18n).
    (Perhaps should it also mention that it should be a single word?)
    



config/telepathy-kded-config.cpp
<http://git.reviewboard.kde.org/r/104314/#comment9420>

    You set the localized tag names here, but they look ugly, is it possible to remove the "%" and make the first letter uppercase (and obviously the opposite in the drop event)
    
    Or perhaps it is a better idea to let the translators translate "Title", "Author", etc. and you replace them?



config/telepathy-kded-config.cpp
<http://git.reviewboard.kde.org/r/104314/#comment9414>

    I suggest not to use "please" but to say "leave them untranslated because ..."
    
    Replace "The text" with "The default text"
    
    I'm not sure what is this "xgettext: no-c-format" here, but if it is required here (perhaps for the "%"s) you probably need it also in the other string.
    
    Also I'm not sure if ir is a good thing to duplicate this string, in 2 different files, is there a way to have it only once?
    
    



telepathy-mpris.cpp
<http://git.reviewboard.kde.org/r/104314/#comment9412>

    Just add track number to make me happy :D



telepathy-mpris.cpp
<http://git.reviewboard.kde.org/r/104314/#comment9413>

    See comment for the other string


- Daniele Elmo Domenichelli


On March 23, 2012, 8:49 p.m., Othmane Moustaouda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104314/
> -----------------------------------------------------------
> 
> (Updated March 23, 2012, 8:49 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Description
> -------
> 
> Enabled the possibility for the user to set his custom 'Now listening' presence string
> 
> 
> This addresses bug 282944.
>     http://bugs.kde.org/show_bug.cgi?id=282944
> 
> 
> Diffs
> -----
> 
>   config/CMakeLists.txt bd0b3f8dd89352d3b1f5e438f56123c508574ab4 
>   config/nowplaying-widgets.h PRE-CREATION 
>   config/nowplaying-widgets.cpp PRE-CREATION 
>   config/telepathy-kded-config.h 670c513df6b0cf7e69529f980a7f809b3f3d45a0 
>   config/telepathy-kded-config.cpp 1fb515f56500407b7b221d1d5310237f8d94ca7f 
>   config/telepathy-kded-config.ui 0d616c5503b22f37c0ae4d5a94411d0f97b25d38 
>   telepathy-mpris.h d909863e68d0fdc7a6eb4ebb972d975ba224b6ef 
>   telepathy-mpris.cpp 8dae701a203f6c5ba43a622ce7125615b4392b63 
> 
> Diff: http://git.reviewboard.kde.org/r/104314/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Othmane Moustaouda
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20120326/1a33ff9e/attachment-0001.html>