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
Review Request: Bug 292081 - JJ: Label about info at opendesktop.org is truncated
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104464/ --- Review request for Amarok. Description --- Fix for bug #292081 AnimatedBarWidget is modified to wrap text when widget is too narrow. This addresses bug 292081. https://bugs.kde.org/show_bug.cgi?id=292081 Diffs - src/aboutdialog/AnimatedBarWidget.h 8ec32ad src/aboutdialog/AnimatedBarWidget.cpp f40dd6d Diff: http://git.reviewboard.kde.org/r/104464/diff/ Testing --- Tested changed widget correctly wraps text when About dialog is resized. Thanks, Lachlan Dufton ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request: Bug 292081 - JJ: Label about info at opendesktop.org is truncated
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104464/#review12113 --- A couple of minor things - first, in the heightForWidth code, it's not at all clear what's going on with the padding (you use padding * 4 at first, and later just use 8). Clean that up and comment it. Second, the else keyword is superfluous. Otherwise, looks good. I haven't tested myself (I'm away and don't have Amarok built), but the code's plausible and you've tested. - Sam Lade On April 1, 2012, 11:10 p.m., Lachlan Dufton wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104464/ --- (Updated April 1, 2012, 11:10 p.m.) Review request for Amarok. Description --- Fix for bug #292081 AnimatedBarWidget is modified to wrap text when widget is too narrow. This addresses bug 292081. https://bugs.kde.org/show_bug.cgi?id=292081 Diffs - src/aboutdialog/AnimatedBarWidget.h 8ec32ad src/aboutdialog/AnimatedBarWidget.cpp f40dd6d Diff: http://git.reviewboard.kde.org/r/104464/diff/ Testing --- Tested changed widget correctly wraps text when About dialog is resized. Thanks, Lachlan Dufton ___ 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
