Skip to content

Conversation

@MatthiasKreileder
Copy link
Contributor

@MatthiasKreileder MatthiasKreileder commented Aug 11, 2019

Conditions Breakpoints: UI-support

Breakpoint.jsx: Add a glyph that, when clicked, turns into a text input form. The user can use this input form to specify a break condition.

SourceCode.jsx: Highlight conditional breakpoints in orange, to set them apart from unconditional or disabled breakpoints.

Closes #185

This is the GUI equivalent to the (gdb) condition <bkpt_num> <condition> command.

Here's a screenshot of the UI:

conditional_bkpt

@cs01
Copy link
Owner

cs01 commented Aug 11, 2019

Nice thanks! I can’t promise when I will be able to review, but I definitely will eventually.

@MatthiasKreileder
Copy link
Contributor Author

MatthiasKreileder commented Aug 11, 2019

I can’t promise when I will be able to review

No worries at all! It's not a pressing issue :) We'll see who comes first: me living up to my "I'll migrate gdbgui to create-react-app" promise, or you reviewing this PR.

@cs01
Copy link
Owner

cs01 commented Aug 15, 2019

Okay I checked this out. It works, very cool! I think people will like this feature.

The console displays an error message in red though, when the breakpoint condition is invalid. I believe that should be enough to communicate that the condition was not set correctly?

I agree. If the breakpoint takes, gdb returns it as the cond key, otherwise I assume it's either empty or missing. So since gdbgui always re-renders breakpoints based on that data, the condition the user sees should reflect what gdb is actually doing, and the error in the console should be enough like you said.

There are a few changes it needs before it's merged.

  • When I tried out the conditional input there was a warning about controlled vs. uncontrolled components. They give more info here about what that means and how to fix it. We should be using controlled components. Lmk if you need help. https://reactjs.org/docs/forms.html#controlled-components
    image
  • Check out how Chrome handles conditional breakpoints (it's a quick 1 min video): https://www.youtube.com/watch?v=l6PKY-l6wys. Some things I like about it: The color of the breakpoint changes when it's conditional. Setting the condition is not too "in your face" since it's probably used maybe 10% or less of the time when a breakpoint has been added? (But I'm sure those time it is used it's really helpful) I don't really like how you have to know to right click on the gutter since that makes it hard to discover, but I guess it's kind of an advanced feature. Anyway...
  • Make conditional breakpoints a different color, probably a similar orange as what the Chrome devtools use
  • Replace the input with a bootstrap 3 glyph that indicates "conditional breakpoint" (https://glyphicons.bootstrapcheatsheets.com/). Maybe glyphicon-glyphicon-edit or -share? When hovered, it should say "add/modify condition". When clicked, it should display the input with either the placeholder or the value. When the user types and presses enter the value is saved, the input box turns back to the icon, possibly with a different color indicating the condition is activated, and the color of the breakpoint in the text editor's gutter also changes color (probably to orange). The reason I think this makes sense is because visual real estate is scarce and the amount of space the input takes up is large relative to how often it's used.

Some nits:

  • Use the autoformatter prettier (gdbgui has a .prettierrc file in its root). You should ideally get your text editor to run it on save, but you can also run manually: npx prettier gdbgui/src/js/**/* --write. I just landed format files and add prettier call to nox lint session #290 which ensures all the formatting is up to date, so you will have to pull the latest master into your branch.
  • I think I saw a file where the newline was removed from the end of the file, so make sure your text editor adds a newline at the end of file on save

Thanks again for taking the time to put this together.

@cs01
Copy link
Owner

cs01 commented Aug 15, 2019

I went ahead and merged master and ran prettier.

@MatthiasKreileder
Copy link
Contributor Author

Great! Thanks for the review! I'll read up on the things you pointed out, address the comments and then push an update at the end of this week.

SourceCode.jsx: Highlight conditional breakpoint in orange.

Breakpoints.jsx: Hide the breakpoint condition input form
behind a glyph.  The glyph, when clicked, turns into a text
input field.
@MatthiasKreileder
Copy link
Contributor Author

Just a friendly reminder @cs01: I think I addressed all comments (and ran prettier :)

@cs01
Copy link
Owner

cs01 commented Sep 1, 2019

Thanks, I am out of town this weekend but I should be able to review it this week.

@MatthiasKreileder
Copy link
Contributor Author

MatthiasKreileder commented Sep 1, 2019

Gotcha! I appreciate that this project is time-consuming for you as the maintainer. Any day during September is fine from my side. Thanks for being open to contributions in the first place 👍

@cs01
Copy link
Owner

cs01 commented Sep 10, 2019

Ah I'm sorry. I will get to this! I do appreciate the new feature. Bear with me 🤦‍♂.

@MatthiasKreileder
Copy link
Contributor Author

Hehe, no worries. Thanks for gdbgui (I use it daily).

@cs01
Copy link
Owner

cs01 commented Sep 15, 2019

I checked it out, looks good! I updated some minor UI things (increase contrast on orange breakpoint, make "condition" text italic, bold when set, change font by moving into its own span).

I wanted to give you a chance to review my changes before I merge. LMK if you have any feedback. Thanks!

@MatthiasKreileder
Copy link
Contributor Author

LGTM! I had a look at the final version (after your changes) and I think it's definitely better now! Thanks for reviewing (and improving) this PR!

@cs01 cs01 merged commit c3dabbf into cs01:master Sep 18, 2019
@MatthiasKreileder MatthiasKreileder deleted the conditional_breakpoints_ui_support branch September 18, 2019 17:12
cs01 pushed a commit that referenced this pull request Jul 14, 2020
cs01 pushed a commit that referenced this pull request Jul 14, 2020
cs01 pushed a commit that referenced this pull request Jul 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add conditional breakpoint UI support

2 participants