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

Breakpoint Menu and Debug Menu in the Decompiler Context Menu #2260

Merged

Conversation

NirmalManoj
Copy link
Member

@NirmalManoj NirmalManoj commented Jun 23, 2020

Your checklist for this pull request

Detailed description
This PR implements menus for Breakpoint and Debug. It is mostly taken from the DisassemblyContextMenu we have been using and there are some additional helper functions as discussed in #2256 .

Test plan (required)

  • Make sure code compiles without new warnings (please tell me if there are any).
  • Test if breakpoints and debug functionalities are working just like it used to before.

Closing issues

@NirmalManoj NirmalManoj requested a review from karliss Jun 23, 2020
src/menus/DecompilerContextMenu.cpp Outdated Show resolved Hide resolved
src/menus/DecompilerContextMenu.cpp Outdated Show resolved Hide resolved
src/menus/DecompilerContextMenu.cpp Outdated Show resolved Hide resolved
@karliss
Copy link
Member

karliss commented Jun 23, 2020

What's the strategy for adding the actions to decompiler widget? Old context menu added actions to parent widget. I don't like that as modifying parent from children should be avoided. If the menu is flat the menu user can easily do addActions(menu->getActions()) . I don't think it works with nested menus.

changed logic used for offset
tr() - Suggested
@NirmalManoj
Copy link
Member Author

NirmalManoj commented Jun 23, 2020

@karliss I'm not sure what you mean by
I don't like that as modifying parent from children should be avoided. If the menu is flat the menu user can easily do addActions(menu->getActions()). Can you please explain?

Currently, this is how actions are added to the decompiler widget.
https://github.com/radareorg/cutter/blob/3100872d99f0480538dcecf6d0ee5b305a3410cc/src/widgets/DecompilerWidget.cpp#L89

@karliss
Copy link
Member

karliss commented Jun 23, 2020

@karliss I'm not sure what you mean by
I don't like that as modifying parent from children should be avoided. If the menu is flat the menu user can easily do addActions(menu->getActions()). Can you please explain?

Currently, this is how actions are added to the decompiler widget.
https://github.com/radareorg/cutter/blob/3100872d99f0480538dcecf6d0ee5b305a3410cc/src/widgets/DecompilerWidget.cpp#L89

Does it work for actions that are in submenus?

@NirmalManoj
Copy link
Member Author

NirmalManoj commented Jun 23, 2020

Does it work for actions that are in submenus?

Are all menus other than mCtxMenu not submenus? I mean, in this PR, mCtxMenu is the main menu, breakpointMenu and debugMenu are sub-menus, right? Or does submenus mean menus inside one of these menus?

@karliss
Copy link
Member

karliss commented Jun 23, 2020

Are all menus other than mCtxMenu not submenus?

Yes I was referring to breakpoint menu and debugMenu, everything that isn't the top level decompiler context menu.

@NirmalManoj
Copy link
Member Author

NirmalManoj commented Jun 23, 2020

Are all menus other than mCtxMenu not submenus?

Yes I was referring to breakpoint menu and debugMenu, everything that isn't the top level decompiler context menu.

Then yes, it works for actions that are in sub-menus. Actually I haven't made any changes to this line of code.
https://github.com/radareorg/cutter/blob/3100872d99f0480538dcecf6d0ee5b305a3410cc/src/widgets/DecompilerWidget.cpp#L89

@NirmalManoj NirmalManoj requested a review from karliss Jun 24, 2020
src/widgets/DecompilerWidget.cpp Outdated Show resolved Hide resolved
This commit implements logic to handle multiple breakpoints for QAction actionToggleBreakpoint
@NirmalManoj NirmalManoj force-pushed the breakpoint-dec-context-menu branch from 6233ff5 to beff827 Compare Jun 25, 2020
src/widgets/DecompilerWidget.h Outdated Show resolved Hide resolved
src/widgets/DecompilerWidget.cpp Outdated Show resolved Hide resolved
src/widgets/DecompilerWidget.cpp Outdated Show resolved Hide resolved
src/widgets/DecompilerWidget.cpp Outdated Show resolved Hide resolved
src/menus/DecompilerContextMenu.cpp Outdated Show resolved Hide resolved
src/menus/DecompilerContextMenu.cpp Outdated Show resolved Hide resolved
src/menus/DecompilerContextMenu.cpp Outdated Show resolved Hide resolved
src/menus/DecompilerContextMenu.h Outdated Show resolved Hide resolved
src/menus/DecompilerContextMenu.h Outdated Show resolved Hide resolved
@karliss karliss self-requested a review Jun 28, 2020
Copy link
Member

@karliss karliss left a comment

Only few minor cleanups remaining for this PR.

@NirmalManoj NirmalManoj requested a review from karliss Jun 29, 2020
@karliss karliss merged commit dc4042b into rizinorg:decompiler-refactoring Jun 29, 2020
8 checks passed
@NirmalManoj NirmalManoj moved this from In progress to Done in Improving Decompiler Widget (GSoC) Jul 3, 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 15, 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 15, 2020
NirmalManoj added a commit to NirmalManoj/cutter that referenced this pull request Jul 26, 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