rizinorg / cutter

Free and Open Source Reverse Engineering Platform powered by rizin
https://cutter.re
GNU General Public License v3.0
15.73k stars 1.15k forks source link

[WIP] Add Multiline Selections/Operations #3323

Open yasmiins opened 6 months ago

yasmiins commented 6 months ago

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:

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 commented 6 months ago

@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 commented 6 months ago

@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 commented 4 months ago

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.