Re: Review Request: Diagnostics Dialog for Amarok.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104449/#review12140 --- This review has been submitted with commit ce15c154eb7f641c3ce6a273660edb11a2bdcab1 by Andrzej J. R. Hunt to branch master. - Commit Hook On April 2, 2012, 1:37 p.m., Andrzej Hunt wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104449/ --- (Updated April 2, 2012, 1:37 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/ Updated Screenshot (Version 2) http://git.reviewboard.kde.org/r/104449/s/502/ Thanks, Andrzej Hunt ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Diagnostics Dialog for Amarok.
--- 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 ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Diagnostics Dialog for Amarok.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104449/#review12109 --- Ship it! As Bart said, very good patch, thanks for it! Besides to what Bart already pointed out, I've found 2 minor things. src/MainWindow.h http://git.reviewboard.kde.org/r/104449/#comment9536 Nitpicking: perhaps unnecessary newline src/dialogs/DiagnosticDialog.h http://git.reviewboard.kde.org/r/104449/#comment9537 Any special reason why the QWeakPointer is used? If there is none, plain pointer should be used instead. - Matěj Laitl 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 ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Diagnostics Dialog for Amarok.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104449/ --- (Updated April 2, 2012, 1:37 p.m.) Review request for Amarok. Changes --- Thanks for the comments, here's an updated patch (and screenshot). I've just applied for git access, so I'll commit once/if it's enabled. (The reason I used a QWeakPointer was because I had seen the same being done in the ExtendedAboutDialog (I now see why that is done there) -- I've changed it to a normal pointer.) (I'll do the changes to ScriptManager/PluginManager as described in https://bugs.kde.org/show_bug.cgi?id=296415#c4 as a separate patch when I next have time.) 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 (updated) - 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 (updated) --- Screenshot of Dialog http://git.reviewboard.kde.org/r/104449/s/501/ Updated Screenshot (Version 2) http://git.reviewboard.kde.org/r/104449/s/502/ Thanks, Andrzej Hunt ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
