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

OutputManager: When listeners are registered, they need to be notified of existing globals #3586

Merged
merged 1 commit into from
Sep 5, 2024

Conversation

AlanGriffiths
Copy link
Contributor

In particular, WindowWlSurfaceRole::output_global_created() uses the notification to register as an observer on the global. Without this, it cannot track configuration changes

@AlanGriffiths AlanGriffiths requested a review from a team as a code owner September 5, 2024 12:00
Copy link
Contributor

@mattkae mattkae left a comment

Choose a reason for hiding this comment

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

This change makes sense to me.

However, this does seem like a minor paradigm shift with our listeners. For example, XdgOutputV1 is an OutputConfigListener who listens on output_config_changed. When we add the listener (in XdgOutputV1's constructor), we don't automatically trigger the event for the listener, but rather allow the listener to manually trigger its own event.

It might be surprising that some listeners trigger events on listen, while others do not. But that's my 2c :)

@AlanGriffiths
Copy link
Contributor Author

However, this does seem like a minor paradigm shift with our listeners.

I'm sure there are both other registrations that do this and other registrations that don't. I guess it depends on what is being notified - in this case, I don't see a downside.

@mattkae mattkae added this pull request to the merge queue Sep 5, 2024
@mattkae mattkae removed this pull request from the merge queue due to a manual request Sep 5, 2024
@AlanGriffiths AlanGriffiths changed the title When listeners are registered, they need to be notified of existing globals OutputManager: When listeners are registered, they need to be notified of existing globals Sep 5, 2024
Copy link
Contributor

@hbatagelo hbatagelo left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@hbatagelo hbatagelo added this pull request to the merge queue Sep 5, 2024
Merged via the queue into main with commit 18d3daa Sep 5, 2024
36 of 37 checks passed
@hbatagelo hbatagelo deleted the Fix-OutputManager-registration branch September 5, 2024 21:07
github-merge-queue bot pushed a commit that referenced this pull request Sep 6, 2024
Surfaces need to track scale changes on outputs they appear on

Fixes: #3552

(Reviewers note: this depends on #3586)
AlanGriffiths pushed a commit that referenced this pull request Sep 9, 2024
…d of existing globals (#3586)

In particular, `WindowWlSurfaceRole::output_global_created()` uses the
notification to register as an observer on the global. Without this, it
cannot track configuration changes
AlanGriffiths pushed a commit that referenced this pull request Sep 9, 2024
Surfaces need to track scale changes on outputs they appear on

Fixes: #3552

(Reviewers note: this depends on #3586)
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.

3 participants