GNOME Bugzilla – Bug 765267
Don't show orientation lock when g-s-d won't rotate
Last modified: 2016-04-22 14:31:59 UTC
gnome-shell will show the orientation toggle button when iio-sensor-proxy says that it has an accelerometer. But the XRandR plugin in gnome-settings-daemon, which applies the orientation changes, will only rotate the main screen if it is a builtin screen: https://git.gnome.org/browse/gnome-settings-daemon/tree/plugins/xrandr/gsd-xrandr-manager.c#n1090 And the code to check whether there is a "laptop screen": https://git.gnome.org/browse/gnome-settings-daemon/tree/plugins/xrandr/gsd-xrandr-manager.c#n306 This would avoid showing the orientation lock on desktops, as mentioned in: https://bugzilla.gnome.org/show_bug.cgi?id=763754
Created attachment 326450 [details] [review] monitor-config: Update laptop heuristics to match GnomeRROutput gnome-desktop's GnomeRROutput class has heuristics to classify a display as builtin similar to our own[0]. The two heuristics don't quite match though, so different core components can end up with a different view on the current display configuration. Minimize that risk by adding a couple of rules that bring the two heuristics closer together. [0] https://git.gnome.org/browse/gnome-desktop/tree/libgnome-desktop/gnome-rr.c#n1674
Created attachment 326451 [details] [review] monitor-manager: Add get_has_builtin_output() Wrap the existing laptop_display_is_on() method in a public function that gnome-shell can use to query whether a builtin output is present and enabled.
Created attachment 326452 [details] [review] system: Only show rotation lock when a builtin output is present We currently show the orientation lock button when an accelerometer is present, however gnome-settings-daemon's xrandr plugin only applies rotation when a builtin monitor is present. Update the button's visibility to match gnome-settings-daemon.
Note that I don't have hardware to test, so those patches are only compile-tested.
(In reply to Florian Müllner from comment #4) > Note that I don't have hardware to test, so those patches are only > compile-tested. You can run src/fake-input-accelerometer in iio-sensor-proxy to create a fake one. It will expect input to switch the orientation. Make sure to install iio-sensor-proxy from git however, otherwise you'll need to start iio-sensor-proxy by hand (as root as well).
Patches look fine to me, FWIW.
(In reply to Bastien Nocera from comment #5) > (In reply to Florian Müllner from comment #4) > > Note that I don't have hardware to test, so those patches are only > > compile-tested. > > You can run src/fake-input-accelerometer in iio-sensor-proxy to create a > fake one. Thanks. I've done a quick test now, and the orientation-lock button disappears as expected when turning off the built-in monitor.
Review of attachment 326452 [details] [review]: this looks fine
Review of attachment 326450 [details] [review]: ::: src/backends/meta-monitor-config.c @@ +929,3 @@ case META_CONNECTOR_TYPE_eDP: case META_CONNECTOR_TYPE_LVDS: + case META_CONNECTOR_TYPE_DSI: can we export this function or move it elsewhere... ::: src/backends/meta-monitor-manager.c @@ +516,3 @@ case META_CONNECTOR_TYPE_LVDS: case META_CONNECTOR_TYPE_eDP: + case META_CONNECTOR_TYPE_DSI: ... so that we can re-use it here?
Review of attachment 326451 [details] [review]: ::: src/backends/meta-monitor-config.c @@ +1063,3 @@ + + if (self->current) + return laptop_display_is_on (self->current); This will return false in case of a laptop where the internal display has been turned off in configuration. See below ::: src/backends/meta-monitor-manager.c @@ +1598,3 @@ + g_return_val_if_fail (META_IS_MONITOR_MANAGER (manager), FALSE); + + return meta_monitor_config_get_has_builtin_output (manager->config); Having a builtin output isn't configuration though. We should be looking at manager->outputs[i].connector_type here
Could we extend the D-Bus API so that mutter exports whether it thinks something is a builtin/laptop display? That way, we could remove the heuristics code from gnome-desktop and have a single answer for that.
(In reply to Rui Matos from comment #10) > Review of attachment 326451 [details] [review] [review]: > > ::: src/backends/meta-monitor-config.c > @@ +1063,3 @@ > + > + if (self->current) > + return laptop_display_is_on (self->current); > > This will return false in case of a laptop where the internal display has > been turned off in configuration. Mmh, I thought that was the correct behavior. In fact, it's how I tested in comment #7. Bastien?
(In reply to Florian Müllner from comment #12) > (In reply to Rui Matos from comment #10) > > Review of attachment 326451 [details] [review] [review] [review]: > > > > ::: src/backends/meta-monitor-config.c > > @@ +1063,3 @@ > > + > > + if (self->current) > > + return laptop_display_is_on (self->current); > > > > This will return false in case of a laptop where the internal display has > > been turned off in configuration. > > Mmh, I thought that was the correct behavior. In fact, it's how I tested in > comment #7. Bastien? That's the expected behaviour. We only "touch" the builtin display, so it needs to be 1) present (on a laptop or tablet) 2) not turned off (tablet/laptop turned off with external display turned on shouldn't return true)
Created attachment 326500 [details] [review] monitor-manager: Expose output_is_laptop() method We currently duplicate the heuristics of whether an output is considered a laptop or not. Avoid this by sharing a small helper method.
Created attachment 326501 [details] [review] monitor-config: Update laptop heuristics to match GnomeRROutput (In reply to Rui Matos from comment #9) > can we export this function or move it elsewhere... > > ::: src/backends/meta-monitor-manager.c > @@ +516,3 @@ > case META_CONNECTOR_TYPE_LVDS: > case META_CONNECTOR_TYPE_eDP: > + case META_CONNECTOR_TYPE_DSI: > > ... so that we can re-use it here? Yeah, makes sense. [0] https://git.gnome.org/browse/gnome-desktop/tree/libgnome-desktop/gnome-rr.c#n1674
(In reply to Bastien Nocera from comment #13) > (In reply to Florian Müllner from comment #12) > > (In reply to Rui Matos from comment #10) > > > + if (self->current) > > > + return laptop_display_is_on (self->current); > > > > > > This will return false in case of a laptop where the internal display has > > > been turned off in configuration. > > > > Mmh, I thought that was the correct behavior. In fact, it's how I tested in > > comment #7. Bastien? > > That's the expected behaviour. The implemented behavior is correct then. But maybe we need better function names to clarify the intention?
Review of attachment 326500 [details] [review]: ++
(In reply to Florian Müllner from comment #16) > (In reply to Bastien Nocera from comment #13) > > That's the expected behaviour. > > The implemented behavior is correct then. But maybe we need better function > names to clarify the intention? Ok, in that case I think we need a better name yes. _is_builtin_display_on() ?
Review of attachment 326501 [details] [review]: ok
Created attachment 326517 [details] [review] monitor-manager: Add get_is_builtin_display_on() (In reply to Rui Matos from comment #18) > _is_builtin_display_on() ? Works for me.
Created attachment 326518 [details] [review] system: Only show rotation lock when a builtin output is present Use new method name.
Review of attachment 326517 [details] [review]: ++
Attachment 326500 [details] pushed as ab6c008 - monitor-manager: Expose output_is_laptop() method Attachment 326501 [details] pushed as b6f11fa - monitor-config: Update laptop heuristics to match GnomeRROutput Attachment 326517 [details] pushed as ed5c3b3 - monitor-manager: Add get_is_builtin_display_on()
Attachment 326518 [details] pushed as 9c483dd - system: Only show rotation lock when a builtin output is present