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

feature: adding an is_above method to the SurfaceStack so that compositor authors can write tests to assert the relative z-order of surfaces #3707

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mattkae
Copy link
Contributor

@mattkae mattkae commented Dec 19, 2024

What's new?

  • Bumped the project minor version to 20 (new symbols)
  • Added AbstractShell::is_above, FocusController::is_above, and SurfaceStack::is_above
  • Added methods to the WindowManagementTestHarness to focus a window and query is_above
  • Added two MinimalWindowManagerTests that assert points about is_above
  • Updated the symbols.map file

@mattkae mattkae requested a review from a team as a code owner December 19, 2024 20:30
Copy link
Contributor

@robert-ancell robert-ancell left a comment

Choose a reason for hiding this comment

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

Code changes look fine, will need someone else who better knows Mir architecture to clarify if there should be this z_order feature.

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.

I'm not convinced that we have to bake in a z-order to our APIs. If we ever start positioning surfaces in 3D, then the z-order will depend on viewpoint(s).

Having said that, we already have:

    void raise(SurfaceSet const& surfaces);
    void swap_z_order(SurfaceSet const& first, SurfaceSet const& second);
    void send_to_back(SurfaceSet const& surfaces);

So, maybe that thought is moot.

Now, the semantics of z_order() are not intuitive.

  1. '0' == "at front" OR "expired"OR "not found"
  2. layers are not respected (comparing windows from different layers doesn't work)

I think we would do better with an interface for making assertions:

EXPECT_TRUE(shell->assert_that(window1, is_above, window2));

src/server/symbols.map Outdated Show resolved Hide resolved
src/server/scene/surface_stack.cpp Outdated Show resolved Hide resolved
@mattkae
Copy link
Contributor Author

mattkae commented Jan 6, 2025

I think we would do better with an interface for making assertions:

EXPECT_TRUE(shell->assert_that(window1, is_above, window2));

@AlanGriffiths Do you mean this API literally, or would the shell have a method like Shell::is_above(Window const&, Window const&)?

@AlanGriffiths
Copy link
Collaborator

@AlanGriffiths Do you mean this API literally, or would the shell have a method like Shell::is_above(Window const&, Window const&)?

The API I suggested is more fluent (reads more like English) and allows some additional predicates to be added without breaking ABI (e.g. shares_layer_with). But I didn't really intend it literally. HTH

@mattkae mattkae changed the title feature: adding a z_order method to the SurfaceStack so that compositor authors can write tests to assert the relative z-order of surfaces feature: adding an is_above method to the SurfaceStack so that compositor authors can write tests to assert the relative z-order of surfaces Jan 7, 2025
@mattkae mattkae requested a review from AlanGriffiths January 7, 2025 13:58
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.

Just some nitpicking

src/server/scene/surface_stack.cpp Outdated Show resolved Hide resolved
src/server/scene/surface_stack.cpp Outdated Show resolved Hide resolved
…ors can write tests to assert the relative z-order of surfaces
@mattkae mattkae force-pushed the feature/focus_order_tests branch from cb662b9 to ab38a60 Compare January 17, 2025 14:35
@mattkae mattkae added this pull request to the merge queue Jan 17, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 17, 2025
@mattkae mattkae added this pull request to the merge queue Jan 17, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 17, 2025
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