openMSX / debugger

31 stars 15 forks source link

cleaning up BreakpointViewer when symbols are deleted #126

Closed pvmm closed 2 years ago

m9710797 commented 2 years ago

Although I think your patch is technically correct, IMHO it's not very robust. So I'd like to (first) ask you to consider some alternatives.

The current patch strong depends on the string value of the tool-tip. Who knows, maybe in the future we might want to show additional information, and then this implementation breaks.

Also there might be more than one label with the same numeric value. If only one of such labels is removed, it might be a good idea to not show the numeric value, but instead some other label with the same value.

So instead of using the tool-tip-string, I would suggest you use a more 'stable' way to retrieve the address. I think it should be possible to get the address from the actual breakpoint object? Or maybe you know of a better way.

Then I propose you run the same function that is used to lookup the label for breakpoints entered via openMSX itself (not via the debugger) .... Hmm, actually do you already try to use labels (instead of plain address) for such breakpoints?

m9710797 commented 2 years ago

Ah, and when new labels are entered, it would be nice if those are automatically used in the breakpoint viewer (if the address happens to match).

pvmm commented 2 years ago

The current patch strong depends on the string value of the tool-tip. Who knows, maybe in the future we might want to show additional information, and then this implementation breaks.

OK, I fixed this one.

Also there might be more than one label with the same numeric value. If only one of such labels is removed, it might be a good idea to not show the numeric value, but instead some other label with the same value.

Since there is no distinction on what kind of change the SymbolTable is going through, we are assuming that the user always wants a label instead of an address if SymbolTable goes through any kind of change. So I think this goes counter to the current functionality of labels in BreakpointViewer, in which the user specifically tells the debugger what he wants by writing a label or an address in BreakpointViewer widget. So doing what you propose seems a bit arbitrary to me.

If you are actually proposing we write specific events to deal with different SymbolTable changes, can we at least break it down into different PRs?

pvmm commented 2 years ago

Then I propose you run the same function that is used to lookup the label for breakpoints entered via openMSX itself (not via the debugger) .... Hmm, actually do you already try to use labels (instead of plain address) for such breakpoints?

This involves using openMSX 18 new breakpoint watcher feature. I have a different branch scheduled for dealing with that.

m9710797 commented 2 years ago

You're right, let's not do everything in one PR. I've already pulled this part. I would be very happy if you're willing to continue working on the other parts :-)

My reason is the following:

I think the disassembly view works like this: whenever there's a label available for a certain jump-target, that label is shown instead of the numerical address (it would be nice if the address can still be shown in a popup, but that's less urgent because you still see the raw bytes in the previous disassembly view columns).

Personally I think it would be nice if we could do this not only for jump-targets, but everywhere we show addresses in the debugger. So concretely if we show an address in the breakpoint viewer, we try to show that as a label. Even if the breakpoint was entered as a numeric value.

Another way to say the same thing: the breakpoint viewer should (in a way) be "stateless". That is: when you throw away all the information from the breakpoint viewer widget, and then repopulate it from the breakpoint information you query from openMSX (plus the information of the Symbol table in the debugger), the result should be the same as at the start.