Debugger BreakpointWidget: Edit breakpoints dialog #11293
Conversation
|
Can someone explain to me why my previous work managed to make its way into this PR? It didn't show those commits on the branch I had made before... I know they will probably have to be cleaned out before this gets merged in. |
|
Are you comparing your branch to master on your personal repo? Because your repo has master with extra commits. Make a new branch from origin/master (main dolphin repo) and then cherry-pick the commits you want from this branch. Then rename things and re-push. As for the PR, we don't need to duplicate the NewBreakpointDialog into an edit dialog. Just add an option to populate NewBreakpointDialog on creation. |
|
@TryTwo If so, should we change the name to be more generic, like BreakpointDialog? It doesn't make sense to me to have a specialized editing window exist in a dialog that is named for creating a new breakpoint. |
JoshuaMKW
force-pushed
the
edit-breakpoints-dialog
branch
2 times, most recently
from
d6ad968
to
15d1dda
Compare
|
Alright, cleaned up those junk commits, and now I have learned something new! Once you answer the question I have above, I'll do whatever it is that you decide and hopefully we can get this merged in. |
|
Yeah, just |
|
I don't mind if you change the name. I think it might be coded too literally. Isn't it less complex to just delete the existing breakpoint then submit a new one? Rather than adding code to edit an existing one. |
I wanted to preserve the order of breakpoints in the list, that way it doesn't move to the bottom after every edit. |
|
I'll test it out once the buildbot does its thing. |
|
I believe it is ready now, at least I've resolved all review changes |
|
I'm not sure if you need any help fixing any of those build issues. Let me know if you do. |
|
I just have to edit the CMakeLists, as I forgot to |
|
Should be good now |
|
There were also some lint issues in BreakpointWidget, could use an auto-format pass. Only approved people can call a build. |
|
-_- building is frustrating. I'll format that file |
|
There we are, hopefully all is well |
|
Ok, I'll test it tonight. |
|
Seems to work as expected for me. Since making a new breakpoint has the combined code + memory bp interface, I'm not sure it's necessary to trim the UI down when editing. Functionally, I wonder if we should consider the case of wanting to move a conditional to a new address. Right now, you would: Edit -> copy condition -> cancel -> make new BP -> paste the condition -> save -> delete the old BP. If the address wasn't locked, you could just Edit -> change the address, but then it would make a completely new bp and not delete the old one. |
I wanted to double down on the idea of what editing a breakpoint is, and reflect that in the UI. Every time I've ever needed to edit a breakpoint so far, it was to change it from read to write, change the conditional, etc. I've never intended to edit a breakpoint to change the address it is targeting. Also, it does complicate the editing stuff if we allow the address to be changed as it just creates a new one as you've noticed. We would need to store another member or two to preserve that state to know that we should remove the old breakpoint which is now at a different address. IMO it's a lot of extra work for something I feel doesn't align with the purpose of this. |
|
Also, on top of never wanting to edit the start address, I've also never needed to change an instruction BP to a memory BP |
|
Think you still have some lint issues. From the previous build: I'm okay with the way this PR is, it adds important functionality. Just brainstorming additional stuff here. Another thought I had was we could do away with the breakpointdialog entirely and just make the breakpointwidget table editable with buttons for read/write and break/log. That could be a longer term goal. As for this PR someone else should give it a quick review, since I'm no expert at reviewing code. |
|
Retested and saw no issues. I'm good with it. I'll see if I can get someone else to do a more thorough coding check. |
AdmiralCurtiss
force-pushed
the
edit-breakpoints-dialog
branch
from
1684008
to
dd22823
Compare
This PR allows users to edit existing breakpoints by opening the context menu of a breakpoint and clicking
Edit.... This also makes it so when you add a new breakpoint that shares the same address as an existing one, the existing breakpoint becomes the new one that the user has just created.