rizinorg / cutter

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

Decompiler view jumps when toggling a breakpoint or adding a comment #2270

Closed NirmalManoj closed 4 years ago

NirmalManoj commented 4 years ago

Environment information

Describe the bug Toggling a breakpoint(or adding a comment) in the decompiler widget causes the view to jump. This makes it difficult to keep track on the position of code you are looking at.

To Reproduce

  1. Open a ~2 screens high function and scroll down a bit
  2. Select a line somewhere in the middle and add a breakpoint(or comment).
  3. Observe that viewing position in the decompiler widget changes

See this GIF to see exactly what's happening display jumps

Expected behavior

The view should not move while adding toggling a breakpoint. But the highlighting of the line should change accordingly.

Findings so far (for the problem with toggling breakpoints) After hours of debugging, I found that this view jumping problem happens because the whole code gets decompiled again when a new breakpoint is added. Then the code tries to reset the cursor to the previous location(the line at which the breakpoint was toggled). That's how we at least get to see the line at which the breakpoint was added. But the view doesn't get back to the way it was.

NirmalManoj commented 4 years ago

It turns out to be existing for multiple options we have in the context menu of the decompiler. See the GIF below View Jumping Common

karliss commented 4 years ago

For some of them like renaming things re-running decompiler is unavoidable, but there is room for improvements to try better preserve view position.

In case of breakpoints I don't think decompiler needs to be run.

NirmalManoj commented 4 years ago

@karliss Yes, for breakpoints I think we can somehow stop the decompiler from running. In fact, that was the hack that I was thinking about. But ultimately, in order to fix this problem entirely, we have to find a way to reset the view position to exactly where it was before. In case this is not possible in the QPlainTextEdit, and since we are planning to use some other advanced text editor like KTextEditor in the future, I think it might be possible in it.

Can you explain a bit on how you think we can try to better preserve view position?

karliss commented 4 years ago

Not performing an unnecessary decompilation isn't necessary a hack. It is somewhat expensive operation and performing it when not needed makes the UI less responsive. Even with position restoration working perfectly it would be preferable to do it only when necessary.

As for scroll position preserving there two approaches I can think of two approaches that can be explored. The first one is to do the obvious thing. To preserve the scroll position - preserve the scroll position. Save it before changing text and load it afterwards.

Other approach is to try avoiding position reset. When doing basic text modifications cursor position is more or less preserved. Maybe the code can be replaced using text editing API in a way to reduces cursor move.

karliss commented 4 years ago

@NirmalManoj Wasn't this fixed already? I guess gitthub automatically closes the issue only when PR gets merged to master. This can probably be closed.

NirmalManoj commented 4 years ago

I'm closing it.