D11234: [DrKonqi] Show debug button when KDevelop session is running

[Christoph Roick]

croick added a comment.


  Thanks for your review. It was the last in a series of patches and I'll try 
to advertise the whole "Debug your KDE application with KDevelop"-feature 
somewhere.

REPOSITORY
  R871 DrKonqi

REVISION DETAIL
  https://phabricator.kde.org/D11234

To: croick, #plasma_workspaces, apol, mwolff, #kdevelop, sitter
Cc: kossebau, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D11234: [DrKonqi] Show debug button when KDevelop session is running

[Christoph Roick]

This revision was automatically updated to reflect the committed changes.
Closed by commit R871:95b6b8b613c2: Show debug button when KDevelop session is 
running (authored by croick).

REPOSITORY
  R871 DrKonqi

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11234?vs=61377=61456

REVISION DETAIL
  https://phabricator.kde.org/D11234

AFFECTED FILES
  src/drkonqidialog.cpp
  src/drkonqidialog.h

To: croick, #plasma_workspaces, apol, mwolff, #kdevelop, sitter
Cc: kossebau, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D11234: [DrKonqi] Show debug button when KDevelop session is running

[Harald Sitter]

sitter accepted this revision.
sitter added a comment.
This revision is now accepted and ready to land.


  Heh. Fair enough, I too ran down the wrong rabbit hole with the size issue 
until I noticed the policy was wrong.
  
  Diff looks good now. Works very well too. 

REPOSITORY
  R871 DrKonqi

BRANCH
  nicebutton

REVISION DETAIL
  https://phabricator.kde.org/D11234

To: croick, #plasma_workspaces, apol, mwolff, #kdevelop, sitter
Cc: kossebau, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D11234: [DrKonqi] Show debug button when KDevelop session is running

[Christoph Roick]

croick marked 2 inline comments as done.
croick added a comment.


  In D11234#492200 , @sitter wrote:
  
  > Could you explain your thinking behind the adjustSize call please? The way 
I see it the window should scale up as necessary if the size hints are properly 
set (which they should be on master).
  
  
  That's true, but only since your latest commits. Before, the text of the 
report button was cut off (in german for example) once the debug button got 
activated. I removed the call again.

REPOSITORY
  R871 DrKonqi

REVISION DETAIL
  https://phabricator.kde.org/D11234

To: croick, #plasma_workspaces, apol, mwolff, #kdevelop, sitter
Cc: kossebau, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D11234: [DrKonqi] Show debug button when KDevelop session is running

[Christoph Roick]

croick updated this revision to Diff 61377.
croick added a comment.


  - more style fixes
  - do not adjustSize after adding the button, since that is redundant now

REPOSITORY
  R871 DrKonqi

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11234?vs=59627=61377

BRANCH
  nicebutton

REVISION DETAIL
  https://phabricator.kde.org/D11234

AFFECTED FILES
  src/drkonqidialog.cpp
  src/drkonqidialog.h

To: croick, #plasma_workspaces, apol, mwolff, #kdevelop, sitter
Cc: kossebau, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D11234: [DrKonqi] Show debug button when KDevelop session is running

[Harald Sitter]

sitter added a comment.


  Could you explain your thinking behind the adjustSize call please? The way I 
see it the window should scale up as necessary if the size hints are properly 
set (which they should be on master).

INLINE COMMENTS

> drkonqidialog.cpp:203
> +//Report bug button: User1
> +QPushButton* reportButton = new QPushButton(m_buttonBox);
> +KGuiItem2 reportItem(i18nc("@action:button", "Report "),

- goes to the right of the space.

> drkonqidialog.cpp:210
> +
> +bool enableReportAssistant = !crashedApp->bugReportAddress().isEmpty() &&
> + crashedApp->fakeExecutableBaseName() != 
> QLatin1String("drkonqi") &&

could probably just do away with this and merge the condition into the 
setEnabled call below. the variable doesn't seem to do much in the way of 
readability.

REPOSITORY
  R871 DrKonqi

REVISION DETAIL
  https://phabricator.kde.org/D11234

To: croick, #plasma_workspaces, apol, mwolff, #kdevelop, sitter
Cc: kossebau, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D11234: [DrKonqi] Show debug button when KDevelop session is running

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Ping?

REPOSITORY
  R871 DrKonqi

REVISION DETAIL
  https://phabricator.kde.org/D11234

To: croick, #plasma_workspaces, apol, mwolff, #kdevelop, sitter
Cc: kossebau, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D11234: [DrKonqi] Show debug button when KDevelop session is running

[Christoph Roick]

croick added a comment.


  Thanks for looking into this :D
  
  Past self obviously still had to read the style guide and tried not to change 
header files.
  I took your commit cc640cea 
 as 
an example to resize the dialog after inserting or deleting the button.

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

D11234: [DrKonqi] Show debug button when KDevelop session is running

[Christoph Roick]

croick updated this revision to Diff 59627.
croick marked 5 inline comments as done.
croick added a comment.


  - Fix code style.
  - Save state of the debug button in a member variable.
  - Resize the dialog after inserting/removing the debug button.

REPOSITORY
  R871 DrKonqi

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11234?vs=59394=59627

BRANCH
  nicebutton

REVISION DETAIL
  https://phabricator.kde.org/D11234

AFFECTED FILES
  src/drkonqidialog.cpp
  src/drkonqidialog.h

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

D11234: [DrKonqi] Show debug button when KDevelop session is running

[Harald Sitter]

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 debuggers = 
> debuggerManager->availableExternalDebuggers();

CamelCase

> drkonqidialog.cpp:193
>  foreach(AbstractDebuggerLauncher *launcher, debuggers) {
> -addDebugger(launcher);
> +if (showexternal || qobject_cast(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 buttons;

CamelCase

Should be `= false`

> drkonqidialog.cpp:250
> +QVector 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

D11234: [DrKonqi] Show debug button when KDevelop session is running

[Christoph Roick]

croick updated this revision to Diff 59394.
croick added a comment.


  Rebase to current master

REPOSITORY
  R871 DrKonqi

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11234?vs=29245=59394

BRANCH
  nicebutton

REVISION DETAIL
  https://phabricator.kde.org/D11234

AFFECTED FILES
  src/drkonqidialog.cpp

To: croick, #plasma_workspaces, apol, mwolff, #kdevelop
Cc: plasma-devel, LeGast00n, ericadams, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D11234: [DrKonqi] Show debug button when KDevelop session is running

[Christoph Roick]

croick added a comment.


  A few screenshots:
  
  Before the patch:
  
  - `ShowDebugButton=false` in drkonqirc F6567349: beforepatch_1.png 

  - `ShowDebugButton=true` in drkonqirc F6567353: beforepatch_2.png 
 F6567358: beforepatch_2_german.png 
 Note how the button is just floating.
  
  After the patch:
  
  - `ShowDebugButton=false` in drkonqirc, but with KDevelop running F6567361: 
afterpatch.png 

REPOSITORY
  R871 DrKonqi

REVISION DETAIL
  https://phabricator.kde.org/D11234

To: croick, #plasma_workspaces, apol, mwolff, #kdevelop
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D11234: [DrKonqi] Show debug button when KDevelop session is running

[Christoph Roick]

croick added a comment.


  I know that DrKonqi hardly pops up these days. But still, if this button 
became more popular, then D11235  and 
D11236  might get some attention as well...

REPOSITORY
  R871 DrKonqi

REVISION DETAIL
  https://phabricator.kde.org/D11234

To: croick, #plasma_workspaces, apol, mwolff
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11234: [DrKonqi] Show debug button when KDevelop session is running

[Christoph Roick]

croick added a comment.


  bump

REPOSITORY
  R871 DrKonqi

REVISION DETAIL
  https://phabricator.kde.org/D11234

To: croick, #plasma_workspaces, apol, mwolff
Cc: plasma-devel, ragreen, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D11234: [DrKonqi] Show debug button when KDevelop session is running

[Christoph Roick]

croick created this revision.
croick added reviewers: Plasma: Workspaces, apol, mwolff.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.
croick requested review of this revision.

REVISION SUMMARY
  - independently of the config setting in drkonqirc propose debugging with 
KDevelop if a session is available
  - default debugger is only displayed if ShowDebugButton=true in [DrKonqi]
  - button is the first action button to be displayed
  - fix adding/removing of the button to the button box

TEST PLAN
  - set ShowDebugButton=false, have no KDevelop session running
  - force a crash (using src/tests/crashtest for instance)
  - DrKonqi shows no debug button
  - start a KDevelop session
  - the button appears with the KDevelop session selectable
  - quit DrKonqi, set ShowDebugButton=true
  - crash again, the button now always is visible, including the default 
debugger as a selection

REPOSITORY
  R871 DrKonqi

BRANCH
  nicebutton

REVISION DETAIL
  https://phabricator.kde.org/D11234

AFFECTED FILES
  src/drkonqidialog.cpp

To: croick, #plasma_workspaces, apol, mwolff
Cc: plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart