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

Fixed not switching Disassembly to not opened Graph view using <space> + a bit refactor #1370

Merged
merged 6 commits into from Mar 27, 2019

Conversation

Projects
None yet
5 participants
@AntonReborn
Copy link
Contributor

AntonReborn commented Mar 23, 2019

What had to be fixed:

Fixes #1268

What has been changed:

Moved logic behind the handling of the raisePrioritizedMemoryWidget signal to the common base class MemoryDockWidget. It triggers an action corresponding to the memory widget (Disassembly, Graph, Hexdump, Pseudocode), shows, raises and set focus to it.

Test plan:

  • Close Graph dock window;
  • Go to disassembly;
  • Press space a few times;
  • The Graph view should be opened and switching should happen.
@xarkes

xarkes approved these changes Mar 23, 2019

Copy link
Member

xarkes left a comment

Looks good to me, however there is one thing I noticed.
I don't know if it behaved like that before, but right now, when switching from disas to graph, every single time it redraws everything, and when the graph is a bit big, the interface might freeze for a second each time you press space.
I'm not sure why there is this "lag", it's probably the same one we had with overview (cc @Vane11ope) but I think it would be better to fix it before merging this.

@@ -1,25 +1,32 @@
#ifndef GRAPHWIDGET_H
#define GRAPHWIDGET_H

#include "CutterDockWidget.h"
#include "MemoryDockWidget.h"
#include "core/Cutter.h"

This comment has been minimized.

Copy link
@xarkes

xarkes Mar 23, 2019

Member

Why do you need to include Cutter.h here?

This comment has been minimized.

Copy link
@AntonReborn

AntonReborn Mar 23, 2019

Author Contributor

Don't need, thanks

@Vane11ope

This comment has been minimized.

Copy link
Contributor

Vane11ope commented Mar 23, 2019

overall looks pretty good to me too especially those refactoring such as making the properties private and using getter to access them instead of touching directly.

just one styling issue to keep our coding conventions and i'm good.

Show resolved Hide resolved src/widgets/MemoryDockWidget.cpp Outdated
@AntonReborn

This comment has been minimized.

Copy link
Contributor Author

AntonReborn commented Mar 24, 2019

@xarkes fixed a performance issue in the last commit. I do not know all the invariants in a graph view update flow. But this refresh looks redundant and seems like I didn't break anything.

@xarkes

This comment has been minimized.

Copy link
Member

xarkes commented Mar 24, 2019

Yep much better, there's just a tiny issue that remains ...
I don't know if it's important to anyone, but if you scroll the graph, and press space, the position in the graph is not saved.
If no one cares about it, we can merge.

@thestr4ng3r thestr4ng3r merged commit 5818998 into radareorg:master Mar 27, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@xarkes

This comment has been minimized.

Copy link
Member

xarkes commented Mar 27, 2019

Thanks! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.