Navigate double-clicked sidebar addresses by content type - #1138
Conversation
Double-clicking an address in a debugger sidebar now navigates the current pane when the target is the same kind of thing it already shows (both code, or both data) and the other pane otherwise, so following a data pointer no longer hijacks the code view and repeated double-clicks stop drifting across panes. A right-click "Navigate in Current Pane" action forces the focused pane. The graph/linear token double-click and the TTD time-travel columns are unchanged. Fixes #1134 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fuzyll
left a comment
fuzyll
left a comment
There was a problem hiding this comment.
Some of this seems superfluous/confusing, and I am concerned about a possible usability issue with the pane-deciding code. The rest seems fairly straight-forward, though.
|
|
||
| View* view = frame->getCurrentViewInterface(); | ||
|
|
||
| // Decide same-pane vs other-pane by whether the target and the current view are the |
There was a problem hiding this comment.
This seems really fragile. Are we not able to know what the view is here? Also confused because some views show both code and data. So, wouldn't this code potentially navigate the wrong pane in that situation?
For example, if you have Hex View open and Linear View open and you click an address inside of a function, Hex View can show "code", so I think this would navigate the Hex View and not the Linear View?
| } | ||
|
|
||
|
|
||
| bool TTDEventsQueryWidget::addressForCell(int row, int column, uint64_t& addr) |
There was a problem hiding this comment.
My LLM says the following:
This function does not recognize the specialized Exception Events Program Counter column. Its name does not contain the contiguous substring PC, and its physical index is not ExceptionPCColumn. Consequently, the new “Navigate in Current Pane” action is disabled for that address.
...and then followed up with:
The same code treats column index ModuleAddressColumn as an address regardless of the specialized table schema. In Module Events, that physical index is Module Checksum, so showing that normally hidden column enables “Navigate in Current Pane” and interprets its checksum as an address. Column recognition needs to account for m_widgetType instead of reusing the AllEvents indices.
| if (!selectedValue(value)) | ||
| return; | ||
|
|
||
| // Navigate to the target, opening it in the other pane when it is a different kind |
There was a problem hiding this comment.
This comment is above every single call to NavigateToAddress. This is not a good idea. Not only do we already have this content within the function itself, but we also have it in the .h file. If the implementation ever changes, we have to update that information in two places plus every single location that calls it. That's way too much.
Just leave the header and function comments and remove all of the extra comments above every call to it.
| if (sel.empty()) | ||
| return false; | ||
|
|
||
| const QModelIndex& index = sel[0]; |
There was a problem hiding this comment.
You re-read the index here, but I thought QTreeView::doubleClicked would already provide this? Seems like we should be plumbing the index from that through to a function like this, rather than re-reading it? I believe this pattern is probably in other places, too, and I'm not sure if I'm just missing something or if this is doing work it really shouldn't be.
2 participants
Double-clicking an address in a debugger sidebar (register value, breakpoint, module, memory region, stack frame, debugger-info entry, or a TTD address column) now chooses the pane by content type:
A right-click "Navigate in Current Pane" action forces navigation into the focused pane (added to Stack Trace, Debugger Info, and the TTD Calls/Memory/Events widgets; the other widgets already had an equivalent same-pane action).
The graph/linear token double-click and the TTD time-travel columns are intentionally left unchanged.
Fixes #1134
🤖 Generated with Claude Code