canonical / mir

The Mir compositor
GNU General Public License v2.0
627 stars 100 forks source link

zwlr_foreign_toplevel_handle_v1.app_id() does not respect xdg_toplevel.set_app_id() #3626

Open AlanGriffiths opened 1 week ago

AlanGriffiths commented 1 week ago

Running the wlcs tests from within CLion I see:

WAYLAND_DEBUG=client cmake-build-debug/wlcs /home/alan/CLionProjects/mir/cmake-build-debug/lib/miral_wlcs_integration.so --gtest_filter=ForeignToplevelHandleTest.can_minimize_foreign  
...
[1105973.424]  -> xdg_toplevel@17.set_app_id("fake.wlcs.app.id")
...
[1105974.183] zwlr_foreign_toplevel_handle_v1@4278190081.app_id("clion_clion.desktop")

Running the same test from a console window:

WAYLAND_DEBUG=client cmake-build-debug/wlcs /home/alan/CLionProjects/mir/cmake-build-debug/lib/miral_wlcs_integration.so --gtest_filter=ForeignToplevelHandleTest.can_minimize_foreign  
...
[1267160.475]  -> xdg_toplevel@17.set_app_id("fake.wlcs.app.id")
...
[1267160.901] zwlr_foreign_toplevel_handle_v1@4278190081.app_id("fake.wlcs.app.id")

The latter is what I (and the wlcs code) expect.

I guess that the difference is that CLion is a classic snap (with an associated AppArmor profile). But if a client sets the app_id, what business do we have in overriding it? Especially with a .desktop file name?

AlanGriffiths commented 1 week ago

A workaround is to prefix the wlcs command with aa-exec -p unconfined vis:

aa-exec -p unconfined cmake-build-debug/wlcs /home/alan/CLionProjects/mir/cmake-build-debug/lib/miral_wlcs_integration.so --gtest_filter=ForeignToplevelHandleTest.can_minimize_foreign
AlanGriffiths commented 1 week ago

Especially with a .desktop file name?

That bit is likely #3608

mattkae commented 1 week ago

The real issue here is that we're abusing the zwlr_foreign_toplevel_handle_v1.app_id() field, and we've known this from the start. Instead of just returning the app ID as you gave it to us, we instead try to resolve it to a value that is more useful when resolving desktop files on the client's side (e.g. the app armor profile).

So, even if the user has set an app_id, we instead use that to resolve a desktop file. Maybe we should just be introducing a new field here, but that is a design decisions that we'd have to make.