MR - DebuggerIntf for terminal window

[Martin_fr]

This contains questions @Matthew from https://gitlab.com/freepascal.org/lazarus/lazarus/-/merge_requests/702#note_3631251914

First of all a general note (for all readers here / already mentioned on the issue):

The package IdeDebugger is an internal part of the IDE. And it will stay such, in terms of "don't use it outside the IDE - it will continually get breaking changes without announcement, and without grace period". It's considered "working as designed" if it compiles and runs with the IDE build from the matching commit. No compatibility.

As I expressed on the linked issue: Access for add-ons will be done in the package IdeIntf, in units like IdeDebuggerIntf.


A bit of background on some existing ideas, based on "if it was a perfect world" (given the 24hour limit to each day, that is currently not fully planed):

While IdeDebugger will always be compiled in the IDE, the IDE would ideally one day be able to compile completely without this package. An add-on should be able to register as a complete replacement.

So in that perfect case we would need yet another Interface:
- IdeIntf: IdeDbgFrontendIntf => an interface to the IDE, which is used by IdeDebugger (or replacements) to hook into the IDE
- IdeDebuggerIntf: plugins that want to hook into the package IdeDebugger, and extend the frontend provided by it (albeit, menu entries in the IDE are going through existing IdeIntf units)
- LazDebuggerIntf / DebuggerIntf: any talk to the backend

From https://gitlab.com/freepascal.org/lazarus/lazarus/-/merge_requests/702#note_3631251914

TDebugTerminalProvider is an abstract base with class-level identity so the settings UI
can query without instantiating — ProviderName (stable id), DisplayName, Description,
Supported — plus a per-session lifecycle Attach / Detach / Clear / AddOutput /
BringToFront, with input raised through an OnSendInput event. A registry singleton
holds RegisterProvider / FindByName / DefaultProvider. Providers may publish a
settings bag (TPersistent, RTTI-serialised, same shape as TDebuggerProperties), which
an options frame renders in a property grid; the selection and each provider's settings
persist in the environment options. The built-in window is wrapped as one provider and
stays the default. Session lifecycle is driven from DebuggerChangeState — dsInit →
Clear + Attach, dsStop → Detach.

The substantive difference from RegisterStdInOutStreamHandler: mine is user-selected
rather than first-handler-claims. With a chain, which terminal you get depends on
registration order, which the user can't see or change — and with two terminal packages
installed, both wanting the console, there's no way to express a preference. Selection
makes it explicit and persistent.
Your chain is the better fit for handlers that want a slice rather than the whole stream
(a logger tee, say), and those two aren't exclusive — selection could decide the display
while a chain handles the rest. I don't have a strong view; I'd just rather not rebuild it
twice.

GUI to choose active handler

So, if you registered your handler, then what should happen?
- Where should the "user selection" GUI appear?
- Which package provides the code for the  "user selection" GUI?

By default, no such choice exists. By default the IDE (or IdeDebugger) does not need that Code/GUI.

First feeling would be that the package that wants the user to choose should hold that code.
But on 2nd thought, if many packages want that, then that isn't practical.

Btw, on a technical level, the simple RegisterTargetIOStreamHandler does not prevent user selection. It just brings the same question as above.
- If you register a stream handler, you also would register a menu entry to open your window.
- You can then also register a page for "Tools > Options": In that page you can have an active checkbox, and you only claim the stream in that case. (You can also toggle the "active" directly from the window).

Of course an "active checkbox" in an option dialog is inconvenient if you have several 3rd party windows like that, as you don't have a single place to see and change the current active handler.

But point is, choice can be added for the user to select which handler takes the content.  (So long as all handlers play nice / you install a package with a rogue one and all bets are off)

So API for handling data and API for register/user choice can be separated.
Of course the exact "how to present the choice" still needs to be discussed. Ideas welcome.

* Maybe first of all, where/how would you have displayed the choice to the user?
* Will such a split of API work for the ideas you had? What would not work?



As for what hooks can be intercepted

Attach / Detach / Clear / AddOutput /
BringToFront, with input raised through an OnSendInput event. A registry singleton holds RegisterProvider / FindByName / DefaultProvider

The latter methods are for "GUI chooser". I think that should be a hook of its own.

"BringToFront": That is currently triggered in the IDE/IdeDebugger code, but not sure, if the window should decide that itself when it receives content? We may have to define which additional data that would need.

"Attach" / "Detach":
- You mean if the user changes settings while the debug session is running?
- Or you mean "Start/End" of debug session (Run, or Attach to running app)?
- That is a signal send from the IdeDebugger to the window?

"Clear": who triggers that? Is that the same as "start new debug session?


But in any case, if we go with RegisterTargetIOStreamHandler then the registered handler should be a corba interface (so it can have different methods)

Just a sample draft

Code: Pascal  [Select][+][-]

1. ITargetIOStreamHandler = interface
2.   procedure StartingDebugSession(...)
3.   procedure StoppingDebugSession(...)
4.   procedure HandleTargetStreamData(...)
5. end;

