Review Request: Diagnostics Dialog for Amarok.

[Bart Cerneels]

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

Ship it!


Looks quite perfect style wise (ok, I found one thing to nitpick on ;). Code is clean and functional. 

There is one thing I noticed while testing: all are plugins are version 1.0 (X-KDE-PluginInfo-Version=1.0)
We bump X-KDE-Amarok-framework-version an each release and use that to check if a plugin can be used or not. It's currently not relevant to include this anyway since we have never committed to ABI stability of our APIs and hence always bump the framework version.

If you already have a developer account (identity.kde.org and request write access to the git repos via sysadmin request on bugs.kde.org) you can push it with the changes I mentioned. If not I can push it for you but urge you to get that account for future contributions.


src/dialogs/DiagnosticDialog.cpp
<http://git.reviewboard.kde.org/r/104449/#comment9531>

    I would put the body on a newline. Just a bit more readable within the rest of the amarok codebase.



src/dialogs/DiagnosticDialog.cpp
<http://git.reviewboard.kde.org/r/104449/#comment9535>

    Either separate them into running/stopped lists or append "(stopped)". The rational is that if that line is partially copy/pasted on IRC, we'll know that it's incomplete.



src/dialogs/DiagnosticDialog.cpp
<http://git.reviewboard.kde.org/r/104449/#comment9534>

    Same as for plugins but with enabled/disabled.



src/dialogs/DiagnosticDialog.cpp
<http://git.reviewboard.kde.org/r/104449/#comment9532>

    We spell Qt with lower case t and make apple QuickTime jokes about those who don't ;)



src/dialogs/DiagnosticDialog.cpp
<http://git.reviewboard.kde.org/r/104449/#comment9533>

    The phonon backend also needs a version. Might help track down bugs to specific versions or builds. Hope you can get it easily.


- Bart Cerneels


On March 31, 2012, 6:15 p.m., Andrzej Hunt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104449/
> -----------------------------------------------------------
> 
> (Updated March 31, 2012, 6:15 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> Adds a diagnostics dialog to Amarok. This shows versions for KDE, QT, Phonon, the Phonon backend, and all scripts and plugins.
> 
> As described in https://bugs.kde.org/show_bug.cgi?id=296415.
> 
> This patch also changes/corrects PluginManager::plugins() to be const.
> 
> 
> This addresses bug 296415.
>     https://bugs.kde.org/show_bug.cgi?id=296415
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt 6e590e8 
>   src/MainWindow.h b149cb9 
>   src/MainWindow.cpp 98b1c77 
>   src/PluginManager.h 6b9f3ca 
>   src/PluginManager.cpp c46b12f 
>   src/dialogs/DiagnosticDialog.h PRE-CREATION 
>   src/dialogs/DiagnosticDialog.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/104449/diff/
> 
> 
> Testing
> -------
> 
> 
> Screenshots
> -----------
> 
> Screenshot of Dialog
>   http://git.reviewboard.kde.org/r/104449/s/501/
> 
> 
> Thanks,
> 
> Andrzej Hunt
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20120402/5c2eb38a/attachment.html>