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

Save layout of the Decompiler Widgets #2410

Merged
merged 2 commits into from Aug 30, 2020

Conversation

NirmalManoj
Copy link
Member

@NirmalManoj NirmalManoj commented Aug 29, 2020

Your checklist for this pull request

Detailed description
The decompiler widget was made sync-able and the support for multiple decompiler widgets was implemented by PR #2402. However, the layout management was not implemented in that PR, and because of this layouts were not restored. This PR aims to solve this issue.

Even though this PR will restore the layout, in case multiple widgets are open with multiple decompilers, all widgets will be opened with the last opened decompiler. The logic for implementing the ideal situation where the decompiler will also be restored will require complex refactorings. I don't intend to do that by this PR. This is not a big deal for now considering the fact that we are not restoring seek.

Test plan (required)

  1. Compile PR and test if the layout gets restored as it should.
  2. I'm not sure if there is anything else that I should add, so please have a quick look at how other memory widgets store and restore layouts and check if anything relevant is missing in this PR.

Closing issues

@NirmalManoj NirmalManoj self-assigned this Aug 29, 2020
@NirmalManoj NirmalManoj requested review from karliss and ITAYC0HEN Aug 29, 2020
@@ -108,6 +108,11 @@ DecompilerWidget::DecompilerWidget(MainWindow *main) :

DecompilerWidget::~DecompilerWidget() = default;

QString DecompilerWidget::getWidgetType()
Copy link
Member

@karliss karliss Aug 29, 2020

Choose a reason for hiding this comment

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

It's probably better to use this when doing setObjectName so that they are in sync.

Copy link
Member

@karliss karliss Aug 29, 2020

Choose a reason for hiding this comment

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

Also object name shouldn't be translated it's an internal ID not a UI string.

Copy link
Member Author

@NirmalManoj NirmalManoj Aug 30, 2020

Choose a reason for hiding this comment

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

Thanks for pointing this out. Fixed it.

@karliss karliss added BUG Decompiler Issues and feature requests related to the decompiler in Cutter labels Aug 29, 2020
@karliss
Copy link
Member

karliss commented Aug 29, 2020

The logic for implementing the ideal situation where the decompiler will also be restored will require complex refactorings.

It shouldn't require a lot of refactoring. The mechanism for widget specific view setting saving is already there. All you have to do is implement serializeViewProprties and deserializeViewProperties.

But it is probably butter to do it separately as it is important to fix this bug as soon as possible.

@NirmalManoj
Copy link
Member Author

NirmalManoj commented Aug 30, 2020

It shouldn't require a lot of refactoring. The mechanism for widget specific view setting saving is already there. All you have to do is implement serializeViewProprties and deserializeViewProperties.

Thanks for telling me. I will look into this and will see how this can be done. But not in this PR.

@NirmalManoj NirmalManoj requested a review from karliss Aug 30, 2020
Copy link
Member

@karliss karliss left a comment

Looks good. I am investigating the travis failure. Unlikely to be related to your issue but needs to be fixed either way.

@karliss karliss merged commit d7ef6e9 into rizinorg:master Aug 30, 2020
8 of 9 checks passed
@ITAYC0HEN
Copy link
Member

ITAYC0HEN commented Aug 30, 2020

how can we avoid such issues next times we add a widget? I would personally forget about it

@karliss
Copy link
Member

karliss commented Aug 30, 2020

a) it only affects widgets with multiple copies, for others it's more difficult to get wrong
b) refactoring to reduce amount of code that needs to be updated
c) developer docs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BUG Decompiler Issues and feature requests related to the decompiler in Cutter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants