GNOME Bugzilla – Bug 730179
Mutter WFits extension
Last modified: 2021-07-05 13:46:32 UTC
Hi, I created plugin that allows run Wayland Functional Integration tests [1] on Mutter. It's nice feature for testing. Regards, Marek
Created attachment 276585 [details] [review] Add plugin skeleton
Created attachment 276586 [details] [review] Add wayland-fits.xml
Created attachment 276587 [details] [review] Implementation of requests
Created attachment 276588 [details] [review] Stitching it all together
Created attachment 280874 [details] [review] Add plugin's skeleton rebased to master
Created attachment 280875 [details] [review] implementations rebased to master
Review of attachment 280874 [details] [review]: ::: configure.ac @@ +373,3 @@ +AC_ARG_ENABLE(wfits, + AS_HELP_STRING([--enable-wfits], + [Enable Wayland Functional Integration Tests extension]),, Enable Wayland Functional Integration Test *Suite* *plugin* ::: test/testplugin/Makefile.am @@ +5,3 @@ +pkglibdir=@MUTTER_PLUGIN_DIR@ + +INCLUDES=@MUTTER_CFLAGS@ -I $(top_srcdir)/src -DMUTTER_LIBEXECDIR=\"$(libexecdir)\" -DHOST_ALIAS=\"@HOST_ALIAS@\" -DMUTTER_LOCALEDIR=\"$(prefix)/@DATADIRNAME@/locale\" -DMUTTER_PKGDATADIR=\"$(pkgdatadir)\" -DMUTTER_DATADIR=\"$(datadir)\" -DG_LOG_DOMAIN=\"mutter\" -DSN_API_NOT_YET_FROZEN=1 -DMUTTER_MAJOR_VERSION=$(MUTTER_MAJOR_VERSION) -DMUTTER_MINOR_VERSION=$(MUTTER_MINOR_VERSION) -DMUTTER_MICRO_VERSION=$(MUTTER_MICRO_VERSION) -DMUTTER_PLUGIN_API_VERSION=$(MUTTER_PLUGIN_API_VERSION) -DMUTTER_PLUGIN_DIR=\"@MUTTER_PLUGIN_DIR@\" This would be more readable if it was split into multiple lines. Can't actually see it all in the review web view. @@ +13,3 @@ + -I$(top_srcdir)/src/backends \ + -I$(top_srcdir)/src/compositor \ + -I$(top_srcdir)/src/ui This seems more or less like the wfits plugin depends on non-exposed mutter API. Will that be a problem? ::: test/testplugin/mutter-wfits.c @@ +54,3 @@ +#define META_MUTTER_WFITS_PLUGIN_GET_PRIVATE(obj) \ +(G_TYPE_INSTANCE_GET_PRIVATE \ + ((obj), META_TYPE_MUTTER_WFITS_PLUGIN, MetaMutterWFitsPluginPrivate)) The coding style for these type declarations usually ignore the (occasionally ignored) 80 line length rule. While I prefer max 80 chars line length, I think these type declarations should be consistent with all the other ones. @@ +122,3 @@ +{ + MetaWaylandCompositor *compositor = meta_wayland_compositor_get_default (); + GRand *rand = g_rand_new_with_seed (g_get_monotonic_time ()); I think its better to make the color deterministic, especially if we ever want to do any sort of screenshot based testing, it'd be good if the color is the same every time.
Review of attachment 280875 [details] [review]: ::: test/testplugin/mutter-wfits-implementation.c @@ +34,3 @@ +get_time (void) +{ + return meta_display_get_current_time_roundtrip (meta_get_display ()); Any reason why we can't just use CLOCK_MONOTONIC here? When using the native clutter backend this is more or less what we get. @@ +56,3 @@ +}; + + Extra empty line. @@ +69,3 @@ + struct wl_resource *result = wl_resource_create (client, + &wfits_query_result_interface, + 1, id); AFAIK variable declarations should be in the beginning of the block. @@ +133,3 @@ + clutter_stage_get_actor_at_pos (CLUTTER_STAGE (compositor->stage), + CLUTTER_PICK_REACTIVE, + (double) x, (double) y); Indentation is wrong. Also, there are several things that won't happen (like enter/leave events, updating input device actor, ..), so there might be things here and there that won't work since events are not created properly (from within clutter). Maybe using clutter_event_put () instead of clutter_do_event () will help with that? @@ +212,3 @@ + clutter_event_set_time (ev, get_time ()); + + clutter_do_event (ev); Same here about maybe using clutter_event_put () here might be better (?).
Review of attachment 276588 [details] [review]: Not sure why this was separate from "wfits: add implementation of interfaces" and "wfits: add wayland-fits.xml protocol" since separately, they don't do anything. ::: test/testplugin/mutter-wfits.c @@ +170,3 @@ set_background (); + + g_assert ( Since we rely on side effects of assertions, I think we should cause a compile error if g_assert is defined to be a no-op. @@ +188,3 @@ +extern const struct wfits_manip_interface manip_interface; +extern const struct wfits_query_interface query_interface; +extern const struct wfits_input_interface input_interface; Not sure I see the point of having the implementations in another file, but if you want to do that, I think at least do it properly with a .h file with declarations.
I applied the patches (needed some simple conflict resolving), ran wfits, and it seemed to mostly work. I got 82%: Checks: 41, Failures: 0, Errors: 7 where the errors were timeouts. Anyway, I think adding it even though something is broken is a good idea.
Some high-level points from a glance at the code and Jonas's comments: In general, we consider plugins to be a dead-end - not something that we want to develop. And because only *one* plugin can now be loaded at a time, having the test functionality as a plugin doesn't allow, say, testing the gnome-shell executable. For this reason, I think the appropriate setup here would be to have a separate executable linking to libmutter, in the same way as mutter/tests/mutter-test-runner. Is there any reason to make this optional at build time? If something doesn't add extra dependencies I don't see a reason to have it optionally built - that just leads to problems like people committing patches that break the code. I also find the division into separate commits here quite confusing. Some of the commits don't seem to do anything (or even compile?) on their own. Maybe everything should just be squashed into one big commit? (If left as standalone, every commit message should have a body that describes what it does with some detail, not just a subject)
(In reply to Jonas Ådahl from comment #8) > Review of attachment 280875 [details] [review] [review]: > > ::: test/testplugin/mutter-wfits-implementation.c > @@ +34,3 @@ > +get_time (void) > +{ > + return meta_display_get_current_time_roundtrip (meta_get_display ()); > > Any reason why we can't just use CLOCK_MONOTONIC here? When using the native > clutter backend this is more or less what we get. The reason is that everywhere in mutter is used this function. Another is that I don't have to exctract the value from struct timespec, I get uint64 right from this function.
Created attachment 301700 [details] [review] Add test mode to Mutter Few things changed since I wrote the original patches. Mainly Mutter now checks if the event is 'sane'. Since we synthesize the events, we need to somehow bypass some of the checks, otherwise the event don't get dispatched.
Created attachment 301702 [details] [review] add wfits-runner stand-alone executable that creates environment for wayland-fits
(In reply to Marek Chalupa from comment #12) > (In reply to Jonas Ådahl from comment #8) > > Review of attachment 280875 [details] [review] [review] [review]: > > > > ::: test/testplugin/mutter-wfits-implementation.c > > @@ +34,3 @@ > > +get_time (void) > > +{ > > + return meta_display_get_current_time_roundtrip (meta_get_display ()); > > > > Any reason why we can't just use CLOCK_MONOTONIC here? When using the native > > clutter backend this is more or less what we get. > > The reason is that everywhere in mutter is used this function. Another is > that I don't have to exctract the value from struct timespec, I get uint64 > right from this function. Not input events when running on the native backend; they get their time from CLOCK_MONOTONIC. When running nested they seem to come from X11 though, whatever the clock XI2 uses. Just seemed unnecessary to roundtrip just to get the time.
Review of attachment 301700 [details] [review]: ::: src/core/util-private.h @@ +35,3 @@ void meta_set_is_wayland_compositor (gboolean setting); +void meta_set_test_mode (gboolean); +gboolean meta_get_test_mode (void); Naming makes it sound like we are getting a test mode. It should rather be "is_testing" or something like that. ::: src/wayland/meta-wayland-seat.c @@ +261,3 @@ + * pass this check */ + if (meta_get_test_mode ()) + return TRUE; Does this mean that tests depending on known input event sequence would break if the person running the test move the mouse (or table, moving the mouse) or touches a key, while the test is running? Sounds rather like we need a "if in_test_mode && is_test_device (input_device) then ignore event completely". We also probably need to exit when testing completes, so the user won't be stuck in a idling test session. That or re-enable user driven input events again. Not sure it makes any sense to continue using a test session though.
Review of attachment 301702 [details] [review]: ::: src/Makefile-tests.am @@ +4,3 @@ +wfits_dir = tests/wfits +wfits_protocol_files = \ nit: naming of these variables are inconsistent with the equivalent ones in Makefile.am. @@ +6,3 @@ +wfits_protocol_files = \ + $(wfits_dir)/protocol/wayland-fits-protocol.c \ + $(wfits_dir)/protocol/wayland-fits-server-protocol.h These files should be added to CLEANFILES. @@ +19,3 @@ +$(wfits_dir)/protocol/wayland-fits-protocol.c : $(wfits_protocol_xml) + $(AM_V_GEN)$(WAYLAND_SCANNER) code < $< > $@ + echo $(WAYLAND_SCANNER) Drop this? @@ +23,3 @@ + $(AM_V_GEN)$(WAYLAND_SCANNER) server-header < $< > $@ + +BUILT_SOURCES+=$(wfits_protocol) spaces around +=, and I suspect it should be wfits_protocol_xml. @@ +24,3 @@ + +BUILT_SOURCES+=$(wfits_protocol) + Building simply didn't work for me. make created a directory called $(wfits_dir) in src/ and put some things there. Doing a s/\$(wfits_dir)/tests\/wfits/g and removing the wfits_dir definition fixes it though. ::: src/tests/wfits/wfits-runner.c @@ +22,3 @@ + * ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF + * THIS SOFTWARE. + */ Should include config.h here, right? @@ +50,3 @@ +get_time (void) +{ + return meta_display_get_current_time_roundtrip (meta_get_display ()); Again, I don't like adding this dependency on a running Xwayland server, as the goal is to not require it. As long as we only let test device events through when running a test-session, I suspect it wont be a problem to use the monotonic clock here, since that is what the native backend uses. @@ +102,3 @@ + g_assert (result); + + meta_window_get_session_geometry (surf->window, &x, &y, &w, &h); This doesn't look like the correct way to get the geometry, this has nothing to do with saving sessions. Not sure what "surface_geometry" means as it seems undocumented, but I suspect its position and size of the surface (not "window") on the screen? In that case I think what you should use is meta_window_get_buffer_rect. @@ +164,3 @@ + clutter_stage_get_actor_at_pos (CLUTTER_STAGE (stage), + CLUTTER_PICK_REACTIVE, + (double) x, (double) y); This explicit cast is redundant (casting from int to double). @@ +195,3 @@ + /* set right button relative to evdev */ + /* following 'magic numbers' are from meta-wayland-pointer.c + * from default_grab_button () */ The 1, 2 and 3 stand for CLUTTER_BUTTON_PRIMARY, CLUTTER_BUTTON_MIDDLE and CLUTTER_BUTTON_SECONDARY. @@ +334,3 @@ + GError *error = NULL; + + char *fake_args[] = { NULL, "--wayland" }; How would one run this with --display-server? Also making this a stand-alone executable still makes it impossible to run tests when running gnome-shell. Couldn't we just make it a command line argument to regular mutter, meaning it'll be available to all libmutter users?
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/mutter/-/issues/ Thank you for your understanding and your help.