Albeit, maybe the first two (and others like debugger state change) are generic. Either in a base interface class, or registered separately.
- I.e. The IdeDebuggerIntf could have "RegisterDebuggerStateChangeHandlers" => then the package needs to register to handlers. (Or the debugger needs to call state changes on this list *AND* on specific handles like the IOStream-Handler.


I can also see your point about a list of TargetIOStreamHandler (logging, rather than exclusive).

* We could have one "DisplayIOStreamHandler" (property for only one event). The add on must set itself, when it becomes selected (see point on "GUI chooser")
* A list (not even needed right now, but can be done later)

Neither changes the separation of
- GUI chooser
- Data Handling

Of course a list would allow to have 2 display handler active at the same time. Sounds strange, but whenever I think "Who would", I soon found someone who did.

Also if the list has priorities, filters can be created.

On the other hand, only active handlers should be called. Some apps produce large amount of output, and if the IDE calls lots of inactive code, that has an impact. So ever a list would mean that add-ons need to register/unregister depending on the user having them active.

So add this point, I am open to either : Single handler or list.


One more: if the hook registers an interface, then methods on the interface can take the data, and extra info via an interface providing callbacks. E.g. see LazDebuggerIntfExceptions => when a notification is sent, the sender includes an interface that can provide extra info.
- new methods can later be added
- info does not need to be pre-computed, but only gets done if queried.

[MattBradford]

Answering the two GUI questions first, with a screenshot — because this part
already exists. Not as a proposal for this discussion: it is working code from
another project that predates it. Reacting to something real is cheaper for you
than reacting to my description of it.

**Where the choice appears, and which package provides the code**

The IDE provides it, once. A page under Tools > Options > Debugger ("Debug
Terminal"): a dropdown listing every registered handler that reports itself
supported on this platform, and beside it a property grid bound to whatever
settings the selected handler publishes. A registering package supplies no GUI
code at all — it supplies a stable name, a display name, a description, a
"supported" flag, and optionally a settings object. The grid is generic and
renders that object through RTTI, the same way the debugger backend properties are
already rendered.

That is the answer to your second thought, and I think you had already reached it:
it is not practical for every package to carry chooser code, and a per-package
"active" checkbox leaves the user with nowhere to see what is currently active.
Both problems go away if the chooser is written once, in the IDE, and packages only
register.

Selection persists in the environment options, and so does each handler's settings
object, keyed by the handler's stable name — so switching away from one and back
does not lose its configuration.

**The second entry in that dropdown is not a demo**

It is the console panel from an embedded RISC-V debugger package of mine, extracted
so it depends only on DebuggerIntf, IDEIntf, SynEdit and LCL — nothing IDE-private.
It has settings a user genuinely has to change: line-ending translation, local
echo, backspace handling, colours. That is why I care about the selection question
rather than treating it as bikeshedding — I already have two terminals installed in
one IDE, both wanting the stream, and I need to be able to say which one gets it.
It is the case you can't express with registration order.

**Attach / Detach / Clear / BringToFront — what they mean in mine**

Attach and Detach are **start and end of a debug session**, not settings changes.
They are driven from DebuggerChangeState: dsInit calls Clear then Attach, dsStop
calls Detach. So yes — a signal sent from the IDE side down to the window. A
settings change while a session is running I deliberately do not propagate; the
live settings object is refreshed once per run, so an edit takes effect on the next
run. That seemed the less surprising behaviour, but I have no strong case for it.

Clear is triggered by the IDE at dsInit, so in practice it is "new debug session".
It is a separate method only because I wanted "empty the display" to be callable
without implying a lifecycle change.

BringToFront is the one I would keep on the IDE side, and your instinct to question
it is right in a way that argues against the window deciding: whether the window
should pop up is a *user preference*, and the user's preferences live in IDE
config, not in the window. In my embedded backend the window is fed continuously
but only auto-pops when the user asked for it, so "data arrived" is probably the
wrong trigger. If the window did decide, the minimum extra information it would
need is whether this is the first output of the session and what the user's
auto-open preference is — at which point the IDE may as well make the call.

**Does the data / choice split work — and what does not**

The split works, and in fact it is already how mine is put together: the chooser
only ever touches name, display name, description, supported, and the settings
object; the stream path only ever touches the session and data methods. Splitting
them into two registrations costs me nothing.

One thing does not map cleanly onto a corba interface, and it is worth saying
before anyone builds it: the settings object. It is a TPersistent streamed by RTTI,
which is what lets the generic property grid render a handler's settings without
knowing anything about that handler. An interface cannot be streamed that way. The
fix is small — the interface exposes a method returning the TPersistent, and the
settings stay an object even though the handler is an interface — but if the
settings object gets dropped along the way then the chooser page loses the half
that makes it worth having.

The related wrinkle: the chooser has to list handlers that are not instantiated,
which is why mine puts identity and Supported at class level. With interfaces you
would either register a factory, or register a live instance and accept that
construction happens at registration time. The second is fine if construction is
cheap — mine creates no window until Attach — but it is a decision, not a detail.

**Single handler or a list**

If it helps to have a preference stated: one *selected* display handler, plus an
optional list of passive observers that never claim the stream. That gives you the
logger tee and the filters without reintroducing the ambiguity, because exactly one
thing is ever responsible for displaying. Your performance point argues the same
way — with selection, exactly one handler is live by construction, and there is no
inactive code to call.

I would not build the observer list now. Nothing needs it yet.

**On the shape generally**

None of the above is me defending my implementation. If a corba interface in
IdeDebuggerIntf is where this goes, I will rebuild onto it — it is the right
placement by your layering and I have no attachment to the class hierarchy. The one
thing I would argue for keeping, whatever the mechanism, is that the chooser is
written once in the IDE rather than once per package. Everything else is yours to
shape.

[Martin_fr]

Many good points. Few (one) question left open before I go into details.

Suppose we did exactly what you describe. Then the registered alternative "terminal" needs a window in which to show the apps output.
You said "no GUI", but don't you at least need the window in which you have the memo to show the content (and maybe buttons, or other controls, for the user to change formatting)?

If it is not "auto popped up" then how does the user get it to show?
- Would it have its own menu entry in the "debug windows" list?
- Or is it supposed to take over the entry of the existing "console"?

And in either case, what happens if the console is open? Does it idly stay open (and vice versa if the user makes the console active, after using the terminal)?

If it should "take over the space", does that work with/without docking? Does it re-use the enclosing TForm, or replace the entire TForm => what if it needs more space ? What if the replacement has 2 floating windows?

[MattBradford]

Yes, the package supplies its own window. I was unclear there: "no GUI code" meant
no chooser and no options page, not no window. A provider brings its terminal
window and whatever controls it wants inside it. What it doesn't bring is a page in
Tools > Options, or an active checkbox.

The behaviour I'd expect as a user is the obvious one: one debug terminal visible
at a time, one menu entry that shows it, one settings slot. Choosing a provider
changes what that slot contains, not how many slots there are. The built-in window
is just the provider that occupies it by default.

But I'd get there without any form takeover. Each provider registers its own window
through IDEWindowCreators under its own window id, so docking position, size and
float state are remembered per provider by the machinery that already does that.
The single slot is the menu command and the options page, not a shared TForm.
Selection decides which window that one command opens, and deselecting closes the
outgoing one.

That sidesteps most of what you asked. Nothing re-uses or replaces another window's
TForm, so there's no question of what happens when the replacement needs more room,
and a provider that wants two floating windows can have them — the slot is a
command, not a container.

What happens today, honestly: the outgoing window stays docked where it was and
simply stops receiving anything. That's a bug, and your question is what found it.

The cause is worth spelling out because it's the same ambiguity you spotted
earlier. I have two different events collapsed into one. "This provider is now the
active sink" and "a debug session has started" both arrive as Attach, and only
session-stop calls Detach — so deselecting a provider never tells it to stand down.
When you asked whether Attach/Detach meant a settings change during a run or the
start and end of a session, I answered start and end of a session. The better
answer is that there are two events there and I had merged them. Splitting them is
what fixes the orphaned window, and it also gives the chooser something to call
when the user changes the selection with no session running at all.

So if this ends up as registered handlers rather than my providers, I'd suggest the
same split: something for "you are now / no longer the active handler", separate
from "a session started / stopped". They fire at different times and a handler
plausibly cares about them differently — mine wants to close its window on the
first and keep it open on the second.

One thing I don't have a good answer for. If a user has one terminal docked
bottom-right and switches to another provider, should the incoming window inherit
where the outgoing one was sitting, or keep its own remembered position? Layout
kept per window id gives you the second for free, and that's what I have. But
someone thinking of it as one slot might reasonably expect the first. That's the
one place where the single-slot idea leaks, and I don't have a strong view — if
you do, I'll follow it.

[Martin_fr]

Ok, while I am working on the big reply, some minor ideas concerns are coming to mind. I place them into this separate post...


one or multi

It is possible someone wants to see the output in raw and terminal at the same time. => of course a single provider can have a split view, but if the 2 formats are from diff providers, then I would not be surprised if someone wanted both open...

Same for other Windows, e.g. alternative Stack view (which could display a different stack)

We do not have to do that now, but keep the possibility open.


About BRINGTOFRONT, I guess you have it working... So ok. I wonder because docked and none docked IDE have different ways to do that. But if it works => fine.

Yes, the primary decision is in the IDE. Or well shared.

- The IDE knows the config
- The IDE can query if the window is visible
- The IDE knows there is content coming
BUT
- The IDE does not know if that content will be shown (and if not, bring to front may be wrong)
- The IDE needs also to know if the Window had been open during the current debug session (for the once only feature) / That can be queried, but may be expensive.


Mind, I have seen debugged apps flooding the IDE with so much output, the API needs to be designed for high throughput. Doing lots of extra calls for each piece of data received...



When the user changes provider... (replace close/re-open the window).

Once we have a plan, I will want some serious testing commitment on that (docked and none docked IDE).

I mainly use undocked, and I hardly use "saved desktops" myself.

The latter worries me if it works.
E.g.
- I have a saved "debug desktop" (that will be activated on debug). It has the "console" Window OPEN and in a certain place.
- I change to "terminal" provider
- I press F9 and start debug
- The IDE changes to the saved desktop

That desktop wants to bring up the wrong dialog.

And imho they can't share the same ID, different providers may use different amounts of windows. Or no proper Window, just a task-icon hint....

So the IDE "desktops" will know them as different.

One thing does not map cleanly onto a corba interface, and it is worth saying
before anyone builds it: the settings object

Yes, that can be returned as object from one of the methods on the interface.
If needed, see my next reply.
See the example on how that is handled with a TFrame.

The related wrinkle: the chooser has to list handlers that are not instantiated,

See how it is done for DisplayFormatters. (package IdeIntf)

[Martin_fr]

OK, lets have a go.  (IDE and IdeDebugger are mixed up below, usually both mean IdeDebugger)

TLazDbgIde / ILazDbgIde are the prefixes for the IdeDebuggerIntf (see e.g. IdeDebuggerValueFormatterIntf)
Names are not final...

Those are still "ideas pitched". Since you already have a working implementation, you will know better how well those ideas could work, or what they miss. So feedback please.


1) OK, for now lets start with "one provider at a time" and lets skip any list broadcast for now.

2) Provider selection gui

Yes, that needs to be in the IDE. (or IdeDebugger probably).

I just want to solve one part here => there will be many other windows that can have alternative views/providers (almost any can have). There already is CPU View by Alexander Bagel that may register its windows.

Obviously I don't want to keep reinventing the wheel. If the IDE allows for a dozen windows to register and most IDE wont have any of them registered, then most IDE wont show any of those choosers... So it would be good to have something generic, that can be re-used for each of them (with properties to adapt for different needs). It can start out for just the one, but designed to grow.
 
This may mean to make the registration using a "functionality independent" interface.

If all add-ons (for all windows) use the same interface for registration, then the same class (list-class) can be used for storing them (and the GUI can be extended for re-use).

E.g.:
- one interface ILazDbgIdePlugIn (may be a base class) to be in the "named list" provides meta data (name, description, availability/support
- one interface ILazDbgIdeTargetIoDisplay  (better word for display needed) that has all the methods that are called during debugging

2a) Activating/Switching (user selects)

The IdeDebugger needs to have a hook "SetCurrentTargetIoDisplay" (or similar).

When an entry in the GUI is selected, the GUI could set this. The ILazDbgIdePlugIn would have a method returning it.

But maybe it be better to instead call ILazDbgIdePlugIn.HandleUserActivation(ListId). The plug in may want to hook more than one part of the IdeDebugger. (It could even be registered in several lists)
When SetCurrentTargetIoDisplay is called by the plug in, it will inform the old handler NotifyRemoveFromHook, and the new Handler NotifyAddedToHook

