-
Notifications
You must be signed in to change notification settings - Fork 197
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
In case of initialzoom
set, set up drawpath
first and then do the…
#599
In case of initialzoom
set, set up drawpath
first and then do the…
#599
Conversation
src/document.h
Outdated
@@ -525,6 +525,7 @@ struct Document { | |||
SetSelect(drawroot->parent->grid->FindCell(drawroot)); | |||
} | |||
while (len < drawpath.size()) drawpath.remove(0); | |||
if (onlysetupdrawpath) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In how many places is Zoom
called? if only few, it may be cleaner than rather using a boolean argument, to just split this function in 2 (e.g. ZoomSetDrawPath
and ZoomDraw
) and call those as needed.
Just an idea.. boolean args to a function that make it take different paths thru the function is usually not so great, often better to split or specialize the function for the different callers instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your feedback and review!
Zoom
is called from five places in the code. In every instance (excluding the initial zooming when the document is loaded), it seems that we have a complete layout calculation and the draw of the selection being necessary. I think that it makes sense to split out the setting of the drawpath
into a separate function so that we can make use of the initial zooming but not to move the other part of the Zoom
function into a second separate split function. Would you agree?
Thanks for your feedback in advance!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the updated commit in the PR :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool all these performance optimizations.. looks like you're starting to understand all the code pretty deeply now :)
… layout and render This commit saves the extra layouting and rendering before the zoom.
e1f8368
to
b1ae4de
Compare
It takes some time, step after step I learn a little bit more. And I try to learn from my mistakes. ;-) |
src/document.h
Outdated
@@ -525,6 +525,10 @@ struct Document { | |||
SetSelect(drawroot->parent->grid->FindCell(drawroot)); | |||
} | |||
while (len < drawpath.size()) drawpath.remove(0); | |||
} | |||
|
|||
void Zoom(int dir, wxDC &dc, bool fromroot = false, bool selectionmaybedrawroot = true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
selectionmaybedrawroot
not needed anymore?
… layout and render
This commit saves the extra layouting and rendering before the zoom.