Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Add Multiline Selections/Operations #3323

Draft
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

yasmiins
Copy link

@yasmiins yasmiins commented Mar 25, 2024

Your checklist for this pull request

Detailed description

The goal is to add the ability to perform multiline selections in the the Disassembly view, so that certain operations in the Disassembly Context Menu (i.e. Set as code, Set as data, Copy lines as text, Add Breakpoints) can be performed on multiple lines/instructions at once.

The main objectives include:

  • Implement multiline selection for individual lines with Shift + Left Click
  • Implement rectangular selection for contiguous lines with Alt + Left Mouse Drag
  • Refactor DisassemblyContextMenu to add the ability to perform operations on multiple selections

Test plan (required)

Below is a GIF demonstrating the selection of individual lines using Shift + Left Click. The selection color is temporarily set to green for now for visibility during development.

demo

Challenges faced with selections/highlights:

  1. I have not found a way to retain multiline selections when scrolling. The qDebug logs show that the stored cursor positions seem to be invalidated when I scroll, causing all of the cursor offsets in the multiLineSelections list to default to a fallback value. I am thinking that a potential solution could involve storing line numbers instead of cursor positions, and for each line number, retrieve the instruction offset and calculate whether the instruction is visible within the frame upon each viewport update, highlighting the line if so.

  2. Achieving full-line color for individual non-contiguous selections is hindered by QTextEdit's default behavior.

Closing issues

closes #2601

@karliss
Copy link
Member

karliss commented Mar 25, 2024

@yasmiins I strongly suggest you not to waste time with first two points (non continuous selection and rectangular selection).

What matters most is actual interactions with selected range.

Fancy text selection modes by themselves has little use, even worse they will likely make the interactions more confusing if the actions correctly don't respond to them.

On the other hand context menus properly acting on selected range will be useful even if the disassembly window only supports regular continuous selection.

@yasmiins
Copy link
Author

yasmiins commented Mar 26, 2024

@yasmiins I strongly suggest you not to waste time with first two points (non continuous selection and rectangular selection).

What matters most is actual interactions with selected range.

Fancy text selection modes by themselves has little use, even worse they will likely make the interactions more confusing if the actions correctly don't respond to them.

On the other hand context menus properly acting on selected range will be useful even if the disassembly window only supports regular continuous selection.

I agree, can try and fix the scrolling issue afterwards.

To make multiline operations work, the DisassemblyContextMenu class will need significant refactoring. My proposal for this is to modify the current class variable RVA offset in DisassemblyContextMenu to be a list of offsets QList<RVA> offsets. In turn, I will have to adjust all methods in the class that currently use the offset variable to handle that list, regardless of whether they support multiline selections. I believe this approach will reduce code complexity and be more maintainable in the long-run, as opposed to creating a separate class variable specifically for methods that support multiline selections.

What do you think of this approach? Are there any objections or design considerations for the code/class structure that I should be aware of?

@karliss @XVilka

@karliss
Copy link
Member

karliss commented May 5, 2024

I am against the idea of list of offsets. For some of the operations it doesnt make sense to do on list of positions, and the ones that do will likely have to disassemble code again anyway. When you select range intent is to change it no matter how it was split, or what junk it contained before. This is also against the bigger picture of having unified range selection across different widgets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiline Operations / Multiline Selection
4 participants