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

Remove or shrink unnecessary page rerender calls #4611

Merged
merged 3 commits into from Feb 10, 2023

Conversation

hrdl-github
Copy link
Contributor

@hrdl-github hrdl-github commented Jan 21, 2023

This requires #4158 that requires fewer explicit redraws when layers are deleted or their visibility is modified. Addresses #4580 in combination with #4158. This reduces the number of page repaints required. Full repaints are still used upon

  • finalisation of text edits
  • layer changes
  • initialisation

TODO:

  • More extensive testing
  • Remove debug messages

@hrdl-github
Copy link
Contributor Author

hrdl-github commented Jan 22, 2023

This requires #4158 that requires fewer explicit redraws when layers are deleted or their visibility is modified.

I think I was wrong about this one, as they behave the same now. This means that this PR is independent of #4158. This PR requires #4158 after all to rerender layers in the correct order.

I've restored repainting at a few points where it is needed.

src/core/gui/PageView.h Outdated Show resolved Hide resolved
@hrdl-github
Copy link
Contributor Author

Do you have any idea why a repaint would be performed here?

// Repaint page
control->getWindow()->getXournal()->layerChanged(selectedPage);
If it's not needed I would just remove it to save a repaint.

@hrdl-github
Copy link
Contributor Author

There are some additional buttons triggering ACTION_GOTO_{NEXT,PREVIOUS,TOP}_LAYER, which change the selected page AND some layers' visibility. I don't think a repaint is needed if the selection but not the visibility changes, hence 3228571.

@hrdl-github
Copy link
Contributor Author

After a bit more testing this weekend I don't see any more issues.

Copy link
Contributor

@bhennion bhennion left a comment

Choose a reason for hiding this comment

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

To be honest, I'm a bit conflicted about this PR.
One the one hand, it seems to work fine without the repainting you removed. On the other hand, what's the point of refreshing the page buffer if it's not to display it?

For instance, when an EditSelection is created (meaning some Elements just got selected), the Elements are:

  • removed from the page, added to a selection, painted to a selection buffer, and all of that is then painted on screen in the main loop
  • erased from the page buffer (via rerenderPage() which is an overkill) in a separated thread, followed by a full repaint.

With this PR, you removed the asynchronous full repaint, while I think the synchronous repaint is the superfluous one: we should wait for the page buffer to be updated before repainting.

That's just an example, and I think the other instances are similar. At this point, I can't imagine a situation where the page buffer needs updating without a follow up repaint being needed.

@hrdl-github
Copy link
Contributor Author

Thanks, this makes perfect sense. Instead of complicating the repainting logic I could just remove the call to rerenderPage() where it is not needed and do a more limited rerendering where necessary, e.g. in the EditSelection logic, which I indeed managed to break.

@hrdl-github hrdl-github changed the title Make page repaints optional Remove or shrink unnecessary page rerender calls Feb 1, 2023
@hrdl-github
Copy link
Contributor Author

I've removed the rerenderPage() calls that I don't think are needed and replaced the ones in the EditSelection constructor with range-based calls.

src/core/control/layer/LayerController.cpp Outdated Show resolved Hide resolved
src/core/control/tools/EditSelection.cpp Outdated Show resolved Hide resolved
src/core/control/tools/EditSelection.cpp Outdated Show resolved Hide resolved
src/core/control/tools/EditSelection.cpp Outdated Show resolved Hide resolved
src/core/gui/PageView.cpp Outdated Show resolved Hide resolved
src/core/gui/XournalView.cpp Outdated Show resolved Hide resolved
src/core/gui/XournalView.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@bhennion bhennion left a comment

Choose a reason for hiding this comment

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

LGTM!
Squashing and merging in 48 hours unless an objection is raised.

@bhennion bhennion added the merge proposed Merge was proposed by maintainer label Feb 7, 2023
@bhennion bhennion merged commit c914d24 into xournalpp:master Feb 10, 2023
@Technius Technius added this to the v1.2.0 milestone Feb 23, 2023
@hrdl-github hrdl-github deleted the page_repaint_standalone branch March 8, 2023 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge proposed Merge was proposed by maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants