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

Comment Menu for the Decompiler Context Menu #2265

Merged

Conversation

NirmalManoj
Copy link
Member

@NirmalManoj NirmalManoj commented Jun 25, 2020

Your checklist for this pull request

Detailed description
This PR implements the option for comments as it existed earlier in the disassembly context menu. As we had before, this PR contains implementation for adding/deleting comments using CC? commands in r2. Possibilities for having inline comments will have to be discussed and studied further before implementing that. That won't be handled by this PR.

Notes
Please don't care about formatting and have a look at changes introduced in just the last commit. I made this branch from the branch made for PR #2260 When it gets merged, I will rebase this and fix formatting issues.

Test plan (required)

  • Make sure it compiles with no extra warnings
  • See if it's working exactly as it did with the disassembly context menu.

@NirmalManoj NirmalManoj requested a review from karliss Jun 25, 2020
@NirmalManoj NirmalManoj changed the base branch from master to decompiler-refactoring Jun 25, 2020
src/menus/DecompilerContextMenu.cpp Outdated Show resolved Hide resolved
src/menus/DecompilerContextMenu.cpp Show resolved Hide resolved
@@ -15,6 +16,8 @@ DecompilerContextMenu::DecompilerContextMenu(QWidget *parent, MainWindow *mainWi
offset(0),
mainWindow(mainWindow),
actionCopy(tr("Copy"), this),
actionAddComment(tr("Add Comment"), this),
actionDeleteComment(tr("Delete comment"), this),
Copy link
Member

@karliss karliss Jun 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not related to this specific position)
Right clicking doesn't seem to update offset. To reproduce:

  • click on a line
  • right click on completely different line
  • select "Add comment"
  • observe that comment gets added to the line clicked first

Copy link
Member

@karliss karliss Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are you going to do about this?

Copy link
Member Author

@NirmalManoj NirmalManoj Jul 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will require a PR of its own and is outside the scope of this PR. This issue exists in other places as well (e.g. X-Refs dialog and the Initialization Script Editor) where we have QPlainTextEdit. I haven't figured out how to fix this yet.

How important is this issue? Do we really need to change the cursor position on right-click?

Copy link
Member

@karliss karliss Jul 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, solving this is important. Context menu is supposed to work for the object you right-click.

In X-Refs dialog and Initialization Script editor those are just plain text editors with default context menu. There are no objects to act on just list of characters, so there is no need to change cursor position when right clicking. That's also how it works in many other programs.

In decompiler widget on the other hand the text represents specific objects - variables, comments, functions. And context menu actions act on those objects. In software that has an understanding of objects represented by text like IDEs which have actions like rename symbol, go to declaration, find references, right click context menu performs the actions on object right clicked.

If you want to fix it separately - fine. But create an issue as I asked in #2260 already.

Copy link
Member Author

@NirmalManoj NirmalManoj Jul 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. As I understand now, this is not as complicated as I thought it was. I tried modifying the function DecompilerWidget::eventFilter(QObject *obj, QEvent *event) for responding to the right clicks and it seems to be working fine. However, I am not doing it with this PR. I will create an issue.

src/menus/DecompilerContextMenu.cpp Outdated Show resolved Hide resolved
@NirmalManoj NirmalManoj force-pushed the comment-dec-context-menu branch from 1ba293e to ab1e7d1 Compare Jun 29, 2020
@NirmalManoj NirmalManoj changed the title Comment Menu for the Decompiler Context Menu (DO NOT MERGE) Comment Menu for the Decompiler Context Menu Jun 29, 2020
@NirmalManoj NirmalManoj self-assigned this Jun 29, 2020
@NirmalManoj NirmalManoj requested a review from karliss Jun 29, 2020
@NirmalManoj
Copy link
Member Author

NirmalManoj commented Jun 30, 2020

The same described in #2270 is present while adding comments as well.

src/menus/DecompilerContextMenu.cpp Outdated Show resolved Hide resolved
src/menus/DecompilerContextMenu.cpp Outdated Show resolved Hide resolved
@karliss karliss merged commit 8a61553 into rizinorg:decompiler-refactoring Jul 6, 2020
8 checks passed
@NirmalManoj NirmalManoj moved this from In progress to Done in Improving Decompiler Widget (GSoC) Jul 6, 2020
NirmalManoj added a commit to NirmalManoj/cutter that referenced this pull request Jul 15, 2020
NirmalManoj added a commit to NirmalManoj/cutter that referenced this pull request Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants