Skip to content
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

Breakpoints appear as verified if verified by at least one session #55106

Closed
roblourens opened this issue Jul 26, 2018 · 24 comments

Comments

@roblourens
Copy link
Member

commented Jul 26, 2018

I don't know whether this was suggested before. Now we show red breakpoints only when the bp is bound in the selected debug process, and gray otherwise. I think it would be really helpful to be able to tell the difference between a breakpoint that isn't bound anywhere, and one that is bound but in a different process from the selected one. Often I want to check that my breakpoints are bound, and have to check the list of debug processes to make sure.

So we would see something like

  • Red: bound in selected process
  • Dark red: bound in some process, not the selected
  • Gray: not bound anywhere

cc @weinand

@weinand

This comment has been minimized.

Copy link
Member

commented Jul 26, 2018

This proposal makes a lot of sense.
We just have to keep in mind that for accessibility reasons the shape should differentiate the breakpoint state as well.

@isidorn

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2018

Dark red feels stronger than red. Shouldn't that decoration be less strong since it is not currently bound?
PR welcome

Code pointer
https://github.com/Microsoft/vscode/blob/469786a7913ef99199d54683e0187c7e8e7e6f89/src/vs/workbench/parts/debug/browser/breakpointsView.ts#L569

@isidorn isidorn added this to the Backlog milestone Jul 26, 2018
@isidorn isidorn added the help wanted label Jul 26, 2018
@isidorn isidorn removed their assignment Jul 26, 2018
@roblourens

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2018

By "dark red", I'm imagining something like a cross between gray and red. The "bright red" we have should look stronger and "active". I have no idea how the shape should be differentiated.

@vscodebot

This comment has been minimized.

Copy link

commented Sep 21, 2018

This iteration we focus on issue grooming. This issue is being closed to keep the number of issues in our inbox on a manageable level, we are closing issues that are not going to be addressed in the foreseeable future: We look at the number of votes the issue has received and the number of duplicate issues filed. More details here. If you disagree and feel that this issue is crucial: We are happy to listen and to reconsider.

If you wonder what we are up to, please see our roadmap and issue reporting guidelines.

Thanks for your understanding and happy coding!

@vscodebot vscodebot bot closed this Sep 21, 2018
@roblourens

This comment has been minimized.

Copy link
Member Author

commented Sep 21, 2018

This one really bugs me every day, can we keep it?

@weinand weinand reopened this Sep 21, 2018
@weinand weinand removed the *out-of-scope label Sep 21, 2018
@isidorn

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2018

@roblourens I closed this issue since now the breakpoints should always properly show their status depending on the active session.
But sure we can keep this open.

@roblourens

This comment has been minimized.

Copy link
Member Author

commented Sep 24, 2018

But they still are gray when bound, but not in the active session, right? My complaint is that they look the same whether unbound or bound in a different session, and I just want to know that they are bound somewhere.

@isidorn

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2018

@roblourens correct, your complaint is valid

@YisraelV

This comment has been minimized.

Copy link
Contributor

commented Nov 19, 2018

I'm going to try and work on this one.

@YisraelV

This comment has been minimized.

Copy link
Contributor

commented Nov 26, 2018

@isidorn would you object to changing property "verified" of class "BaseBreakpoint" into enum with three values: 1. notVerified 2. verified 3. verifiedInanotherSession.

There are few usages so refactoring would be easy.

@isidorn

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2018

@YisraelV I guess that makes sense if we want this feature request.
However I do not have time to look into this this milestone. If the PR is straight forward and clean please submit it and I can reveiw some time mid December. Thanks

@YisraelV

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2018

@isidorn ok thanks. Are there any issues that would be more helpful to look into?

@isidorn

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2018

@YisraelV this one for example #62851

@YisraelV

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2018

@weinand I'm working on this right now and saw you suggested the shape should be different. Any ideas as to how?

@YisraelV

This comment has been minimized.

Copy link
Contributor

commented Dec 10, 2018

@isidorn it seems to me that regardless of this issue, the code at getBreakpointMessageAndClassName would benefit from refactoring as it is a bit messy.

would you consider, when you have the time, a pull request that made the code cleaner?

I'm adding my ideas so you can understand what kind of changes I'm talking about.

A few points that could use tidying up:

  1. The constants "state" and "debugActive" are declared a few lines before they are used, which creates a confusion about their role.
  2. the function "appendMessage" is used only once, and is declared much before it's used, which is also confusing.
  3. the function "appendMessage" checks for a condition that is always true.
  4. when checking for unsupported breakpoint type, a different coding flow style is used for log point vs conditionals breakpoint, even though the underlying logic is exactly the same.
  5. Through the function there are many conditionals, Including nested ones. This number could possibly be reduced.
  6. the same type of breakpoint gets different css class prefixes based on available css classes. This is confusing. Rather we should give every type unique prefix and add selectors in the css to accommodate all of them.
@isidorn

This comment has been minimized.

Copy link
Contributor

commented Dec 10, 2018

@YisraelV Good points. I am open for a PR that addresses these issues. I would keep that PR seperate from this feature request. Thanks

@pavelfeldman

This comment has been minimized.

Copy link

commented Sep 16, 2019

tl/dr: could we render breakpoint as resolved if it was resolved in at least one of the sessions?

My worry is that the average user won't be able to appreciate the difference between resolved and resolved in the current session states. For example, a full stack developer will be switching to a Node session and all their browser breakpoints will get rendered as half-resolved. User will be wondering if that means those breakpoints were disabled. As a result, we are putting too much burden on them and confuse them w/o bringing much add value.

I like the clear contract where resolved breakpoint is something that will eventually be hit, while unresolved is something that is not going to be hit. It sends a clear signal to the user. Could we try keeping it this way and render breakpoint as resolved if it was resolved in at least one of the sessions?

In order to cover the edge case with the resolved breakpoint not being hit in a different session where it was not resolved, you could render a list of sessions the breakpoint is resolved in on hover.

Another side-note on the session selection UX: it should be clear what switching the session control does and what it does not. Since it does not control the breakpoint behavior, the rendering of the breakpoint should not be changed upon that change. It is important to build a solid mental model of what sessions do and preferably limit it to the changes to the pause/resume control and debugger console selection selection.

@roblourens

This comment has been minimized.

Copy link
Member Author

commented Sep 16, 2019

This is a good point. It would be fine with me, as long as there is still a way for me to tell exactly which sessions it's resolved in.

dgozman added a commit to dgozman/vscode that referenced this issue Sep 17, 2019
…icrosoft#55106

When multiple sessions are active, we show status from each one when hovering
over breakpoint. If breakpoint is not supported/verified in all sessions, we
change the icon to indicate that.
@isidorn

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2019

@pavelfeldman good points and this makes sense to me.
However what would happen if a breakpoint is set on Line 9. The debug session says it should be set on line 10 instead. And the user now clicks on another debug session which has the breakpoint as unverified. Should the breakpoint stay on line 10, or should it jump to its original location of line 9?
How about if a third session verified the breakpoint and it says that is should be on line 15 instead. Changing from first to third should clearly make the breakpoint jump to line 15.

I believe we should use the following heuristic:
If a session says the breakpoint is unverified that means most of the time that it is not interested in that breakpoint. And switching focus to that sessions should make no UX difference to the user regarding the breakpoint. The breakpoint should only get changed if the focus moves to another session which also resolved the breakpoint.

Regarding the PR: I am not a big fan of the complex Hover showing all the breakpoint verified states since I doubt that many users will benefit from it. I would only add it later if we hear from users that they are confused.

@weinand @roblourens @pavelfeldman @dgozman let me know what you think. This is just a pragmatic idea.

@pavelfeldman

This comment has been minimized.

Copy link

commented Sep 19, 2019

Isidor, I like your heuristic idea since it means that the general use case won't be affected. There is a minor counter-argument to your proposal:

In your example, as I select the session, breakpoint jumps between the lines 9, 10 and 15. But no matter which session I select, when one of the sessions breaks, it'll break on its own line. So you make it look like you control which line it'll stop on, but we know that is not your intention. I'm primarily trying to avoid confusions like this for the general case.

How about an even simpler heuristic: if the breakpoint is resolved into a number of different locations in different sessions, you render it in its original location?

I've been dogfooding a full stack multi-session environment and in my use cases, sessions sharing the same files were rare, exotic. Sessions sharing the same files compiled into different targets with different resolved states were non-existent. So I would only be interested in providing a slick experience for the general 95% use case vs barely enabling those interested in differently resolved breakpoints in selected session by less prominent means and UIs.

I also saw your comment on the PR and I wanted to follow up on the following:

  • A lot of our UI is sort of a slave of the focused session. This goes against this

I believe that if we want to make multi-session a common case, we would need to go against that principle and reduce the amount of UI affected by the selected session to a pre-defined minimum. Selected session does not affect any of the system's the behavior, it only affects selected inputs, namely console and pause/resume controller, for good. If we wanted to stick with the principle you refer to all the way, breakpoints would only be set in a selected session at the first place, but we don't do that and for a good reason. Session will be a new concept to a lot of these users and we should only introduce new UX primitives to improve their experience. In this case, it seems de-emphasizing session's importance to merely be a selector that suggests which process my REPL is for and what I'm about to start / stop sounds sufficient. The simpler the rules of the game are, the easier they would be to accept.

WDYT?

@roblourens

This comment has been minimized.

Copy link
Member Author

commented Sep 19, 2019

How about an even simpler heuristic: if the breakpoint is resolved into a number of different locations in different sessions, you render it in its original location?

I think that having a breakpoint resolved in different locations for different sessions is probably a rare edge case. Unless it's more common for a different workflow/language that I'm not familiar with? This solution is probably ok. Or, show it in every resolved location.

@isidorn isidorn closed this in 1c4c8ce Sep 20, 2019
@isidorn

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2019

@pavelfeldman yes, your heuristic simplifies the things even more since now the breakpoints view is not a slave to the focused session in any way.
So I went ahead with our combined proposal. Please try it out in vscode insiders from Monday and let me know how it goes.

Since this was mostly debt / cleaning up work in code I decided to do it right now and drop the PR #81068 even though that one was also a fine approach, I believe tackling it fully on the model level is cleaner. This allowed me to also add some test. As for the nice hover, I suggest that we add it later users get confused.

@roblourens even though we are not adding a new color I closed this issue as I would like that you try the new approach and if you still think we need a new color we can reopen this one.

As for the overall UX as slave to the focused session agree that we should try to simplify the rules of the game and that breakpoints no longer depening on this makes thing simpler and better.

@isidorn isidorn self-assigned this Sep 20, 2019
@isidorn isidorn modified the milestones: Backlog, September 2019 Sep 20, 2019
@dgozman

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2019

@isidorn I like this approach! Played with it and found a small issue. When breakpoint is supported by one session, and not supported by another, we render it differently depending on focused session. Also, rendering in the editor and in the breakpoints list sometimes disagrees (unsupported vs verified). I think that if we move notion of supported to the breakpoint model (similar to verified), this will be automatically fixed.

@isidorn

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2019

@dgozman good catch and you are correct about the way to fix this. To track this issue I have created #81303 and plan to look into it later today / tomorrow.

@roblourens adding verification-needed and you can add verified if you like how this behaves once you try it out. Thanks

@roblourens roblourens added the verified label Oct 3, 2019
@isidorn isidorn changed the title Introduce third breakpoint color to indicate "bound in some process" Breakpoints appear as verified if verified by at least one session Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.