Skip to content

[Debug] Add support for custom labels on breakpoints #1803

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SougandhS
Copy link
Contributor

@SougandhS SougandhS commented Apr 7, 2025

When working with multiple breakpoints in a complex codebase, it can become challenging to quickly identify and focus on key lines of code, especially when dealing with 10+ breakpoints. This PR introduces the ability for users to set custom labels on breakpoints, allowing for easier identification and differentiation. With custom labels in place, users can quickly locate and highlight important breakpoints, significantly improving workflow and debugging efficiency similar to other IDEs.

Right-click on a breakpoint and choose Label

image


Screen.Recording.2025-04-09.at.3.20.35.PM.mp4

Once removed, highlight and custom label will be removed

image

@iloveeclipse iloveeclipse added the enhancement New feature or request label Apr 7, 2025
Copy link
Contributor

github-actions bot commented Apr 7, 2025

Test Results

 1 764 files  ±0   1 764 suites  ±0   1h 29m 34s ⏱️ -4s
 4 381 tests ±0   4 357 ✅ ±0   24 💤 ±0  0 ❌ ±0 
13 143 runs  ±0  12 976 ✅ ±0  167 💤 ±0  0 ❌ ±0 

Results for commit fa32006. ± Comparison against base commit ed41e32.

♻️ This comment has been updated with latest results.

@mickaelistria
Copy link
Contributor

Cool change. Just a note about the dialog: instead of a "Remove" button, I would suggest that we add a note "Leave blank to stick to default".

@SougandhS
Copy link
Contributor Author

Cool change. Just a note about the dialog: instead of a "Remove" button, I would suggest that we add a note "Leave blank to stick to default".

Thanks, modified it now
image

@laeubi
Copy link
Contributor

laeubi commented Apr 7, 2025

instead of a "Remove" button, I would suggest that we add a note "Leave blank to stick to default".

A "clear" button would be useful or use a text-box with a SWT.CLEAR style.

@merks
Copy link
Contributor

merks commented Apr 7, 2025

Cool feature indeed.

Starting the word after a comma with an upper case letter is just not proper style. I also really don't like the phrase "stick to".

How about this?

Provide a custom label, or blank for the default label

@SougandhS
Copy link
Contributor Author

A "clear" button would be useful or use a text-box with a SWT.CLEAR style.
Provide a custom label, or blank for the default label

Thanks for the suggestions, will incorporate these 👍

@SougandhS
Copy link
Contributor Author

Redesigned the dialog box
image

@merks
Copy link
Contributor

merks commented Apr 7, 2025

Then the dialog is opened, is the existing text selected? Because in that case one can simply hit delete or backspace to achieve what the clear button does...

@mickaelistria
Copy link
Contributor

The "Clear" button is indeed redundant (so was the former "Remove" one). I understand @laeubi suggestion was not to create a new SWT button here, but instead to add the SWT.CLEAR style to the Text widget so it has a cute icon on the right to allow.

@laeubi
Copy link
Contributor

laeubi commented Apr 7, 2025

Then the dialog is opened, is the existing text selected? Because in that case one can simply hit delete or backspace to achieve what the clear button does...

While this works, its not always obvious I think also one then needs to press the OK button so

  • one button press (to clear the field)
  • one keypress (to OK)

versus

  • one keypress (to Clear)

but instead to add the SWT.CLEAR style to the Text widget so it has a cute icon on the right to allow.

That would be good in any case if an "empty" textfield has a special meaning.

@mickaelistria
Copy link
Contributor

Clicking "clear" or "remove" while still having the text visible can be much more confusing that just clearing the text and clicking OK. Or the button should be titled "Clear and Apply" and thus "OK" renamed to "Apply"...
The rationale that "less actions the better" is only true when this comes at an almost null price on understandability, readability and risk of confusion. Here a third button, if done right, requires users to read much more content for an operation that's usually trivial to many pieces of software "DEL then ENTER" is a classical workflow that is not worth attempting to simplify by adding more controls to it.

@laeubi
Copy link
Contributor

laeubi commented Apr 7, 2025

What about Reset to default then... I'm not sure if "reading a button" or "reading a label" makes much difference, as a user I would need to read the label to understand how to reset it to default.

As an alternative, one could have a checkbox [x] use default label that disables the textbox if checked. Then it would even be possible to temporary disable the custom label without retype the text again.

@mickaelistria
Copy link
Contributor

I'm not sure if "reading a button" or "reading a label" makes much difference, as a user I would need to read the label to understand how to reset it to default.

The more controls, the more input a user has to deal with to take their decisions, the less serene they are in their usage.

Not so long ago, people where often complaining about Eclipse UI being "heavy", "overwhelming" and so on. Concretely, this was meaning there were too many dialogs with too many controls in them. We've made continuous efforts since then to keep things leans and not add extra weight were the "standard" workflow is trivial to handle.
There is more value to be found in finding ways to get rid of redundant and unnecessary controls, than in adding more controls.

@laeubi
Copy link
Contributor

laeubi commented Apr 7, 2025

Well if you want to go that path then the whole dialog is useless and instead it should be able to edit the label in place by having an edit / clear button next to the label text.

For me a label is also a "control" and just that we make the text longer it does not makes it more "clear" or less "heavy"...

