Optimize disassembly scroll and selection logic - #3646
Conversation
|
Will test it one more time tomorrow on different machines Should probably update rizin to latest dev before this |
thestr4ng3r
left a comment
There was a problem hiding this comment.
Navigating by cursor keys does not work for me, but it did before. hjkl however works fine.
I also found a way to break it. Use this binary: hello-macos-arm64e-stripped.gz Seek to entry0 and scroll up. The first instruction of the function is shown twice with all its metadata:
https://github.com/user-attachments/assets/a1f9c041-68a5-491e-9f40-ca76b321c28e
Another point is the behavior when seeking to an unaligned address. When the offset is already in the visible range, the cursor just disappears. When it is outside of it, the screen shown directly after seeking is exactly what is to be expected, but after scrolling up the unaligned instruction never disappears:
https://github.com/user-attachments/assets/c622eaf1-735c-40af-8fea-a88f3ef5ecc6
However the behavior before your changed was arguably even worse, and it is hard to define what the expected behavior would be in every case, so this is not something that has to be solved right now.
As a sidenote, I think this could benefit a lot from a test suite. It was attempted to introduce one before but unfortunately never got finished: #1345
Apart from those issues I absolutely love the changes! Makes using the disassembly widget much more enjoyable.
| leftPanel->update(); | ||
|
|
||
| // update buffer by erasing extra lines | ||
| constexpr int multiplier = 5; |
There was a problem hiding this comment.
What was the reasoning for choosing exactly 5 here? Is it just a good tradeoff between memory usage and performance? Might be worth documenting this with the constant.
There was a problem hiding this comment.
What was the reasoning for choosing exactly 5 here? Is it just a good tradeoff between memory usage and performance? Might be worth documenting this with the constant.
No particular reason, yes it did seem like a good middle ground. From what I've seen the multiplier is usually between 3 - 10. Since the user can seek to a completely random offset (meaning the scrolling isn't always linear), keeping it on the lower side seems viable
|
I want this in Rizin 😭 |
Should be fixed now (tested on macos 14 - x86_64)
It seems like for some offsets rizin isn't calculating the offset before X instructions correctly, In this particular case, when rizin is queried for the offset that is exactly "max visible lines" (which was 21 in my case) before the current offset
I agree, if the seek is outside the visible range and caused by the console, we could set a flag and fetch disassembly again on scroll instead of prepending or appending, but that could cause other bugs. I think it's best to open a separate issue for it. I've left as it is for now
Couldn't agree more with this, I think cutter itself could benefit a lot from tests, there have been too many cases where changing one thing silently breaks something else There was also another issue, if metadata lines exceeded the number of visible lines, selecting it was causing weird behavior. That's also fixed |
thestr4ng3r
left a comment
There was a problem hiding this comment.
Tested again and can confirm the issues are fixed. From my side, this is good to merge.
Your checklist for this pull request
Detailed description
There will be a lot of text to read so get some popcorn 🍿
This PR does the following:
1- Scroll by visual lines instead of disassembly lines (see #3604)
Previously whenever a scroll happened, cutter cleared all of the disassembled lines it had and queried rizin again, even if the scroll was only for one instruction. This causes two problems:
To solve this we keep a buffer of "max visible lines * 5" which is filled in as user scrolls, meaning at first only "max visible lines" are queried from rizin, if user scrolls upwards the entire "max visible lines" above the current top instruction is fetched and prepended to our "lines" buffer. Since the previous lines are still saved - if the user scrolls back down we can just show those specific lines from our buffer instead of querying again.
Lines are erased from start or end based on scroll direction if the buffer exceeds the size cap of "max visible lines * 5"
The "lines" buffer is fully cleared if user seeks to some address using some external signal - like the Visual nav bar at the top (basically seeking using any method other than directly clicking on the instruction line itself)
Old:
old-scroll.webm
New:
new-scroll.webm
2- Adds infinite selection via mouse and keyboard
There was no way in cutter to select more lines than whats currently shown on screen and there was also no way to select text via keyboard. This is done via manually handling the selection instead of letting Qt handle it
selection.webm
Edit: One thing I forgot to mentions is that the selection is preserved no matter how far user scrolls up or down.
3- Fixes empty space at the bottom of disassembly widget viewport
Whenever "max visible lines" were calculated it didn't account for the fact that the last line might be partially visible meaning there is not enough space to display it fully, which created empty space at the bottom
Old: (see space at bottom - that space is preserved even when scrolling)

New:

4- Adds a visible/blinking cursor to the disassembly panel
Avoids the issue of not knowing which line we are on, if the line contains similar text that is highlighted. Also helps to know where selection will start using physical keys
Old:
old-cursor.webm
New:
new-cursor.webm
There are also some other optimizations which will be apparent from the code
From a simple test involving a lot of scrolling and seeking the outcome was:
Test plan (required)
Test every function of disassembly widget, including:
.....
Try to break it
Closing issues
closes #3604
and others (will update after one last test)