As the original/current window is on the list too, it would be de-activated, and the switch over would work.

Well, I do still have to sleep that over... E.g. valueformatter, backendconverter, ... they all have there own. But then replacing existing dialogs => that feels like a shared task for all dialogs...


3) CONFIG / RTTI

I am actually dissatisfied with the current backend property grids.... But not enough to make it a priority. Ideally each of them should provide a frame, and all the options shoud be nicely presented in that frame.

And other registrable extensions already have that. Look at unit IdeDebuggerValueFormatterIntf (package IdeIntf): ILazDbgIdeValueFormatterSettingsFrameIntf provides a frame.

And TLazDbgIdeValueFormatterRegistryEntry = class / class function GetSettingsFrameClass: TClass;  // that is TFrameClass, but no LCL dependency here.

So, I would prefer a solution like that.  An individual add on can still just put a grid on the frame... Not nice, but the add-ons choice.

4) color config

Just because I saw it in the image... Of course that can be part of the config frame.

Watches,Locals and ASM currently have their colors as part of the editor-colorscheme. Advantage, if the user picks a dark scheme, they follow. Disadvantage, the preview doesn't really work. And its awkward to have it split from other options, if there were other options.
Afaik, Its possible to register dummy highlighters for any such added color-set....

I am not sure how good an idea it is to extend that... But wanted to mention it.

Parts of the color picker exist as frame. (but not in the INTF package). So not sure if they can be provided for re-use in custom config frames.

ILazDbgIdeTargetIoDisplay

5) **Attach / Detach / Clear / BringToFront

I would name them differently... But that is once we have a shared general idea.

Also undecided if that should be 1 or 2 (or more) functional Hooks in IdeDebugger (the plugin may have to call multiple Set/Add).

The dbg-state is generic notifications, lots of extensions may want to know about them. So a RegisterDebuggerStateChangeEvent(ILazDbgIdeDebbugerStateChangeHandler) could be added.

Should the final hander for "console events" just inherit that... Sounds good at first, but there may be other similar generic notifications. And there is ONLY ONE base class.

Btw, I am facing the same decision for other API parts (e.g. IIRC getting watch values)

About debugger state => the current states are a mess. They need to be re-done someday. So the API may have some translated calls.


Ok, I hope I didn't miss anything.

Depending how well that works we may be able to start defining methods for the interfaces.

About the "templates" you see in some of the existing. The idea is that all interface are allowed to get new methods.

If an actual class today uses the interface it does not have the new methods. But if it inherits the template, then they will be defined there with defaults (slight chance of naming conflicts remains).

Similar in some cases the IDE passes info as interface, so the receiver then needs to call on the interface to get the info
- extendable
- computation on request only

[MattBradford]

Taking the two posts together. Short version first: I think everything you describe
is right, and I'd like to build it as a copy of the ValueFormatter pattern rather
than as the generic version, at least until there is a second consumer.

On the generic plug-in list. I agree with the goal and I'd rather not be the reason
you end up with a dozen chooser dialogs. But looking at what's in the tree today,
value formatters and backend converters each have their own interfaces, their own
registry and their own options page, and the shared thing doesn't exist yet. If I
build a generic ILazDbgIdePlugIn now it will be designed against exactly one real
consumer — mine — and the odds of it fitting CPU View or an alternative stack view
are not good. The failure mode is that it becomes a shared thing everyone has to
work around rather than a shared thing anyone wants.

So my suggestion is: I mirror IdeDebuggerValueFormatterIntf as closely as I can, so
that the console registry is structurally the same shape as the ones already there.
If you later decide to fold them into a common list, folding several registries that
already look alike is a mechanical job, and by then you'll have two or three real
consumers to design against instead of one. That also matches your own "it can start
out for just the one, but designed to grow".

To be clear about who builds what: the generic list is yours to design, and I'm not
proposing to write it. It's a decision about the IDE's extension architecture as a
whole, it has to serve windows I know nothing about, and you're the one who will
maintain it.

What I will do is keep the split you described, so that folding mine in later costs
you as little as possible: one interface carrying identity and metadata — name,
description, supported, settings frame class — and a separate one carrying the
methods called during debugging. Your generic list would consume the first and never
need to know about the second. If your eventual version has a concept mine lacks,
ListId being the obvious one, that looks like a method gained rather than a structure
rebuilt.

If that split isn't what you had in mind, tell me now and I'll follow whatever shape
you want on the metadata side — that half is effectively your API, I'm just an early
implementor of it.

What I'm changing from what I showed you, either way:

- Contract moves from components/debuggerintf to a new unit in IdeIntf, with the
  ILazDbgIde / TLazDbgIde naming.
- Corba interfaces instead of my abstract base class.
- Settings via a frame class, following ILazDbgIdeValueFormatterSettingsFrameIntf
  and GetSettingsFrameClass, instead of my RTTI property grid. Your dissatisfaction
  with the current grids is fair — mine is one of them.
- Listing without instantiating: I'll use the DisplayFormatter registry-entry
  approach rather than my class-level identity scheme.

One thing that fell out of reading that unit: GetObject / GetDefaultsObject for
TXmlConfig.WriteObject is close to what my settings bag already does, so the
persistence side survives the change almost unaltered. Good.

On activation: NotifyAddedToHook / NotifyRemoveFromHook is exactly the split I was
missing, and it fixes the orphaned-window bug structurally rather than by me
remembering to close things. Nothing to add.

