Skip to content

Let a file type declare which lines can carry a breakpoint and BreakPointStore is now a Singleton per File - #357

Draft
danielpourbakhsh wants to merge 3 commits into
one-ware:mainfrom
FEntwumS:feature/breakpoint-line-pattern
Draft

Let a file type declare which lines can carry a breakpoint and BreakPointStore is now a Singleton per File#357
danielpourbakhsh wants to merge 3 commits into
one-ware:mainfrom
FEntwumS:feature/breakpoint-line-pattern

Conversation

@danielpourbakhsh

Copy link
Copy Markdown
Contributor

What this changes

Three things about breakpoints in the editor, one commit each. 7 files,
+258 / −7.

  1. The breakpoint moves onto the line number instead of occupying a column of
    it's own. -> Inspired by the UIs of popular and well-kown IDEs
  2. All editors share one breakpoint store, so a breakpoint store outlives the editor
    that set it. -> Fixes Bug Scenario: Same File open in two Editor Instances, with two asynchronous Breakpoint Stores.
  3. A file type can say which of its lines can carry a breakpoint at all. -> Regular Expression added to TypeAssistance, so the Editor can know which Line is Breakpointable

1. Breakpoints on the line numbers

The breakpoint margin sat between the line numbers and the text, costing a
column. Rider and VS Code instead put the breakpoint on the line number itself:
the number gives way to the dot, and the gutter keeps the width the numbers give
it.

BreakPointLineNumberMargin derives from LineNumberMargin and takes over its
slot in TextArea.LeftMargins, so MeasureOverride stays inherited and the
column is exactly as wide as before.

Behaviour change: in a file whose type supports breakpoints, a click on the
line number now toggles a breakpoint and no longer selects the line. That is
what those editors do, and the two meanings cannot share one click. Files whose
type does not support breakpoints keep the stock margin and its selection
behaviour untouched.

BreakPointMargin is marked [Obsolete] rather than removed, so anyone using
it directly keeps working.

Two details that are not obvious from the diff: the foreground colour is read
from the editor, because AvaloniaEdit binds LineNumbersForeground only on the
margin it creates itself and not on one inserted in its place; and the dot
shrinks when it would not fit, so the column never grows wider than the numbers
alone would make it.

2. One shared breakpoint store

SetEnableBreakpoints gave every editor its own BreakpointStore, so a
breakpoint reached nothing beyond the margin that drew it. BreakpointStore.Instance
is now that one store, following ExplorerNameComparer and
TypeAssistanceIconStore in the same assembly.

Behaviour changes, both intended:

  • A breakpoint survives closing and reopening its file.
  • The same file open in two views shows the same breakpoints in both.

A shared store outlives the margins that use it, so both margins now subscribe
for as long as they are attached rather than from their constructor on.
Subscribing once and never detaching would let the store hold every margin of
every closed editor alive and redraw them on each change — harmless while the
store died with the margin, a leak once it does not. The obsolete margin gets
the same treatment, since it is still constructible.

3. A file type declares its breakpointable lines

The margin accepts a breakpoint on every line of a file whose type supports
them. For a language where not every line is executable — a comment, a blank
line, a directive — the breakpoint does not fail visibly. The backend moves it
to the next line that has code, silently, while the dot stays where the user put
it. The dot and the place the target actually halts are then two different lines,
and nothing says so.

ITypeAssistance gains one member:

string? BreakPointLinePattern => null;

A default interface member returning null, so every existing language keeps
its current behaviour and no implementer has to change.
TypeAssistanceBase
exposes it as a protected init property, next to LineCommentSequence.

The rule stays with the file type; the margin only applies it. The check reads
the line text, not the line number, which keeps any one language's syntax out
of the core.

Two questions I would like your read on

Opened as a draft because of these. The code works and builds; both questions
are about where things belong, and you know this codebase and its plugins
better than I do.

1. Where should a target's breakpoint capacity live?

The case that prompted this: the target I debug holds 16 breakpoints in
hardware
— a fixed number of comparators. Today the margin accepts any number
of them, and the seventeenth fails when the session starts, far from the click
that caused it.

What I tried first, and dropped. A MaxBreakPoints member next to
BreakPointLinePattern on ITypeAssistance. It works, and I removed it again,
because it does not survive its own reasoning:

  • A capacity is a property of the target, not of the file type. The same
    source can run on targets with different capacities. Putting the number behind
    a language interface means reaching a hardware constant through the wrong
    contract.
  • It is a per-target resource, but ITypeAssistance is per file type and the
    margin counts per file. Two source files on one target, and the count is wrong.

