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

Rewrite layout management code #2172

Merged
merged 16 commits into from
May 22, 2020
Merged

Conversation

karliss
Copy link
Member

@karliss karliss commented May 1, 2020

Your checklist for this pull request

WARNING

This branch performs one way upgrade of settings and layout. If you have some complex layout that you want to keep using in the old Cutter backup your cutter settings.

Detailed description

Can be reviewed. Do not merge into 1.10.3 !

  • Separate all layout state into a structure making it clearer when and where it's saved, loaded, switched.
  • As part of layout state introduce dock view properties which is QVariant dictionary that can be used for easily saving single dock config. Makers of new widgets should consider if an option belongs to global settings or the new widget/layout specific properties. The new mechanism is suitable for situations where user might want to open two copies of a widget with different view settings. For example two hexwidgets one showing memory as hex bytes, other as floating point numbers. To use the new mechanism developer needs to override @CutterDockWidget::serializeViewProperties@ and @CutterDockWidget::deSerializeViewProperties@ methods.
  • Allow saving multiple layout profiles in addition to builtin Default and Debug. The logic is that "Default" and "[default]Debug" always contain the last layout in corresponding mode. When user loads new profile or modifies the current layout it becomes the new "Default". Saved layouts are never automatically modified.
  • Window size is separated from layout. It is loaded when opening Cutter. Switching layout doesn't touch the window size. It's possible to imagine that for some elaborate layouts window size is important but in most cases changing window size when switching layout would be unexpected. To be safe I kept the the window geometry in layout state but it isn't loaded. It will be easier to ignore or remove it later, than making it out of nothing in case it turns out to be necessary. Most examples show restoring state(dock placement) together with geometry(main window size and placement).
  • Remove the custom mechanism for synchronizing widget visibility with action checkboxes in windows menu. Qt has it builtin, no need to reinvent it poorly. The old mechanism tried to deal with multiple widgets of the same kind but it only increased complexity and wasn't actually used.
  • Code attempts to put new docks (new extra, plugins, or after Cutter upgrade) in the biggest area with existing disassembly, graph, hex widget. From my observations in https://github.com/radareorg/cutter/issues/694#issuecomment-621345529 and later it seems that any attempts to put something on the side may potentially result in broken layout.

When I started working on this I tried to visualize layout related code structure.
before:
before
after:
after

Test plan (required)

  • Clean start without upgrade

    • remove cutter config
    • start cutter
    • make sure default layout is reasonable
    • start debuging make sure layout is reasonable
  • layout upgrade a)

    • remove cutter config
    • open 1.10.3 cutter
    • make minor modifications to layout
    • close 1.10.3 cutter
    • open new cutter make sure sure layout is reasonable
    • start debugging make sure layout is reasonable
  • layout upgrade b)

    • remove cutter config
    • open 1.10.3 cutter
    • start debugging
    • make some modification to layout
    • stop debugging
    • close 1.10.3 cutter
    • open new cutter make sure sure layout is reasonable
    • start debugging make sure layout is reasonable
  • saving layout modifications

    • open cutter modify layout
    • restart cutter, make sure layout is preserved
    • start debugging
    • modify debug layout
    • stop debugging
    • start debugging
    • make sure debug layout is preserved
    • restart cutter
    • start debugging
    • make sure debug layout is preserved
    • stop debugging
  • resetting layout (do after previous step)

    • be in non debug mode
    • reset layout
    • make sure it matches initial layout
    • start debugging
    • make sure debug layout isn't reset
    • reset layout
    • make sure you see the initial debug layout
    • modify both debug and normal layout again
    • reset settings
    • make sure both layouts were reset
  • view property saving

    • Open multiple memory widgets(disassembly, hex or graph). At least two copies for one of them and place them side by side.
    • synchronize some of them, desynchronize others
    • restart cutter
    • make sure all the the memory widgets are opened and synchronization state is correctly restored
  • It would be useful if reviewer and maybe more people tried a bunch of random things to see if some sequence of operations or specific layout can cause layout to break.

Closing issues

Not closing #694 , that should be done after finishing second part and implementing layout management UI.
Closes #1921
Maybe closes #1737 . Doesn't do exactly what was asked there, but in my opinion solves the problem in a somewhat acceptable way.

Followup tasks

  • The new mechanism for saving view properties could be used to easily implement Cutter doesn't preserve Graph Layout after reset #1704
  • Better UI for managing layouts
  • view properties could be used for saving some of hex widget config
  • update Plugins to use the new API for dock action
  • Overview widget showing/hiding. Looks there are some leftover code trying to do more than basic checkbox to show, but it doesn't seem to be working in current master or this PR. Fix graph overview showing logic. #2209

@karliss karliss added this to the 1.11.0 milestone May 1, 2020
src/widgets/CutterDockWidget.h Outdated Show resolved Hide resolved
src/widgets/CutterDockWidget.h Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented May 1, 2020

