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.

Sign up for GitHub

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

Wayland server side decorations #3425

Merged
merged 23 commits into from
Jul 2, 2024
Merged

Wayland server side decorations #3425

Changes from 2 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
e80daf5
Add xdg-decoration-unstable-v1 and modify CMakeLists.txt to generate …
tarek-y-ismail Jun 14, 2024
feca272
Add first prototype of xdg-decoration-unstable-v1.
tarek-y-ismail Jun 14, 2024
cc04d24
Apply formatting
tarek-y-ismail Jun 14, 2024
5b8027c
Add missing configure event call.
tarek-y-ismail Jun 14, 2024
68aad57
Apply formatting.
tarek-y-ismail Jun 14, 2024
59c8c08
Remove `mir::wayland` alias in header.
tarek-y-ismail Jun 14, 2024
826096a
Add missing copyright notice to xdg_decoration_unstable_v1.cpp
tarek-y-ismail Jun 14, 2024
7e75976
Strip logs from xdg_decoration_unstable_v1.cpp
tarek-y-ismail Jun 14, 2024
ba4db7c
Add basic error handling for xdg decoration
tarek-y-ismail Jun 24, 2024
9d89052
Apply formatting to xdg_decoration_unstable_v1.cpp
Jun 24, 2024
b8f0b2c
Convert the wayland error check to a sanity check in
Jun 24, 2024
0741868
Add listeners for decoration and toplevel destruction
tarek-y-ismail Jun 26, 2024
2a42cf4
Log instead of throwing `orphaned` when toplevels are destroyed.
Jun 28, 2024
f52391e
Add `destroy_toplevel_before_decoration` to expected failure list.
Jun 28, 2024
6632601
Wrap `toplevels_with_decorations` in a class.
Jul 1, 2024
f240d13
Fix comment typo in `mir::frontend::ToplevelsWithDecorations::unregis…
Jul 1, 2024
27d1207
Make comment in `acceptance-tests/wayland/CMakeLists.txt` clearer.
Jul 1, 2024
e0ca81e
Fix lifetime issues related to `toplevels_with_decorations`.
Jul 1, 2024
2012b20
Fix naming convention of `{register,unregister}Toplevel`.
Jul 1, 2024
6faabb4
Remove redundant `ToplevelsWithDecorations` construction code and dis…
tarek-y-ismail Jul 1, 2024
e69e676
Always unregister toplevel on destruction.
Jul 1, 2024
1af2a26
Improve semantics of `unregister_toplevel` and explain decoration
tarek-y-ismail Jul 2, 2024
faa1e16
Rename `destroy_toplevel_before_decoration` to `destroy_toplevel_befo…
tarek-y-ismail Jul 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions src/server/frontend_wayland/xdg_decoration_unstable_v1.cpp
View file
Open in desktop
AlanGriffiths marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,9 @@ namespace frontend
class ToplevelsWithDecorations
{
public:
ToplevelsWithDecorations() :
toplevels_with_decorations{std::unordered_set<wl_resource*>()}
{
}
ToplevelsWithDecorations() = default;
ToplevelsWithDecorations(ToplevelsWithDecorations const&) = delete;
ToplevelsWithDecorations& operator=(ToplevelsWithDecorations const&) = delete;

/// \return true if no duplicates existed before insertion, false otherwise.
bool register_toplevel(wl_resource* toplevel)
Expand All @@ -47,10 +46,13 @@ class ToplevelsWithDecorations
return inserted;
}

/// \return true if only one element was erased, false otherwise.
/// \return true if the toplevel didn't exist in the set, false otherwise.
/// The return value is used in the destroy callback of the toplevel to
/// check if the toplevel is destroyed before the decoration (orphaned
/// decoration).
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. This is a very weird semantic: returning false on success.
  2. It is normal to document the preconditions, action, and postconditions of a function. Not where its result happens to be used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I myself was a bit confused while using this method.

I've moved the explanation to where the mentioned case is actually handled. I might've detailed things too much, but I think this will save the next person (likely me :)) that works on this code some time.

bool unregister_toplevel(wl_resource* toplevel)
{
return toplevels_with_decorations.erase(toplevel) == 1;
return toplevels_with_decorations.erase(toplevel) == 0;
}

private:
Expand Down Expand Up @@ -143,7 +145,8 @@ void mir::frontend::XdgDecorationManagerV1::get_toplevel_decoration(wl_resource*
tl->add_destroy_listener(
[toplevels_with_decorations = this->toplevels_with_decorations, client = this->client, toplevel]()
{
if (!client->is_being_destroyed() && !toplevels_with_decorations->unregister_toplevel(toplevel))
const auto orphaned_decoration = !toplevels_with_decorations->unregister_toplevel(toplevel);
if (!client->is_being_destroyed() && orphaned_decoration)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const auto orphaned_decoration = !toplevels_with_decorations->unregister_toplevel(toplevel);
if (!client->is_being_destroyed() && orphaned_decoration)
if (!toplevels_with_decorations->unregister_toplevel(toplevel) && !client->is_being_destroyed())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't see what is achieved here other than saving a variable declaration (which the compiler likely optimizes anyway). I think the original might be a bit more readable.

{
mir::log_warning("Toplevel destroyed before attached decoration!");
// https://github.com/canonical/mir/issues/3452
Expand Down
Loading