So the pattern in this PR is a language property and stays; the capacity is not
one and did not.

The direction I would take instead, which needs no new surface:
IDebugSession.SetBreakpointAsync already returns whether the target accepted
the breakpoint, and today nothing looks at that value. A caller could mark a
refused breakpoint unverified and the margin draw it hollow — how VS Code and
Visual Studio show a breakpoint the target would not take. That covers every
reason for a refusal, not just capacity: an address that cannot be mapped, code
that is not loaded, a comparator already in use.

What I am asking:

  • Is the unverified-breakpoint route the direction you would want, or do you see
    a reason to know the limit up front?
  • Is there an existing mechanism in OneWare for the editor to ask something of
    the active target? I did not find one, and I would rather use yours than invent
    a second.

I have deliberately left this out of this PR either way — it needs a debugger to
produce the answer, and there is none in this repository yet.

2. How far does replacing BreakPointMargin reach?

BreakPointLineNumberMargin fully supersedes BreakPointMargin, and after this
PR nothing in this repository constructs the old one. I marked it
[Obsolete] rather than deleting it.

The reason I did not delete it is that I cannot see far enough. It has been
public in OneWare.Essentials since July 2024 and shipped in 68 tagged
releases. Whether a plugin out there builds its own gutter with it is not
something I can determine from here.

What I am asking: can you tell whether anything outside this repository uses
it? If you are confident nobody does, I will delete it in this PR — it is one
commit either way, and 150 lines of code that nothing calls will not age well.

The same question applies in smaller form to the one signature this PR does
change, ExtendedTextEditor.SetEnableBreakpoints. I am happy to keep the bool
overload alongside the new one if you would rather not break it.

The breakpoint margin occupied a column of its own between the line numbers and
the text. Rider and VS Code instead put the breakpoint on the line number: the
number gives way to the dot when one is set, and the gutter keeps the width the
numbers give it.

BreakPointLineNumberMargin derives from LineNumberMargin and takes over its slot
in TextArea.LeftMargins, so MeasureOverride stays inherited and the column is
exactly as wide as it was. A click there now means "breakpoint" and nothing
else - the base class's line selection is deliberately skipped, matching what
those editors do.

BreakPointMargin is marked obsolete rather than removed, so anyone using it
directly keeps working.

Two details that are not obvious from the diff: the foreground colour is read
from the editor because AvaloniaEdit binds LineNumbersForeground only on the
margin it creates itself, and the dot shrinks when it would not fit, so the
column never grows wider than the numbers alone would make it.
SetEnableBreakpoints gave every editor its own BreakpointStore, so a breakpoint
reached nothing beyond the margin that drew it: closing and reopening the file
lost it, the same file open in two views held two unrelated sets, and no
debugger could read any of them.

BreakpointStore.Instance is now that one store, following ExplorerNameComparer
and TypeAssistanceIconStore in the same assembly.

A shared store outlives the margins that use it, so both margins now subscribe
for as long as they are attached instead of from their constructor on.
Subscribing once and never detaching would let the store hold every margin of
every closed editor alive and redraw them on each change - harmless while the
store died with the margin, a leak once it does not.
The margin accepts a breakpoint on every line of a file whose type supports
them. A language whose lines are not all executable has no way to say so, and a
breakpoint on such a line does not fail visibly: the backend moves it to the
next line that has code, silently, while the dot stays where the user put it.

ITypeAssistance gains BreakPointLinePattern, a default interface member
returning null, so every existing language keeps its current behaviour and no
implementer has to change. The rule stays with the file type; the margin only
applies it. Checking the line text rather than the line number keeps any one
language's syntax out of the core.

An invalid pattern from a plugin is logged once and then treated as no
restriction, and removing a breakpoint always stays possible, so one that
predates a rule change can still be taken away. The hover preview follows the
same rule, since a dot that disappears on release is the most misleading
feedback available.

SetEnableBreakpoints now takes the ITypeAssistance rather than a flag, since the
margin needs both values as a unit. The margin's new parameter is optional, so
existing callers keep working unchanged.

Not included: an upper limit per file. A target's breakpoint capacity is a
property of the target, not of the file type, and it is a per-target resource
that cannot be counted per file. IDebugSession.SetBreakpointAsync already
reports whether the target took the breakpoint, which is where that belongs.
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.

1 participant