GNOME Bugzilla – Bug 781906
Move g-s-d xrandr functionality to mutter
Last modified: 2017-07-19 09:21:08 UTC
The new mutter monitor config API is a good excuse to finally move all creating/changing monitor configurations from g-s-d since these operations logically belong in the compositor which has both more information available and can do it more efficiently and with better error handling/recovery. I'm starting with the orientation plugin and will do the fn+f7 key handling next. The heuristic I used here to decide to apply the orientation is pretty simple but I think it's close to optimal. But, suggestions to improve it are welcome of course.
Created attachment 350663 [details] [review] backeds: Add a MetaOrientationManager class This basically moves g-s-d's orientation plugin into mutter so that eventually g-s-d doesn't need to build monitor configurations by itself anymore.
Created attachment 350664 [details] [review] monitor-manager: Hook MetaOrientationManager to change the config This just hooks up the orientation changing event. The config change will be done transparently in the configuration backend.
Created attachment 350665 [details] [review] monitor-config-manager: Automatically apply orientation if possible If we have an orientation available and only a single connected output we try to apply the corresponding monitor transform automatically.
Created attachment 350666 [details] [review] meta-monitor-config: Automatically apply orientation if possible If we have an orientation available and only a single connected output we try to apply the corresponding monitor transform automatically.
Review of attachment 350665 [details] [review]: ::: src/backends/meta-monitor-config-manager.c @@ +361,3 @@ + GList *monitors = meta_monitor_manager_get_monitors (config_manager->monitor_manager); + + if (g_list_length (monitors) != 1 || I can't say I understand all of this code, but here you also need to make sure that the only monitor is the builtin one. You don't want to rotate your TV's output based on the orientation of your convertible laptop.
Review of attachment 350666 [details] [review]: ::: src/backends/meta-monitor-config.c @@ +1929,3 @@ return FALSE; + if (n_outputs == 1) Ditto.
Review of attachment 350663 [details] [review]: ::: src/backends/meta-orientation-manager.c @@ +197,3 @@ + G_CALLBACK (iio_properties_changed), self, 0); + g_dbus_proxy_call (self->iio_proxy, + "ClaimAccelerometer", Ideally, you'd release the accelerometer when the screen that can be rotated is turned off: " Applications should call net.hadess.SensorProxy.ReleaseAccelerometer() when readings are not required anymore. For example, an application that monitors the orientation of the main screen should stop monitoring for updates when that screen has been turned off. This prevents the sensor proxy from polling the device, thus increasing wake-ups and reducing battery life. " The g-s-d code wasn't doing that, it just didn't have a good view into the screen configuration...
Review of attachment 350663 [details] [review]: s/backeds/backends/ in the commit subjet as well.
Review of attachment 350663 [details] [review]: ::: src/backends/meta-orientation-manager.c @@ +58,3 @@ +#define ORIENTATION_LOCK_KEY "orientation-lock" + +static MetaOrientationManager *the_manager = NULL; Should we put this in MetaBackend instead maybe? The other backend things are there, and we can manage the creation better.
General question: When is orientation supposed to be used and when a stored configuration supposed to be used? If the user ever changed the configuration, it'll always be there in monitors.xml, thus orientation would never be used again, as the stored one is preferred. Is this supposed to be avoidable some how?
You're not supposed to be able to control the orientation when there's an accelerometer controlling that orientation.
But you might still have have an external monitor (as per your earlier review comment), or change the resolution, or scale, or underscanning state etc, and in those cases we'll have a stored config, that'll take preference here.
(In reply to Bastien Nocera from comment #11) > You're not supposed to be able to control the orientation when there's an > accelerometer controlling that orientation. Also, really? That is one thing that drives me crazy on a phone/tablet, but luckily it can be turned off.
(In reply to Jonas Ådahl from comment #12) > But you might still have have an external monitor (as per your earlier > review comment), or change the resolution, or scale, or underscanning state > etc, and in those cases we'll have a stored config, that'll take preference > here. Right, good point. So it'd need to apply everything but the rotation... (In reply to Jonas Ådahl from comment #13) > (In reply to Bastien Nocera from comment #11) > > You're not supposed to be able to control the orientation when there's an > > accelerometer controlling that orientation. > > Also, really? That is one thing that drives me crazy on a phone/tablet, but > luckily it can be turned off. GNOME behaves the same way as phones/tablets wrt screen orientation.
(In reply to Bastien Nocera from comment #5) > I can't say I understand all of this code, but here you also need to make > sure that the only monitor is the builtin one. > > You don't want to rotate your TV's output based on the orientation of your > convertible laptop. You're right, added that check. (In reply to Bastien Nocera from comment #8) > s/backeds/backends/ in the commit subjet as well. Fixed, thanks for spotting. (In reply to Jonas Ådahl from comment #9) > +static MetaOrientationManager *the_manager = NULL; > > Should we put this in MetaBackend instead maybe? The other backend things > are there, and we can manage the creation better. I wanted it to be easily accessible from both meta-monitor-config-manager and meta-monitor-config without jumping through many hoops. And since there aren't as many interactions with other components this seemed like the simplest way to achieve something that works. Just to be sure I understand your proposal, we'd stash the orientation instance in the MetaBackendPrivate instance and add a meta_backend_get_orientation_manager() to be called from anywhere that needs it? (In reply to Jonas Ådahl from comment #12) > But you might still have have an external monitor (as per your earlier > review comment), or change the resolution, or scale, or underscanning state > etc, and in those cases we'll have a stored config, that'll take preference > here. I haven't tested this (I don't have the hardware) but as the code is (and it's definitely my goal) the orientation provided transform is always applied even on top of saved configurations. If users don't want it they can easily lock orientation updates at any time. The gsetting is poked via both a button in gnome-shell's top-right menu and a physical key binding.
Created attachment 350973 [details] [review] backends: Add a MetaOrientationManager class This basically moves g-s-d's orientation plugin into mutter so that eventually g-s-d doesn't need to build monitor configurations by itself anymore.
Created attachment 350974 [details] [review] monitor-manager: Hook MetaOrientationManager to change the config This just hooks up the orientation changing event. The config change will be done transparently in the configuration backend.
Created attachment 350975 [details] [review] monitor-config-manager: Automatically apply orientation if possible If we have an orientation available and only a single connected output we try to apply the corresponding monitor transform automatically.
Created attachment 350976 [details] [review] meta-monitor-config: Automatically apply orientation if possible If we have an orientation available and only a single connected output we try to apply the corresponding monitor transform automatically.
(In reply to Rui Matos from comment #15) > (In reply to Jonas Ådahl from comment #9) > > +static MetaOrientationManager *the_manager = NULL; > > > > Should we put this in MetaBackend instead maybe? The other backend things > > are there, and we can manage the creation better. > > I wanted it to be easily accessible from both meta-monitor-config-manager > and meta-monitor-config without jumping through many hoops. And since there > aren't as many interactions with other components this seemed like the > simplest way to achieve something that works. > > Just to be sure I understand your proposal, we'd stash the orientation > instance in the MetaBackendPrivate instance and add a > meta_backend_get_orientation_manager() to be called from anywhere that needs > it? Right. The reasons are mostly to get rid of all globals that doesn't need to be globals, because its not good practice when writing a library. Eventually my plan is to have clear ownership of every part, with explicit clean up paths, and then at one point add valgrind memory leak tests. I have so far (slowly) been moving things away from their own globals to being explicitly owned by MetaBackend whenever I have touched such places. > > > (In reply to Jonas Ådahl from comment #12) > > But you might still have have an external monitor (as per your earlier > > review comment), or change the resolution, or scale, or underscanning state > > etc, and in those cases we'll have a stored config, that'll take preference > > here. > > I haven't tested this (I don't have the hardware) but as the code is (and > it's definitely my goal) the orientation provided transform is always > applied even on top of saved configurations. Ah, I can see that now. Though, it looks like this will cause issues when there is a monitor to the right of the rotated one. The width of the logical monitor will have changed, thus creating a gap/overlap between/over the logical monitor to the right. > > If users don't want it they can easily lock orientation updates at any time. > The gsetting is poked via both a button in gnome-shell's top-right menu and > a physical key binding. Side question: does disabling it cause the sensor to not be listened too (in the sensor proxy daemon); or would we need to "unclaim" it to make that possible? IIRC there are quite a lot of updates i.e. wakeups from a sensor.
(In reply to Jonas Ådahl from comment #20) > Ah, I can see that now. Though, it looks like this will cause issues when > there is a monitor to the right of the rotated one. The width of the logical > monitor will have changed, thus creating a gap/overlap between/over the > logical monitor to the right. Scrap that, that will of course not happen since no auto-rotation is done when there are more than one monitors.
Renaming since I've now implemented the video-rotate keybinding and ended up refactoring the configuration changes because it's basically the same as for supporting the orientation sensor.
Created attachment 351210 [details] [review] backends: Add a MetaOrientationManager class -- No longer a singleton, instead we create the instance in MetaBackend as suggested.
Created attachment 351211 [details] [review] monitor-config-manager: Add API to rotate the current config This will allow us to do automatic rotation of the builtin display if that's the only active monitor.
Created attachment 351212 [details] [review] meta-monitor-config: Add API to rotate the current config This will allow us to do automatic rotation of the builtin display if that's the only active monitor.
Created attachment 351213 [details] [review] monitor-manager: Hook MetaOrientationManager to change the config -- In the previous version I was simply calling meta_monitor_manager_ensure_configured() from the orientation changed handler but I realized that that would cause us to throw away the current configuration and go back to a saved or default config *even* if we then decided not to apply the orientation transform at all. So, I refactored the configuration changing patches and here we're calling the new API provided there to change the current config.
Created attachment 351214 [details] [review] meta-monitor-manager: Add API to rotate the current configuration This will allows us support the XF86RotateWindows key present on some laptops directly in mutter.
Created attachment 351215 [details] [review] keybindings: Add a video-rotate builtin keybinding Moved from g-s-d's media keys plugin since it requires changing the current monitor configuration and we want to remove the old DBus API.
(In reply to Jonas Ådahl from comment #20) > Side question: does disabling it cause the sensor to not be listened too (in > the sensor proxy daemon); or would we need to "unclaim" it to make that > possible? IIRC there are quite a lot of updates i.e. wakeups from a sensor. Yes I'll be doing that too, but it can be done independently of providing the functionality which is the focus of this bug.
(In reply to Rui Matos from comment #15) <snip> > I haven't tested this (I don't have the hardware) but as the code is (and > it's definitely my goal) the orientation provided transform is always > applied even on top of saved configurations. "fake-input-accelerometer" in iio-sensor-proxy is an interactive program that will allow you to "own" this hardware without spending a dime :)
Review of attachment 351210 [details] [review]: ::: src/backends/meta-orientation-manager.c @@ +71,3 @@ + +static void +get_current_orientation (MetaOrientationManager *self) nit: naming is a bit confusing. to me get_ is usually a getter, not something that updates an internal state. @@ +73,3 @@ +get_current_orientation (MetaOrientationManager *self) +{ + gboolean has_accel = TRUE; Is this really what we want? I would assume the opposite, and can't find any documentation about what the absence of "HasAccelerometer" means. @@ +105,3 @@ + + self->orientation_lock = g_settings_get_boolean (self->settings, ORIENTATION_LOCK_KEY); + if (self->orientation_lock) You always set self->orientation_lock before checking it, so there doesn't seem to be a point having it in MetaOrientationManager. @@ +158,3 @@ + g_variant_unref (v); + + sync_state (self); Something for the future maybe, as I suspect it's out of scope for landing these: should we avoid the potential initial mode-reset? If I'm reading things correctly, on startup we'll do 1) Create orientation manager -> get accelerometer async 2) Create monitor manager -> set modes 3) Finish getting accelerometer -> potentially set modes again The problem with this, I suppose, is that we rely on the modeset in 2) to finish before various things done after 2) so its probably not trivial to fix. @@ +224,3 @@ + g_clear_object (&self->iio_proxy); + self->prev_orientation = META_ORIENTATION_UNDEFINED; + self->curr_orientation = META_ORIENTATION_UNDEFINED; Should this call sync_state() instead maybe? It'll set things the same way as here, but it'll also emit the signal, causing the rotation to be reset to the configured state. @@ +241,3 @@ + g_signal_connect_object (self->settings, "changed::"ORIENTATION_LOCK_KEY, + G_CALLBACK (orientation_lock_changed), self, 0); + sync_state (self); This call seems pointless; the orientation will still be undefined at this state as the sensor proxy accelerometer can not possibly have been claimed yet.
Review of attachment 351211 [details] [review]: Lgtm. If we'd ever want to avoid the potential initial mode-reset on startup, we'd have to teach this how to create its own configuration to build upon, but thats another story.
Review of attachment 351212 [details] [review]: lgtm.
Review of attachment 351213 [details] [review]: lgtm, with a couple of nits. ::: src/backends/meta-monitor-manager.c @@ +543,3 @@ + + case META_ORIENTATION_UNDEFINED: + default: nit: Leaving out default: will let us catch added orientations (as if there would ever be any). @@ +561,3 @@ + config, + META_MONITORS_CONFIG_METHOD_TEMPORARY, + NULL); nit: Wouldn't hurt with error reporting his, as is done elsewhere.
Review of attachment 351214 [details] [review]: ::: src/backends/meta-monitor-manager.c @@ +3029,3 @@ return meta_monitor_is_active (laptop_panel); } + As this is exposed via the public API (to be used in gnome-shell I assume?) should it be gtk-doc:ed? @@ +3046,3 @@ + config, + META_MONITORS_CONFIG_METHOD_TEMPORARY, + NULL); nit: same here about error reporting.
Review of attachment 351215 [details] [review]: ::: data/org.gnome.mutter.gschema.xml.in @@ +147,3 @@ </key> + <key name="video-rotate" type="as"> Calling this "video" introduces yet another thing referring to the display/screen/monitor/... and it could easily be confused with video as in what is played by a video player. Maybe 'rotate-monitor' is better? @@ +148,3 @@ + <key name="video-rotate" type="as"> + <default><![CDATA[['XF86RotateWindows']]]></default> Could use a summary, including for example in what direction it rotates. ::: src/core/keybindings.c @@ +3284,3 @@ + meta_backend_get_monitor_manager (backend); + + meta_monitor_manager_video_rotate_activated (monitor_manager); I guess should have been added to the previous patch review, but is "activated" really the right term? Suggestion: meta_monitor_manager_rotate_monitor() ? meta_monitor_manager_rotate_current_monitor() ?
Created attachment 353394 [details] [review] backends: Add a MetaOrientationManager class -- I believe I addressed all review comments here except the liberal usage of the sync_state() call because I find it easier to reason about the state handling by always calling a central syncing point.
Created attachment 353395 [details] [review] monitor-config-manager: Add API to rotate the current config -- rebased
Created attachment 353396 [details] [review] meta-monitor-config: Add API to rotate the current config -- rebased
Created attachment 353397 [details] [review] monitor-manager: Hook MetaOrientationManager to change the config -- addressed comments
Created attachment 353398 [details] [review] meta-monitor-manager: Add API to rotate the current configuration -- addressed comments and made the api private since it's not needed elsewhere
Created attachment 353399 [details] [review] keybindings: Add a rotate-monitor builtin keybinding Moved from g-s-d's media keys plugin, where it was called "video-rotate", since it requires changing the current monitor configuration and we want to remove the old DBus API. -- changed the name as suggested
Created attachment 353400 [details] [review] backends: Add API to switch to predetermined monitor configurations This will allows us to support the XF86Display key present on some laptops, directly in mutter. This is also known, in evdev, as KEY_SWITCHVIDEOMODE. The common usage for this key is to alternate between a few well known multi-monitor configurations though these aren't officially standardized. As an example, Lenovo documents it as: "Switches the display output location between the computer display and an external monitor." On this patch, we're just introducing the configurations that have been implemented in g-s-d until now, which go a bit beyond the above description.
Created attachment 353401 [details] [review] keybindings: Add a switch-monitor builtin keybinding Moved from g-s-d's media keys plugin, where it was called "video-out", since it requires changing the current monitor configuration and we want to remove the old DBus API. This implementation is intentionally simple and not really meant for more than debugging and validating the various configurations. A better user experience will be introduced in gnome-shell with a custom keybinding handler. The default value includes <Super>P in addition to the standard keysym for historical reasons.
Review of attachment 353394 [details] [review]: lgtm.
Review of attachment 353395 [details] [review]: lgtm.
Review of attachment 353396 [details] [review]: lgtm.
Review of attachment 353397 [details] [review]: lgtm
Review of attachment 353398 [details] [review]: lgtm.
Review of attachment 353399 [details] [review]: lgtm.
Review of attachment 353400 [details] [review]: ::: src/backends/meta-monitor-config-manager.c @@ +807,3 @@ + common_mode_w * common_mode_h < mode_w * mode_h) + { + meta_monitor_mode_get_resolution (mode, &common_mode_w, &common_mode_h); I guess you could just set common_mode_(w|h) to mode_(w|h) here? @@ +842,3 @@ + .height = common_mode_h + }, + .scale = 1, Is this really the best? I think we should rather use the preferred mode of the primary monitor than 1. ::: src/backends/meta-monitor-config.c @@ +1718,3 @@ + common_height = outputs[0].modes[i]->height; + } + } This won't work if we want to mirror a 4K tiled external monitor and a non-tiled 4K laptop panel, but being the old config system, I think we should not care. ::: src/backends/meta-monitor-manager.c @@ +2792,3 @@ MetaBackend *backend = meta_get_backend (); + manager->current_switch_config = 0; This means ALL_MIRROR. Is that really correct? I mean the default is *probably* all-linear, but it may also be some custom configuration. Maybe in order to be able to be more correct, we should have a gboolean has_current_witch_config next to it, and let the getter a gboolean .._get(.., MetaMonitorSwitchConfigType *type) { if (has_) { *type = current_type; return TRUE; } else return FALSE; }
Review of attachment 353401 [details] [review]: lgtm. ::: src/core/keybindings.c @@ +3344,3 @@ + MetaScreen *screen, + MetaWindow *window, + ClutterKeyEvent *event, nit: spacing
Created attachment 355600 [details] [review] backends: Add API to switch to predetermined monitor configurations -- (In reply to Jonas Ådahl from comment #51) > @@ +842,3 @@ > + .height = common_mode_h > + }, > + .scale = 1, > > Is this really the best? I think we should rather use the preferred mode of > the primary monitor than 1. Changed it to use the scale returned by meta_monitor_manager_calculate_monitor_mode_scale(). I *think* it should return the same for all monitors since the resolution is the same and thus the MAX() isn't really that useful but kept it anyway. > ::: src/backends/meta-monitor-manager.c > @@ +2792,3 @@ > MetaBackend *backend = meta_get_backend (); > > + manager->current_switch_config = 0; > > This means ALL_MIRROR. Is that really correct? I mean the default is > *probably* all-linear, but it may also be some custom configuration. > > Maybe in order to be able to be more correct, we should have a gboolean > has_current_witch_config next to it, and let the getter a gboolean > .._get(.., MetaMonitorSwitchConfigType *type) { if (has_) { *type = > current_type; return TRUE; } else return FALSE; } Added a new UNKNOWN enum value instead that basically does the same.
Created attachment 355601 [details] [review] keybindings: Add a switch-monitor builtin keybinding -- rebased
Attachment 353394 [details] pushed as aad2280 - backends: Add a MetaOrientationManager class Attachment 353395 [details] pushed as 6d082bf - monitor-config-manager: Add API to rotate the current config Attachment 353396 [details] pushed as 26b6682 - meta-monitor-config: Add API to rotate the current config Attachment 353397 [details] pushed as 6ae42f3 - monitor-manager: Hook MetaOrientationManager to change the config Attachment 353398 [details] pushed as 7360f51 - meta-monitor-manager: Add API to rotate the current configuration Attachment 353399 [details] pushed as c614a2d - keybindings: Add a rotate-monitor builtin keybinding
Review of attachment 355600 [details] [review]: Overall LGTM, except for some compiler complaints. ::: src/backends/meta-monitor-config-manager.c @@ +808,3 @@ + common_mode_w * common_mode_h < mode_w * mode_h) + { + meta_monitor_mode_get_resolution (mode, &common_mode_w, &common_mode_h); IMHO common_mode_w = mode_w; common_mode_h = mode_h; is slightly clearer, as it relates more directly with the condition. (But not a strong opinion, so feel free to ignore) @@ +833,3 @@ + } + + scale = meta_monitor_manager_calculate_monitor_mode_scale (monitor_manager, monitor, mode); We *know* that there's a common mode if we get to this loop, so mode will never be unset. But apparently my compiler doesn't and fails with -Werror=maybe-uninitialized ... ::: src/backends/meta-monitor-config.c @@ +1766,3 @@ +{ + MetaConfiguration *config; + gulong bitmap; This is used undefined later if meta_output_is_laptop() returns false for all outputs
Review of attachment 355601 [details] [review]: ::: data/org.gnome.mutter.gschema.xml.in @@ +158,3 @@ + <key name="switch-monitor" type="as"> + <default><![CDATA[['<Super>p','XF86Display']]]></default> Just to make sure: "Super+P" is what we want to show in the UI? ::: src/core/keybindings.c @@ +3347,3 @@ + + if (config_type == META_MONITOR_SWITCH_CONFIG_UNKNOWN) + config_type = META_MONITOR_SWITCH_CONFIG_ALL_MIRROR; Do we need this? As far as I can see, (CONFIG_UNKNOWN + 1) % CONFIG_UNKNOWN == CONFIG_ALL_MIRROR + 1 that is, just what the below expression returns without special-casing.
Created attachment 355919 [details] [review] backends: Add API to switch to predetermined monitor configurations -- the two conditions you mentioned definitely shouldn't happen but there's no reason to not initialize things properly indeed
Created attachment 355921 [details] [review] keybindings: Add a switch-monitor builtin keybinding -- I added the special casing to be clearer, but yeah it's not needed. re. Super<P> in the UI, well right now it doesn't show up in the UI at all and honestly I prefer to keep it that way since it didn't use to show up when these bindings were in g-s-d either
Review of attachment 355919 [details] [review]: LGTM
Review of attachment 355921 [details] [review]: (In reply to Rui Matos from comment #59) > re. Super<P> in the UI, well right now it doesn't show up in the UI at > all Ah, I missed that it's not added to the keybindings xml; in that case the order clearly doesn't matter ...
Attachment 355919 [details] pushed as 3f9c582 - backends: Add API to switch to predetermined monitor configurations Attachment 355921 [details] pushed as 7e330bd - keybindings: Add a switch-monitor builtin keybinding