Saved desktops — you're right and I hadn't considered it. A saved debug desktop that
restores the console window while a different provider is selected is a real
conflict, and it gets worse because the desktop is applied on F9, after the user has
made the selection. I don't have an answer yet. I agree the window ids can't be
shared, so the desktop will legitimately know them as different windows; the question
is what the IDE does when the desktop asks for a window whose provider is no longer
active. I'll take the testing commitment, docked and undocked, and I'll come back
with something concrete rather than guess in this post.

Throughput: agreed, and it bears on BringToFront. If the per-chunk path has to stay
cheap then "was this window already opened during this session" shouldn't be queried
per chunk — it wants to be a flag the IDE clears at session start and sets on first
open. Cheap to keep, no call into the provider at all.

One at a time, and no broadcast list, suits me. I'd only note that nothing in the
single-slot arrangement prevents the multi case later: since each provider already
owns its window id, two being open at once is a policy change rather than a
structural one. So I don't think we're designing it out.

On colours in the config frame — happy either way. Your point about the editor
colour scheme following a dark theme is a good one and it is the sort of thing I'd
get wrong if I invented my own. If the colour picker frame can be made reusable from
IdeIntf I'd use it; if not, mine can live in my frame and be the poorer for it.

[Martin_fr]

I will respond in detail / and start laying out my ideas of the API too.... But, not tonight.

One quick additon

Corba interfaces instead of my abstract base class.

Just rememberered => The registration of the ValueFormatter uses a class => advantage no instance needed.
But it is also kept minimalistic.

Code: Pascal  [Select][+][-]

1.   TLazDbgIdeValueFormatterRegistryEntry = class
2.   public
3.     class function CreateValueFormatter: ILazDbgIdeValueFormatterIntf; virtual; abstract;
4.     class function GetSettingsFrameClass: TClass; virtual; // class(TFrame, ILazDbgIdeValueFormatterSettingsFrameIntf)
5.     class function GetDisplayName: String; virtual; abstract;
6.     class function GetClassName: String; virtual; abstract; // Used in XmlConfig
7.   end;
8.  

So we can have the exact same (except the first method) for the console.

Does that miss anything that is needed, just to register the plugin?
It has all to display it in the config, and to allow the user to pick one entry.

Every other bit of function goes over the interface (or the frame)

Well, maybe some SupportFlags may later be added...

----------------------

Also Except for the first method that would then be truly generic.

And my fault that I did not reuse that with "TLazDbgValueConvertRegistryEntry"....

---------------
I haven't fully read your part on the generic / not generic yet (its late here)

But I think the above is an example for "could be generic"?

The whole registration could be a generic, so all you need is a new class, to specialize with.

The display/gui does maybe not need to be yet, but could later easily be changed to be reused.

[Martin_fr]

each have their own interfaces, their own registry and their own options page

I do agree, I did not set a good example... I think however that there may be some points were the new generic can be achieved at low cost...

So my suggestion is: I mirror IdeDebuggerValueFormatterIntf as closely as I can,

Good starting point.
I already pointed out the "class" used for the registration of the "user selectable name" for each item.

To iterate a bit more on this. When I started I meant to go interfaces. The main reason was that whoever implementing any addition is not forced to any abstract base class. And that a single class can implement several interfaces.

But - and that is important to keep in mind when deciding what goes onto the register-able class - the registration item doesn't have real functionality of its own. It is only a bit of constant data. A (not advanced) record would have done too.
Only with a class, there is that nice way of a generic that (almost) makes it a one-liner to define your registration entry.

the question
is what the IDE does when the desktop asks for a window whose provider is no longer
active.

Unfortunately, I don't have much answers to contribute here. As I said, personally I do not use the desktops. We will have to see what options come up here.

BringToFront. If the per-chunk path has to stay
cheap then "was this window already opened during this session" shouldn't be queried
per chunk — it wants to be a flag the IDE clears at session start and sets on first
open. Cheap to keep, no call into the provider at all.

First though: It probably needs a mix: the plugin needs to inform on state changes, such as it is considered visible or not. => But I don't want to much state keeping burden...

2nd thought: The IDE sets the state to the plugin, the plugin reacts on receiving data.  I.e. the IDE would (on init debugger, and when the user changes the option, or when the plugin acts) call
  Plugin.SetAutoShowState(ShowOnInput: Boolean);
  // there isn't a "on first data received
  // the plugin calls the IDE when it did autoshow // plugin may also have to call when it hides
  // then the IDE updates the plugins state (the plugin otherwise keeps that state)

So the plugin always just has a state true/false. The IDE maintains if the the "first autoshow" was done and if a 2nd should be done.

On colours in the config frame

That will take me some more time / so maybe later...


So for the implementation, I suggest the following (if you come find anything impractical, or think you have a better idea, I am still glad to hear - with concrete example)

Very first actual draft....

Code: Pascal  [Select][+][-]

