GNOME Bugzilla – Bug 651718
Make tests work on platforms supporting only a single stage
Last modified: 2011-11-14 11:58:07 UTC
Created attachment 189093 [details] [review] Tests patch With platforms like CEX100 on EGL backend, you can't create more than one stage because you don't really have a windowing system. So rather than creating a new one, just use the default one.
I'd rather people stopped using the default stage. platforms which support only one stage should still create it using clutter_stage_new(), and they should destroy it at the end of the application's lifetime, so that we can drop the ridiculous atexit() hack that we have in place for calling the EGL uninitialize function. if there are bugs in creating a stage on platforms supporting only one stage then we should fix those.
Ok, I will rework that so the backend only creates a stage when clutter_stage_new() is called.
Created attachment 190257 [details] [review] Rework coherence between clutter_stage_new and clutter_stage_get_default
Created attachment 190258 [details] [review] Don't even process events without a default stage
Created attachment 190259 [details] [review] Do not associate device with stage at device creation
Review of attachment 190257 [details] [review]: looks generally good. needs to have a couple of things fixed before it can be committed. ::: clutter/clutter-stage-manager.c @@ +296,3 @@ g_object_ref_sink (stage); + if (!stage_manager->stages) this should be: if (stage_manager->stages != NULL) to match the coding style. ::: clutter/clutter-stage.c @@ +2698,3 @@ + + stage_manager = clutter_stage_manager_get_default (); + default_stage = clutter_stage_manager_get_default_stage (stage_manager); there's the general issue that clutter_stage_new() should not do anything more than g_object_new(), since bindings will call the latter and not the former. we should move all of this stuff under the init/constructed implementations, but that can be done in another bug. ::: tests/interactive/test-actor-clone.c @@ +157,3 @@ } + stage = clutter_stage_new (); this is valid for all other changes in the tests: if you create a stage then you're responsible for connecting to the "destroy" signal and call clutter_main_quit(). otherwise some tests won't quit even when closed from the window frame.
Review of attachment 190258 [details] [review]: looks good.
Review of attachment 190259 [details] [review]: looks good.
(In reply to comment #6) ... > > ::: clutter/clutter-stage.c > @@ +2698,3 @@ > + > + stage_manager = clutter_stage_manager_get_default (); > + default_stage = clutter_stage_manager_get_default_stage (stage_manager); > > there's the general issue that clutter_stage_new() should not do anything more > than g_object_new(), since bindings will call the latter and not the former. > > we should move all of this stuff under the init/constructed implementations, > but that can be done in another bug. > > ::: tests/interactive/test-actor-clone.c > @@ +157,3 @@ > } > > + stage = clutter_stage_new (); > > this is valid for all other changes in the tests: if you create a stage then > you're responsible for connecting to the "destroy" signal and call > clutter_main_quit(). otherwise some tests won't quit even when closed from the > window frame. On delete event, the clutter stage calls clutter_main_quit() if the deleted stage is the default one. That's why I haven't changed the tests. I haven't reported that, but it seems a bit awkward to me. That means even if you open 3 windows with 3 different stages, if the window using the default stage is closed, then the main loop will quit.
yes, that's one of the reasons why nobody should be using the default stage. :-) the semantics are different, and the default stage semantics are utterly broken.
Ok, but it remains we need a default stage to bind input devices to it. So in this particular patch I'm changing the behavior of clutter_stage_new() to make it set the default stage. Is that Ok ?
the deprecate-default-stage fixes the semantics of the default stage for platforms that support multiple stages and for platforms that support only one stage. the bit for comment 6 still remains: closing the default stage will terminate the main loop; this is part of the invariants of the default stage and we can't change it. I thought that, on platforms with a single stage, there won't be a way to close the stage anyway, so calling clutter_actor_destroy() or calling clutter_main_quit() will yield the same results. attachment 190257 [details] [review] is not necessary any more; the other two patches should be committed to master. sadly, though, the evdev device manager fails pretty badly if we want to use it with windowing backends that support multiple stages, like wayland, because there's no way to associate a Stage instance with the Device Manager, and thus with the input devices/events. an API like: // stage gets event focus clutter_device_manager_push_stage (device_manager, stage); // stage loses event focus clutter_device_manager_pop_stage (device_manager); would probably be enough; push_stage() could be called when a stage is added to the Stage Manager, and whenever there's a message from the windowing system that a new stage window should get event focus. push_stage()/pop_stage() could even synthesize the ACTIVATE/DEACTIVATE signals on the stage itself.
Attachment 190258 [details] pushed to master Attachment 190259 [details] pushed to master I've experimented with the API proposal in comment 12 and found that it's not really enough — and it complicates the other backends. :-/ we'll need a new bug for the Evdev input backend, or we'll need a non-X11 input multiplexing library that allows associating "windows" to events. for the time being, we can continue to assume a single, default stage, even though it really sucks.