This pull request introduces 1 alert when merging aeae3b1 into a3661fa - view on LGTM.com

new alerts:

  • 1 for Unused import

@xarkes
Copy link
Member

xarkes commented May 6, 2020

Just took a quick look, the refactoring looks very nice, I'll try to test it and make a better review later :)

@karliss karliss marked this pull request as ready for review May 9, 2020 09:05
Copy link
Member

@ITAYC0HEN ITAYC0HEN left a comment

Choose a reason for hiding this comment

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

That's a major improvement, well done!! It looks good and I enjoyed it

Notes before looking at the code

  • When clicking "Reset layout" it's not clear to what layout it is going to be reset. So the behavior is unexpected until you are familiar with Cutter. I suggest renaming "Reset layout" to "Reset to default layout".

  • Should "Reset layout" be so separated from "Save layout" and "Layouts >"? I am not sure what I prefer, the Reset to be together, or the layouts. I love them both. This can maybe be solved by organizing it like
    2020-05-11-14-42-58

  • Plugins: Until now, the widgets of newly added plugins were showed to the user on first-sight. It was usually opened in the right side of the screen. I personally didn't like it. Now, when new plugin is added, the widget of it is added to the main dock, but isn't in focus. I like it. So this is only a quick note about this change to make sure it is intentional.

  • Debug: When entering debug mode the layout was wrong

  • Debug: when stepping, weird things happen to my layout when stepping

Peek 2020-05-11 16-00

  • Debug: Why stack are on the upper view and not Registers? Registers should be top-right and stack should be bottom-right

  • Debug: stopping debug kept the layout:
    2020-05-11-17-20-13

  • Debug: saved debug layout is available when not in debug which shouldn't be since the debug widgets should not be available when not in debug

@karliss
Copy link
Member Author

karliss commented May 11, 2020

When clicking "Reset layout" ...

Renamed it.

Should "Reset layout" be so separated

I was thinking about this as well but couldn't come up with good solution.

Plugins: Until now, the widgets ... So this is only a quick note about this change to make sure it is intentional.

This is result of trying to place new widgets in somewhat reasonable place. The old behavior didn't scale when you had more than one new widget. Depending on what was the right most widget previous time it sometimes caused broken layout similar to what you saw during debuging with hexwidget slowly expanding.

Debug: When entering debug mode the layout was wrong

This might be result of bad layout upgrade. Let me know if you figure out how to reproduce it.

Debug: when stepping, weird things happen to my layout when stepping

Most likely result of broken layout from previous step.

Debug: Why stack are on the upper view and not Registers?

Weren't registers always at the bottom?

Debug: stopping debug kept the layout:

Can't repeat it.

Debug: saved debug layout is available when not in debug

I couldn't decide if we should artificially limit this. If you create some complicated layout wouldn't it be annoying to create it twice instead of loading a debug layout during normal mode and making minor modifications to it?

@karliss
Copy link
Member Author

karliss commented May 17, 2020

Haven't done the tests yet.

@karliss
Copy link
Member Author

karliss commented May 18, 2020

Did the tests.

@karliss karliss requested a review from ITAYC0HEN May 18, 2020 18:30
@ITAYC0HEN
Copy link
Member

General notes

  • would go for a smaller width function-widget
  • I prefer registers to be on top, and stack on the bottom

Had time to test 3 test-cases, all passed successfully

  • Clean start without upgrade
  • layout upgrade a)
  • layout upgrade b)

@xarkes
Copy link
Member

xarkes commented May 21, 2020

Hi, thanks again for this awesome PR.
I didn't have the time to test everything, however I noticed the graph overview is not enabled by default anymore, is it intended? I've noticed you wrote that a follow up task would be to improve its behavior but I don't know exactly what you are referring to.

@karliss
Copy link
Member Author

karliss commented May 21, 2020

@xarkes Overview has been not enabled by default for a while. This PR doesn't change that. To be more precise there is code which is supposed to handle third state for overview - automatic show/hide depending on there being something to show in overview. I assume this automatic showing was the default mode some time ago but it broke a while ago resulting in overview not shown by default.
I just confirmed it, deleting Cutter settings, opening Cutter 1.10.3 appimage and opening graph also results in overview not being shown. Most likely it was broken a few versions before that.

@ITAYC0HEN
Copy link
Member

Yes it is like this for months. We never came with the best solution for WHEN to show it and when not

@ITAYC0HEN
Copy link
Member

This is fantastic work! a big step forward. Much more stable!!

@karliss karliss merged commit 3545f05 into rizinorg:master May 22, 2020
@karliss karliss deleted the layout-management branch June 17, 2020 09:40
karliss referenced this pull request in karliss/cutter-docs Aug 15, 2020
* Use QDockWidget::toggleViewAction instead of custom solution.
* Improve new dock placement.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants