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

Fix toplevel window resize pingpong #3575

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

tarek-y-ismail
Copy link
Contributor

closes #3573.

@tarek-y-ismail tarek-y-ismail self-assigned this Aug 23, 2024
@tarek-y-ismail tarek-y-ismail requested a review from a team as a code owner August 23, 2024 15:53
Comment on lines 80 to 82
auto const should_resize =
(!impl->requested_size && impl->window_size != content_size) || // No requested resize, current size is different
(impl->requested_size && impl->requested_size != content_size && impl->window_size != content_size); // both requested and current sizes are different
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this approach is wrong. In essence, it is guessing whether the client has reacted to a configure event by looking at the current size. That will never work for all cases.

As we have sent a configure, we should be tracking the ack_configure (similar to the layer-shell logic you just fixed)

Copy link
Collaborator

Choose a reason for hiding this comment

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

(And, yes making the test more complex is tempting. I fell for it when I looked at #3040 a year ago)

@tarek-y-ismail tarek-y-ismail force-pushed the fix-toplevel-window-resize-pingpong branch from c219039 to 088fc0b Compare August 28, 2024 09:43
Since `requested_size` helps implement a sort of double buffering, it
should be cleared whenever we "commit" a new value.
@tarek-y-ismail tarek-y-ismail force-pushed the fix-toplevel-window-resize-pingpong branch from 088fc0b to 1307cf4 Compare September 2, 2024 13:36
@tarek-y-ismail
Copy link
Contributor Author

So I think the "loop" was intentional, but the guard clause doesn't work because requested_size isn't cleared after the client size is set, thus checking against the OLD value if requested_size on the next iteration (which continuously flip-flops) instead of checking against the window size. Instead, since requested_size is used as a sort of double buffering, it should be cleared at some point (when window_size is updated for example`)

image

Always Sunny Reaction GIF

Copy link
Collaborator

@AlanGriffiths AlanGriffiths left a comment

Choose a reason for hiding this comment

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

As previously discussed, I don't think this is right, but it does improve things and doesn't seem to cause additional problems. (In particular, canonical/ubuntu-frame-osk#58 remains fixed.)

(I have a suspicion that there may be more conflation of window_size and content_size around that needs resolving for SSD, but that's out of scope for this PR)

@AlanGriffiths AlanGriffiths added this pull request to the merge queue Sep 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 3, 2024
@AlanGriffiths AlanGriffiths added this pull request to the merge queue Sep 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 3, 2024
@tarek-y-ismail tarek-y-ismail added this pull request to the merge queue Sep 3, 2024
github-merge-queue bot pushed a commit that referenced this pull request Sep 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 3, 2024
@tarek-y-ismail tarek-y-ismail added this pull request to the merge queue Sep 3, 2024
Merged via the queue into main with commit db0f621 Sep 3, 2024
38 checks passed
@tarek-y-ismail tarek-y-ismail deleted the fix-toplevel-window-resize-pingpong branch September 3, 2024 14:23
AlanGriffiths pushed a commit that referenced this pull request Sep 9, 2024
AlanGriffiths added a commit that referenced this pull request Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Apps constantly resizing
2 participants