1. type
2.  
3.  
4. // base class
5.   ILazDbgIdeDebugPlugIn = interface {guid}
6. // probably those base entries, if they are needed for the config save/load
7.  
8. // was GetObject
9.     function  GetConfigObject: TObject;  // for TXmlConfig.WriteObject / must have all config in published fields
10.  
11. // GetDefaultObject may not be needed anymore, because TRTTIXmlConfig can handle property defaults // IIRC
12. ////    function  GetDefaultsObject: TObject;  // for TXmlConfig.WriteObject / all published fields with DEFAULT values
13.  
14. // need to when those 2 where needed.... / but I think the config frame will need them.
15. // maybe the can move to the config object // base class or interface....
16.     function CreateCopy: ILazDbgIdeValueFormatterIntf;
17.     procedure Free;
18.  
19.     // this one is for extensions
20.     // a plugin can have optional functionality through extra interfaces
21.     // The IDE can try to get them, and if they are returned then use them
22.     function GetInterface(const iidstr : shortstring;out obj) : boolean; // provided by TObject
23.   end;
24.  
25.  
26.   TLazDbgIdeDebugPlugInRegistryEntry = class
27.   public
28.     class function CreateIdeDebugPlugin: ILazDbgIdeDebugPlugIn; virtual; abstract;
29.     class function GetSettingsFrameClass: TClass; virtual; // class(TFrame, ILazDbgIdeValueFormatterSettingsFrameIntf)
30.     class function GetDisplayName: String; virtual; abstract;
31.     class function GetClassName: String; virtual; abstract; // Used in XmlConfig
32.   end;
33.  
34.  
35.   // The registry already is generic, so it can be reused
36.   // but we need one new one, that does not have the "name of what it does" in it.
37.   generic TLazDbgIdeDebugPlugInRegistry<T: TLazDbgIdeDebugPlugInRegistryEntry> = class(specialize TFPGList<T>)
38.   public
39.     function FindByPlugInClassName(AName: String): T;
40.   end;
41.  
42.  
43.   // copy of ILazDbgIdeValueFormatterSettingsFrameIntf // just for ILazDbgIdeDebugPlugIn
44.  
45.   // The console plugin
46.  
47.  
48.   ILazDbgIdeDebugConsoleWindowPlugIn = interface(ILazDbgIdePlugIn)
49.   // Your proposed list of function, based on what is needed
50.   // Mind that debugger state, should be on a separate interface
51.  
52.   // some may move to base class..
53.   // If you have a better name...
54. { names should indicate if they are a user action, or something else}
55.     procedure HandleUserSelectedAsActive;
56.     procedure HandleUserDeselectedFromActive; // no action needed, can react to remove from hook, if hook is only action // may or may not be called even if it was not active
57.     procedure ProcessAddedToPlugingHook;
58.     procedure ProcessRemovedFromPluginHook;
59.  
60.     procedure HandleUserShow; // menu picked
61.  
62.     //... ReceiveText etc
63.   end
64.  
65.   TLazDbgIdeDebugConsoleWindowPlugInRegistryEntry = class(TLazDbgIdeDebugPlugInRegistryEntry)
66.     // same result as CreateIdeDebugPlugin // just correct type
67.     // not sure if/how to improve?
68.     class function CreateIdeDebugConsolePlugin: ILazDbgIdeDebugConsoleWindowPlugIn; virtual; abstract;
69.   end;
70.  
71.   generic TLazDbgIdeDebugConsoleWindowPlugInGeneric<_BASE: TObject> = class(_BASE, TLazDbgIdeDebugConsoleWindowPlugInRegistryEntry)
72.   // implements defaults for the interface
73.   // so if the interface gets extended, existing plugins continue to work
74.   end;
75.  
76.  
77.   generic TLazDbgIdeDebugConsoleWindowPlugInRegistryEntryGeneric
78.   end
79.  

Then need 2 further separate API
1) SetCurrentTargetIoStreamHandler
2) AddDebuggerStateChangeHandler(...) and Remove....

The receiving handler could be a method or an interface / Sorry, still need to decide.

I need to review on which object those registrations should be done. Will revert with more info later.


For info:
There probably should be an object/interface representing the current IDE-Debugger.

The Plugins (and value formatters) register to global lists. And that is OK. They are not specific to a debug session.

But the 2 above are specific to a specific debugger  (well the first is semi specific, but I think it needs to be treated as if)

At some time in the future, I want the IDE to be able to have multiple debuggers running at the same time. (eg. debug client/server would benefit from that).
Then there was more than one debugger....

well writing that, that makes registration complex, because you need to know when a debugger is created....

So probably also global lists. But they then need a param representing which debugger did sent them...

In any case it can't be fully prepared, because it also either needs 2 windows open, or changing displayed content.... And that will not be in the current API. But basics such as sending the "Sender" can be in the current API.

So let me reflect on that.



In the meantime, we can review the above proposal.

[Martin_fr]

Back to to the question
How many instances of each provider

Lets say you have a provider that has different emulation for all sort of different terminals. Even the ability to individually select specific parts of a standard. Basically a lot of work to configure it.

Then if you have debug, you may need it with different configs. So then you would need to save different "instances" of that provider. (that is what value formatters do).


Disadvantage, the formatter can no longer be identified by e.g. its domain/classname. It needs a UID (or sha).

But then if any choice of provider is stored in the project (not the session, the LPI / or the LPI contains the session), then for a shared project only one party may have that config.

- A UID would be created by who creates the config => no one else ever has it, unless there is export/import.
- An sha could exist, but requires really exact same config. Potentially including the name, otherwise the IDE has a problem if a user tries to save the same settings under a new name (as copy).

Of course, even "single instance", even if identified by classname may be missing on other peoples IDE installations. But its easy to  solve, just install the provider.


So what should it be?
Also, e.g. the build-in is so simple => does it want to support that?


It can be added later, but then some existing config has classnames as UID, and that will not trigger a "I may be different" warning if it loads into another IDE.


I don't have a particular preference here, nor do I think its to much of an issue if it later needed to be changed from one to many. (Or if each plugin decides which side it wants to be on)

Or any other idea.

But just wanted to be sure its been considered upfront. In case.

[MattBradford]

Sorry for the delay here — most of this got answered in !709 while the forum sat
still, which is my fault for splitting the conversation. Closing both open points.

Instances, and the identity question from your last post

You answered this one yourself on the MR, and I think it is the right answer:

instead of a UID we store a domain/class. package/class

So one registered entry per provider, identified by package plus class, and a
plugin that wants several configurations either offers an internal chooser on its
own settings frame or registers itself more than once under different names.
Nothing at the Lazarus level needs an instance concept, and a shared project
resolves in any IDE that has the package installed.

And to your question about whether the built-in wants it: no. Single instance,
'IdeDbgConsole', no configurations to multiply.

That also has a consequence for the draft below, which is the one substantive
thing I would change.

GetClassName should carry the domain

In the registry entry you sketched:

Code: [Select]

class function GetClassName: String; virtual; abstract; // Used in XmlConfig
If that returns a bare class name then two packages that both call their class
TTerminalWindow collide in the stored config, which is the thing package/class
exists to prevent. Either it returns the qualified 'MyPackage/TMyTerminal', or
there is a second class function for the package part and the registry joins
them. I marginally prefer the second, because the package name is then not
something each implementor can typo, but either works and it is your call.

Worth deciding before anything ships a stored id, since changing it afterwards
means a migration.

What the registration entry is missing

You asked whether it misses anything needed just to register. One thing:
availability. A provider that only works on some platforms — or only with some
backends — needs to say so at class level, before instantiation, so the chooser
can filter or grey it out rather than offering something that will not run. You
mentioned SupportFlags as a maybe; I think it is a yes, and cheap now.

Everything else in the entry looks right to me: create, settings frame class,
display name, id. It is the same shape as the value formatter entry, which is
what I was going to mirror anyway.

Two slips in the draft

Flagging these before they get typed into a real unit:

- ILazDbgIdeDebugPlugIn.CreateCopy returns ILazDbgIdeValueFormatterIntf, carried
  over from the copy source.
- ILazDbgIdeDebugConsoleWindowPlugIn inherits from ILazDbgIdePlugIn, but the base
  above it is declared as ILazDbgIdeDebugPlugIn.

On CreateCopy itself — you were unsure whether it is needed. For a single-active
choice it is only needed if the options dialog has to support Cancel after the
user has edited a provider's settings, which it does. So I would keep it, on the
base, returning the base interface.

The data method, while it is still elided

Your draft leaves it as "//... ReceiveText etc". One request before it is fixed:
give it a channel parameter from the start, even though there is only one channel
today.

Capture currently means poStderrToOutPut, so stdout and stderr arrive merged, and
you already made the case for why merged has to remain available — it is the only
form that preserves the order the program wrote in. But you also raised separating
them, and a provider that colours stderr differently is the obvious consumer. If
the method takes a channel now, that separation is later a backend change only. If
it does not, it is an interface change affecting every plugin.

Costs nothing today: one parameter that is always the same value until it isn't.

The rest

Debugger state changes on their own registration rather than inherited into this
interface — agreed, for the reason you gave: there is only one base class and
other generic notifications will want the same treatment.

The activation split — HandleUserSelectedAsActive / HandleUserDeselectedFromActive
separate from ProcessAddedToPluginHook / ProcessRemovedFromPluginHook — is exactly
what my implementation was missing, and it fixes the orphaned-window bug
structurally rather than by my remembering to close things.

Auto-show: the state machine you described, where the IDE sets the plugin's
show-on-input state and the plugin calls back when it has shown itself, has no
slot in the draft yet. It probably wants one on this interface rather than on the
generic base.

I have no further concerns on the plugin-registration half, and I am happy to
start building the console provider against it whenever you are content with the
shape. The hook registration is the part you said you wanted to reflect on, and I
am not waiting on it — the provider side can be written against the registration
entry and the display interface, and pick up the hooks when they settle.

Nothing here is urgent. !709 is merged and the run-params side stores the id
already, so there is no half-finished state sitting in the tree.

[Martin_fr]

GetClassName should carry the domain

Indeed, yes. And change of name.

In valueformatter it appears to be only used to be stored in the XML, and then looked up by FindByFormatterClassName

So for the console registry, it could be

Code: Pascal  [Select][+][-]

1. class function GetClassIdentity: String; virtual; abstract; // Used in XmlConfig

And the FindByClassIdentity.

I kept the class, because normally its one by class. In e.g. SynEdit name patterns use "XML_Name" for a unique not translatable ID. But XML is just one form of storage, and ID can also have other uses than storage.

Better naming ideas welcome.

I don't know about split retrieval.

Authors can still feely modify both parts. If we use TObject.ClassName, they can still change that to whatever the want by changing their source. And then they can never change that if they would have a need to.

Technically there still is a risk of class. We could get the unit name (full dotted if need). That can't be duplicated, or it will clash. Only units can be renamed too.

Domain seems reasonable safe.

Btw, I see domain as
- Part1: packagename or companyname
- / slash
- class name (with or without T), potentially a meaningful replacement.

However: no only Pascal identifiers should be allowed.

The registry could assert the naming requirement: [a-z_]{5,}/[a-z_]{5,}
Potentially allowing a 3rd postfix /[a-z_]+

What the registration entry is missing

Generally ok, but lets see, if we can future proof it a little.

Just throwing out some ideas...

We just learned that sometime just "supported" isn't enough.  The plugin would only know what platform it was compiled on. That may already be enough, if e.g. it compiled in IFDEF against the Windows API, and otherwise is just an empty dummy.

- Backend would come into play if it would have extra functions that directly call the backend.
- Target, no idea, but never say never.
- Other, who knows.

And yes: Flags. May be different things that are on/off


Name are dummy

Code: Pascal  [Select][+][-]

1. function Supports(AnDbgSetupIntf: interface): T___Flags

The problem then is, that future interfaces need more methods for the plugin to be able to query stuff, and the class is fixed, so casts are needed, and the plugin needs a way to know what casts are allowed (against what Lazarus version it is compiled)

Or we go with the superset again. Because again, the user may want to make the choice when he has the "run params" (or IDE/project opts) open. The user doesn't want to need to first go to change the backend, and then come back. That is more work.

But then in any case the final decision may need to be made when the debugger starts (can be implemented when needed). If at that point, something doesn't fit, the user needs to be told then. However at that point an instance of the plugin would be available.

So probably superset, and later a 2nd function on the instance.

Two slips in the draft

For the base class, since Dbg is in the name it does not need to be repeated, the short one wins: ILazDbgIdePlugIn

"CreateCopy". Not sure if it will be needed, if there is only one instance per class. Otherwise the return needs to be fixed.

The data method, while it is still elided

Yes, to "channel param".

For the mixed case, maybe the name should be based on "Unknown"

Since eventually the backend should sent that channel (through the method it uses to sent the text), lets define it in LazDebuggerIntf

I'll have to check for naming ideas later...

[MattBradford]

Layout, and what a saved desktop does

The question I left open — whether an incoming provider's window inherits where
the outgoing one was sitting — and your saved-desktop problem are the same
missing concept. The IDE has layout per window id, and nothing that represents
"the debug terminal, whichever one that currently is".

So: a slot. Providers keep their own window ids, as you said they must —
different providers have different numbers of windows, and one may have none.
What gets added is that a provider registers its window into a named slot, and
the IDE stores a layout against the slot as well as against the ids. Lookup when
a slot window is created is: the provider's own override, then the slot layout,
then the provider's default.

Seeded or tracked: I would track. Whenever the active provider's slot window is
moved or resized, that becomes the slot layout. Seeding only on first show means
the providers diverge again after one switch, which is the behaviour I have now,
just postponed. Tracking gives "the debug terminal stays where I put it" across a
provider change, which is the thing the single slot is for.

The override is for a provider that genuinely cannot live in the inherited
geometry. I would make it a minimum size rather than a boolean — the slot layout
applies, clamped to what the provider says it needs — with a hard "ignore the
slot" flag as the escape hatch for whatever I have not thought of.

Two cases that would otherwise misbehave quietly:

- A provider with no window must not write to the slot layout when it is
  selected. Otherwise switching to a task-icon-only provider and back loses the
  position.

- A provider with several windows applies the slot to one primary window. The
  rest are ordinary windows with ordinary ids, stored as such.


Saved desktops

The desktop case then falls out of it. The IDE knows which window ids are
registered into the terminal slot. When a desktop is applied, a window belonging
to that slot whose provider is not the active one is suppressed, and the active
provider's window opens instead, using the layout the desktop stored for the
slot.

Your F9 sequence becomes: the desktop says "terminal slot, open, bottom right",
the selection says which one, and the console that is no longer active never
appears.

It also migrates for free, which is the part I like. Existing desktops contain
IdeDbgConsole. If the built-in registers into the slot like any other provider,
those desktops read as "slot open, here" with no migration step, and their stored
geometry becomes the initial slot layout.

The cost, so it is on the table now rather than found later: desktops become
provider-agnostic. Someone who wants a saved desktop pinned to one specific
provider cannot express that. I think it is the right trade — a desktop is a
layout, not a debugger configuration — but it is a decision, and not obviously
mine to make.

I will take the testing on this, docked and undocked, saved desktops included.


Naming

Better naming ideas welcome.

Your own reasoning argues against Class as much as against ClassName: it is not
the class name, it is not only for XML, and the second segment may be a
meaningful replacement rather than a class at all. GetPlugInId and FindByPlugInId
say what it is and survive all three. If you want the format visible in the name,
GetQualifiedId.

On the assertion pattern: [a-z_]{5,} rules out digits and any package name under
five characters, which will catch somebody. Per segment I would allow
[A-Za-z_][A-Za-z0-9_]* and compare case-insensitively on lookup — a stored id
differing only in case should not read as a different plugin. And whether the
third segment is permitted at all is worth settling now rather than discovering
it when someone ships one.


Channel

Unknown over Merged, agreed. Merged capture is one cause, and a backend that
simply never distinguishes is another, so Unknown is the honest name for both:
the backend did not tell us. I would not add a separate value for genuinely
merged — no provider can do anything with the difference.

TLazDbgTargetIoChannel = (Unknown, StdOut, StdErr), in LazDebuggerIntf, with the
backend sending it eventually. Agreed on all three.


The smaller points

ILazDbgIdePlugIn — yes, shorter wins.

Supports as a superset at class level now, with a second query on the instance
once the debugger starts, works for me. The instance-time decision is the one
that matters, and it is the one that can be reported to the user properly.

CreateCopy I would keep, even with one instance per class. It is not for copying
providers — it is for Cancel in the options dialog. The settings frame edits
something, and Cancel has to discard that without the live object having been
touched. That is what the value formatter uses it for as well. On the base,
returning the base interface.

[Martin_fr]

On the first part of your message, I am not quite sure how to read it...

But if it proposes, to change how the desktop code (or the IDE window placement in general) works, then the issue is that I don't maintain the code. Not sure who does, or if no one actively does, or if anyone in the team wants to take that on right now.

If, so, then I should probably have made that part clearer. For now we need to find some behaviour that works with the existing code (existing desktop / IDE window management / ...). Well or to find someone in the team who has the time to take that on.

[Martin_fr]

"PluginId" is fine.

case should never mismatch, but case-insensitive compare is fine. It forbids "name stealing" by just changing the case.

Restrictions can always be eased, but should not latter be hardened. I would allow the 3rd part, but not a 4th.

TLazDbgTargetIoChannel = (Unknown, StdOut, StdErr),

dtcUnknown => all enum members have prefixes from the type name they belong to. In this case I think "Dbg Target Channel). 
I think the "d" debug is in most of the similar enums.
(same for the other values)

ILazDbgIdePlugIn

Actually, class level wont work on an interface? It needs an instance to implement it.
So class level would then be on the registration class itself.

It might also be good if this method (both) can return an error/info text so the user knows why it isn't working. (e.g. "only works for remote debugger")

Then again, maybe instead of implementing them now "just in case", doing a forward compatible interface would be smarter.

On the registration class, it can always be added, if we have an abstract base class, to which we can add the default value later.

On the interface that is what the "generic template" is meant to do. So long as all plugins use that, then if an entry is added, the template provides the default answer.
Its not perfect, there can be name clashes, when stuff is added. An no one is forced to use the templates.

I am just not really sure, if we add the "supported" to the interface today, we don't know all the values that it may need to know. We can pass in something that we can extend, but we add a lot of work now, to try and make it compatible, and may still end up having missed the case that will later be needed. (sorry 2nd thought now).