sitter requested changes to this revision.
sitter added a comment.
This revision now requires changes to proceed.
addDebugger seems a bit much, no? Why don't you just have a
m_debugButtonInBox member to track whether it is currently added or not? When
it is already in the buttonBox running the for loop over the buttons is
entirely unnecessary.
INLINE COMMENTS
> drkonqidialog.cpp:190
> + // Only add the debugger if requested by the config or if a KDevelop
> session is running.
> + const bool showexternal = debuggerManager->showExternalDebuggers();
> QList<AbstractDebuggerLauncher*> debuggers =
> debuggerManager->availableExternalDebuggers();
CamelCase
> drkonqidialog.cpp:193
> foreach(AbstractDebuggerLauncher *launcher, debuggers) {
> - addDebugger(launcher);
> + if (showexternal || qobject_cast<DBusInterfaceLauncher*>(launcher))
> + addDebugger(launcher);
We always use curly braces for ifs in Plasma
> drkonqidialog.cpp:248
> + // inserted, then add the other buttons. See removeDebugger below for
> more information.
> + bool hasdebugbutton{false};
> + QVector<QAbstractButton*> buttons;
CamelCase
Should be `= false`
> drkonqidialog.cpp:250
> + QVector<QAbstractButton*> buttons;
> + for (QAbstractButton *b : m_buttonBox->buttons()) {
> + if (b == m_debugButton) {
No single letter variables please.
> drkonqidialog.cpp:261
> + m_debugButton->setVisible(true);
> + for (QAbstractButton *b : buttons) {
> + m_buttonBox->addButton(b, QDialogButtonBox::ActionRole);
No single letter variables please.
REPOSITORY
R871 DrKonqi
REVISION DETAIL
https://phabricator.kde.org/D11234
To: croick, #plasma_workspaces, apol, mwolff, #kdevelop, sitter
Cc: plasma-devel, LeGast00n, ericadams, jraleigh, GB_2, ragreen, Pitel,
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas,
apol, mart
