new widget: table of breakpoints (BreakpointViewer)

[pvmm]

A table of breakpoints that is separated from DisasmView makes breakpoints easier to manage.

TODO:

• fix update issues
• fix widget style properties
• primary field
• secondary field
• join primary and secondary fields
• segment field
• watchpoint table

[pvmm]

Made a little mistake rebasing previously included code, but the github build action alerted me of the mistake. Commit ba0fe8f made it possible.

[m9710797]

Sorry this is not resolved.

[m9710797]

Hi, thanks again for your work. In my last review-pass I only found details. Or I gave remarks about already existing issues in the code (not related to your changes). So I'm close to approving the c++ part of this PR.

However before we can merge it, I would like that someone else takes a look at it, but then from a debugger-user point of view (e.g. does the new feature work like it's supposed to, are there usability issues, ...). I personally never use the openMSX debugger, so I don't think I'm the best person to do this.

@MBilderbeek: could you do this 2nd part? Or delegate to a more appropriate person?

[MBilderbeek]

Perhaps @turbor is a much more appropriate person. Can you take a look?

[pvmm]

It's in draft mode now. I will release it when I finish these last issues.

[pvmm]

Waiting for PR #97

[pvmm]

https://www.githubstatus.com/

Can't push anything right now.

Have you guys considered gitlab?

[pvmm]

This pull request is ready for review. While finishing it I found a new bug that I launched as PR #100. Please fix that first, as it is necessary to completely merge this one. Some new screenshots:

[pvmm]

Rebased on master after PR #100 was applied.

[turbor]

I'm looking at it and playing around with it. I will probably merge this pull request tomorrow.

[turbor]

Here is a quick brain dump from me after using the new breakpoints/debug list widget. These are some of the things I encountered while trying out this feature and how it would fit in my workflow.
As a point of warning: I might not be most generic user of this tool so my remarks might be a big deal to me but might not affect a more common debugger user :-)
All comments below are meant as constructive remarks, but it is late while I'm writing this so...

I have a lot of custom TCL scripts to do my bidding, so to me it is important that the widget doesn't assume to be the sole master of the breakpoints/watchpoints in the openMSX session.
For simplicity sake I will be referring to all variations of breakpoints,watchpoints and conditions under the common moniker 'breakpoints' from here on.

I like the option to enable/disable the breakpoints. Maybe this should be included in openMSX itself as an option. (wink wink nudge nudge)
But if I add a breakpoint using the widget and then in the openMSX console remove an enabled breakpoint it also gets removed from the widget.
One would expect the widget to still list it but have it unchecked.
The other way around, if I use the console to add a breakpoint it shows in the widget as a checked breakpoint, if I uncheck it in the widget it remains in the widget but unchecked
Maybe you could use a tri-state checkbox to indicate a new breakpoint that isn't defined with the widget, so that you can see which are done with the gui and which are done in the console (or by TCL scripts!). This way Qt::PartiallyChecked will indicate a console/TCL set breakpoint and you can bring it under gui control by explicitly checking/unchecking it.

Also it would be nice to see the command of the breakpoint. For now it seems that the widget assumes that the default "debug break" command is always used. The debugger doesn't even seem to recognize breakpoints with an other command?
But I have during testing of my programs used the commands 'set throttle on' and 'set throttle off' in my breakpoints to speed up some initialization code and decrunching of data blocks, simply to speed up play-through tests and speed up switching between game levels.
Of course, since you can have several commands on the same address in multiple breakpoints it might well be that the current underlying structure of the debugger needs to be changed, I didn't look into it but it might assume that there is at most one breakpoint per address.
Heck, it might even be that it is some older code that is preventing to debugger to pick up non 'debug break' breakpoint commands, I didn't take the time to read all the code B-)

Here is something that went wrong.
Under the Conditions tab I entered 5 empty conditions, by simply pressing the 'Add' button 5 times.
Then I entered the condition "[reg E]==0" in one of the blank Condition fields
Then I tried to enable this condition by checking the default unchecked box.
=> now I had only 4 empty conditions in the table.

Here is something annoying.
At first when checking/unchecking a breakpoint the order remained as-is. This is what one would expect.
However the order of the addresses was random so I tried sorting by clicking the table headers. This din't work and had no effect. (please enable this by calling the setSortingEnabled(true) on the view and if needed use a QSortFilterProxyModel)
Now the great annoyance, after clicking on the 'X' header for the check boxes and then checking/unchecking breakpoints it seems that they are now sorted with unchecked first followed by the checked breakpoints. Thus the original fixed order was gone

Also when this happened, editing the Segment column of the first row in the watchpoint tab still worked, but it remained left aligned until I forced a resort by playing with the checkboxes

Also in Qt you can have your table widget stretch the last column to the end of the view automatically. Maybe you wanted the text to be close to the other columns, in such case stretching and left-aligning could be an option.

All in all, this is a great addition to openMSX-debugger.

[pvmm]

Heck, it might even be that it is some older code that is preventing to debugger to pick up non 'debug break' breakpoint commands, I didn't take the time to read all the code B-)

That is pretty much the case. Look at this piece of code inside Breakpoints::setBreakpoints:

                // check and clip command (skip non-default commands)
                int q = bp.lastIndexOf('{');
                if (bp.mid(q).simplified() != "{debug break}") continue;
                (...)
                insertBreakpoint(newBp);

which skips insertBreakpoint() if command is not {debug break}. I chose not to touch it as it goes beyond the scope of this feature, since it looks like it was implemented this way for a reason. So a discussion about why it is the way it is was due to happen later on.

Now the great annoyance, after clicking on the 'X' header for the check boxes and then checking/unchecking breakpoints it seems that they are now sorted with unchecked first followed by the checked breakpoints. Thus the original fixed order was gone

In which table that happens? Since header reordering was disabled that shouldn't be possible. Maybe some attribute in the condition table was missing since condition support was added later.

Also when this happened, editing the Segment column of the first row in the watchpoint tab still worked, but it remained left aligned until I forced a resort by playing with the checkboxes

This is probably because I use resizeColumnToContents() to adjust the size of columns after cell is edited.