@mickaelistria
Copy link
Contributor

instead it should be able to edit the label in place by having an edit / clear button next to the label text.

That's indeed better.
@SougandhS Do you think you can make this work; similarly to how the rename in Project Explorer works.

@SougandhS
Copy link
Contributor Author

That's indeed better. @SougandhS Do you think you can make this work; similarly to how the rename in Project Explorer works.

I have to check on this

@SougandhS
Copy link
Contributor Author

What about Reset to default then... I'm not sure if "reading a button" or "reading a label" makes much difference, as a user I would need to read the label to understand how to reset it to default.

As an alternative, one could have a checkbox [x] use default label that disables the textbox if checked. Then it would even be possible to temporary disable the custom label without retype the text again.

this looks good approach , if user only wants to highlight the breakpoint leaving the default label as it is
@mickaelistria @laeubi WDYT ?

@mickaelistria
Copy link
Contributor

What label vs what style would IMO better be 2 different issues at the moment. I think with some usage of textual conventions (user chosen; like emojis or uppercase), it will become easy to make a breakpoint highly visible without need for extra style.

@SougandhS
Copy link
Contributor Author

SougandhS commented Apr 7, 2025

What label vs what style would IMO better be 2 different issues at the moment. I think with some usage of textual conventions (user chosen; like emojis or uppercase), it will become easy to make a breakpoint highly visible without need for extra style.

then can we provide checkbox for both (show custom label, Highlight breakpoint ) ? that way 2 issues can be solved. Giving more control

@mickaelistria
Copy link
Contributor

then can we provide checkbox for both (show custom label, Highlight breakpoint ) ? that way 2 issues can be solved. Giving more control

Why a checkbox and not a color picker? How to allow italic vs bold and so on...? What do we really need?
Everything is possible with more options, except the problem of some users being intimidated by too many options... So instead of chasing a universal and powerful feature; I think it'd be better to chase for the simplest solution allowing "quickly identify and focus on key lines of code" in the breakpoints view.
As mentioned, I think just allowing to tweak the label (in place in the tree, without a dialog) would be the "lightest" solution to this problem. Style and so on require more development and usage effort.

@SougandhS
Copy link
Contributor Author

That's indeed better. @SougandhS Do you think you can make this work; similarly to how the rename in Project Explorer works.

Not sure this works because the way resource renaming in the Project Explorer is handled differs from how breakpoints/variables are labeled in the Breakpoints view. The initial default label isn't stored in the same way as updates are done in the Project Explorer, such as with the org.eclipse.ui.actions.RenameResourceAction.queryNewResourceNameInline(IResource) method.

@mickaelistria
Copy link
Contributor

Not sure this works because the way resource renaming in the Project Explorer is handled differs from how breakpoints/variables are labeled in the Breakpoints view.

IIRC this uses a TextCellEditor and how the value is get/set is pretty flexible.

@SougandhS
Copy link
Contributor Author

SougandhS commented Apr 9, 2025

Redesigned similar to rename in functionality
https://github.com/user-attachments/assets/ae0f6f39-f6cd-4f49-a2f4-307d43ef5e6c

image

@mickaelistria

@mickaelistria
Copy link
Contributor

Thanks, that looks good!

@laeubi
Copy link
Contributor

laeubi commented Apr 9, 2025

How is this activated? On click or on Context menu? We should be carefully that people does not activate it by accident.

@SougandhS SougandhS force-pushed the BpLAbel branch 7 times, most recently from d7d6ad8 to e1eafb3 Compare April 13, 2025 03:09
@SougandhS SougandhS requested a review from mickaelistria April 13, 2025 05:11
Copy link
Contributor

@mickaelistria mickaelistria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the changes. I think it's now good enough to merge (as soon as we get a successful CI build).
Please consider preparing some changes to documentation and news to declare this new feature.

@SougandhS
Copy link
Contributor Author

SougandhS commented Apr 15, 2025

consider preparing some changes to documentation and news to declare this new feature.

Sure 👍 Thanks a lot

@SougandhS
Copy link
Contributor Author

Build is green now :)

@SougandhS SougandhS force-pushed the BpLAbel branch 2 times, most recently from 4855a45 to 6d83e4d Compare April 15, 2025 07:47
@SougandhS SougandhS force-pushed the BpLAbel branch 4 times, most recently from ef9763f to e6fc388 Compare April 15, 2025 08:03
@SougandhS SougandhS requested a review from mickaelistria April 15, 2025 08:46
@MohananRahul MohananRahul added the noteworthy Noteworthy feature label Apr 16, 2025
@SougandhS SougandhS force-pushed the BpLAbel branch 2 times, most recently from 67673c3 to 22472e2 Compare April 22, 2025 12:39
@SougandhS SougandhS force-pushed the BpLAbel branch 3 times, most recently from 9a14457 to e3778e6 Compare May 8, 2025 03:44
@SougandhS
Copy link
Contributor Author

Hi @mickaelistria
I have incorporated all the changes and build looks good now

This commit introduces the ability for users to set custom labels on
breakpoints, making it easier to identify and differentiate them.
Additionally, breakpoints with custom labels are visually highlighted,
improving workflow and debugging efficiency.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request noteworthy Noteworthy feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants