GNOME Bugzilla – Bug 786971
Improve display arrangement for more than 2 monitors
Last modified: 2018-04-18 10:10:20 UTC
Currently it is a bit of a pain to change the layout if one has more than three monitors. I think that the core of the issue is that snapping is always enforced, making it e.g. impossible to move a monitor in between two or move it from the center to the side. I think a relatively easy way of solving this issue may be to disable snapping over a certain distance. This would allow the user to temporarily create invalid arrangements so that one can start positioning with any monitor.
*** Bug 787982 has been marked as a duplicate of this bug. ***
I think Bug 787982 was a slightly different issue, however it may be fixed with the same fix that will eventually fix this problem. It turns out, the main issue reported by me before was that it's completely impossible to move the screens if one of them is turned off.
I think I might have fixes for all these issues in bug 789711, re-open this if that one doesn't fix it for you. *** This bug has been marked as a duplicate of bug 789711 ***
Nope, this is an issue that is already enforced by the arrangement widget already. So it is not fixed by those changes (I just tested gnome-control-center master).
Created attachment 364319 [details] [review] display: Add signal to monitor notifying about position changes This is in preparation to a new arrangement widget.
Created attachment 364320 [details] [review] display: Add new arrangement widget and hook it up This commits adds a new arrangement widget, refactoring the existing code and addressing a number of issues: * Forced snapping made laying out more than 2 monitors hard * Random gaps would be shown between monitors * The scaling was fixed and usually tiny * The active monitor can be shown
Created attachment 364321 [details] [review] display: Remove unused old display arrangement code Remove the code that has become unused with the new arrangement widget. There are more possible cleanups as there is some code duplication between cc-display-panel.c and cc-display-arrangment.c at this point.
Review of attachment 364320 [details] [review]: what do you mean by "active monitor can be shown" ? ::: panels/display/cc-display-arrangement.c @@ +35,3 @@ + + GnomeBG *background; + GnomeDesktopThumbnailFactory *thumbnail_factory; Allan has requested that we don't display the wallpaper at all since it just adds visual clutter so this can be dropped @@ +87,3 @@ + } + return active; +} both of these functions should probably move into CcDisplayMonitor and CcDisplayConfig respectively @@ +228,3 @@ + { + CcDisplayMonitor *output = l->data; + g_signal_handlers_disconnect_by_func (output, G_CALLBACK (output_changed_cb), arr); this is not needed since we use g_signal_connect_object() below, i.e. when we're gone these connections go away automatically @@ +246,3 @@ + g_assert (priv->config == NULL); + + priv->config = g_object_ref (config); I'd prefer that we don't overcomplicate the memory management if not needed. Since instances of the arrangement widget should never outlive a config instance there's no need for this IMO @@ +374,3 @@ + /* XXX: We shouldn't be accessing ui-number here like this, but we don't + * have any other way to get it! */ + num = GPOINTER_TO_INT (g_object_get_data (G_OBJECT (output), "ui-number")); not that bad IMHO, but we could add a cc_display_monitor_set/get_ui_number() API @@ +566,3 @@ + + priv->drag_active = FALSE; + /* TODO/FIXME: Fire event to update the OK/Cancel state outside! */ you mean the "updated" signal below? @@ +579,3 @@ +} + +enum SnapDirection { I thought you didn't want snapping? ::: panels/display/cc-display-arrangement.h @@ +36,3 @@ + +CcDisplayArrangement* cc_display_arrangement_new (CcDisplayConfig *config); +CcDisplayConfig* cc_display_arrangement_get_config (CcDisplayArrangement *arr); this api seems unused ::: panels/display/cc-display-panel.c @@ +65,3 @@ CcDisplayMonitor *current_output; + CcDisplayArrangement *arrangement; shouldn't be needed @@ +383,3 @@ { panel->priv->current_output = output; + /* TODO: Do this from a signal handler instead! */ definitely @@ +1296,3 @@ + CcDisplayMonitor *output = cc_display_arrangement_get_selected_output (arr); + if (output && output != panel->priv->current_output) + set_current_output (panel, output); please don't use tabs for indentation @@ +1306,3 @@ + priv->arrangement = cc_display_arrangement_new (priv->current_config); + g_object_add_weak_pointer (G_OBJECT (priv->arrangement), (gpointer*) &priv->arrangement); shouldn't be needed @@ +1310,3 @@ + cc_display_arrangement_set_selected_output (priv->arrangement, panel->priv->current_output); + g_signal_connect_swapped (area, "updated", G_CALLBACK (update_apply_button), panel); + g_signal_connect (area, "notify::selected-output", G_CALLBACK (arrangement_notify_selected_ouptut_cb), panel); please use g_signal_connect_object() to avoid use after free bugs
Review of attachment 364319 [details] [review]: ::: panels/display/cc-display-config-dbus.c @@ +664,3 @@ } + + g_signal_emit_by_name (self, "position"); I'd prefer that we emit only if (old_x != x || old_y != y)
Review of attachment 364321 [details] [review]: looks good
Thanks for the review! I'll update the patch later, to address some of the points: * Showing the "active monitor" (i.e. the one that one can change the resolution of) was something i experimented with (for debugging). It really doesn't belong in the commit message. * I agree that we should get rid of the background drawing and am happy to work on a follow up patch (need to look at mockups). I would leave this commit as is just to split the changes into multiple commits. * Yup, accessing the g_object_get_data to access the "ui-number" is fine. It just seems slightly odd to me and the comment stuck in the end.
I do want snapping (in fact it is necessary). The problem is that the old code always snapped rather than only snapping to maximum distance of e.g. 10px. I have tested the new behaviour on a couple of people in the office and am quite happy with it right now. Any more feedback to improve it further would be welcome of course.
Created attachment 366186 [details] [review] display: Add signal to monitor notifying about position changes This is in preparation to a new arrangement widget.
Created attachment 366187 [details] [review] display: Add new arrangement widget and hook it up This commits adds a new arrangement widget, refactoring the existing code and addressing a number of issues: * Forced snapping made laying out more than 2 monitors hard * Random gaps would be shown between monitors * The scaling was fixed and usually tiny
Created attachment 366188 [details] [review] display: Remove unused old display arrangement code Remove the code that has become unused with the new arrangement widget. There are more possible cleanups as there is some code duplication between cc-display-panel.c and cc-display-arrangment.c at this point.
Created attachment 366189 [details] [review] display: Remove incorrect comment The comment was not updated when the workaround in the code was removed.
Created attachment 366190 [details] [review] display: Move output utility functions into CcDisplayMonitor This adds the following API: * cc_display_config_get_ui_sorted_monitors Returns the monitors in UI order * cc_display_config_count_useful_monitors Counts the useful monitors (active and usable) * cc_display_monitor_is_useful Checks if a monitor is active and usable * cc_display_monitor_is_useable Check if a monitor is marked as useable * cc_display_monitor_set_usable Used to mark builtin monitors as unusable if the lid is closed * cc_display_monitor_get_ui_* Get the UI number and strings for display
Created attachment 366191 [details] [review] display: Draw monitors using CSS Updates the design (removing the background) and ports the drawing to use CSS.
Note that the theming does *not* follow the mockups. The reason is very simple, the mockup uses the wrong font colour (i.e. "normal" state) on top of the "selected" state background. This might be acceptable in Adwaita specifically, but may not work in other situations. I now chose a fixed black/white colour scheme to ensure the text is legible, which is probably not ideal either. For Adwaita, the right thing is probably to override this in the theme. But the default can probably also be improved.
Created attachment 366192 [details] Screenshot of current CSS theming
Review of attachment 366187 [details] [review]: For some reason, this patch is not applying here, with the following output: Applying: display: Add new arrangement widget and hook it up error: sha1 information is lacking or useless (panels/display/cc-display-panel.c). error: could not build fake ancestor Patch failed at 0001 display: Add new arrangement widget and hook it up
Hm, I have never seen that error. A bit of googling suggests that it is a bug in git that could be triggered when a submodule has a conflict. Maybe try with a fresh checkout? http://git.661346.n2.nabble.com/quot-sha1-information-is-lacking-or-useless-quot-when-rebasing-with-a-submodule-pointer-conflict-td7576577.html
(In reply to Benjamin Berg from comment #22) > Hm, I have never seen that error. A bit of googling suggests that it is a > bug in git that could be triggered when a submodule has a conflict. Maybe > try with a fresh checkout? > > http://git.661346.n2.nabble.com/quot-sha1-information-is-lacking-or-useless- > quot-when-rebasing-with-a-submodule-pointer-conflict-td7576577.html I am not able to apply the patch even on a new repository. Could you please try to rebase the patches on top of master, and reupload them here? Thanks, and sorry for the trouble.
Created attachment 367566 [details] [review] display: Add new arrangement widget and hook it up This commits adds a new arrangement widget, refactoring the existing code and addressing a number of issues: * Forced snapping made laying out more than 2 monitors hard * Random gaps would be shown between monitors * The scaling was fixed and usually tiny
Ohh, my bad. The problem is probably that the code was moved to meson. I'll reupload the rest of the affected patches. Sorry about that.
Created attachment 367567 [details] [review] display: Remove unused old display arrangement code Remove the code that has become unused with the new arrangement widget. There are more possible cleanups as there is some code duplication between cc-display-panel.c and cc-display-arrangment.c at this point.
Hm, now Bugzilla screwed up the patch ordering... I'd be enough if you just reuploaded all of them, in the correct sequence (I'm assuming you're using git-bz, right?)
Created attachment 367635 [details] [review] display: Add signal to monitor notifying about position changes This is in preparation to a new arrangement widget.
Created attachment 367636 [details] [review] display: Add new arrangement widget and hook it up This commits adds a new arrangement widget, refactoring the existing code and addressing a number of issues: * Forced snapping made laying out more than 2 monitors hard * Random gaps would be shown between monitors * The scaling was fixed and usually tiny
Created attachment 367637 [details] [review] display: Remove unused old display arrangement code Remove the code that has become unused with the new arrangement widget. There are more possible cleanups as there is some code duplication between cc-display-panel.c and cc-display-arrangment.c at this point.
Created attachment 367638 [details] [review] display: Remove incorrect comment The comment was not updated when the workaround in the code was removed.
Created attachment 367639 [details] [review] display: Move output utility functions into CcDisplayMonitor This adds the following API: * cc_display_config_get_ui_sorted_monitors Returns the monitors in UI order * cc_display_config_count_useful_monitors Counts the useful monitors (active and usable) * cc_display_monitor_is_useful Checks if a monitor is active and usable * cc_display_monitor_is_useable Check if a monitor is marked as useable * cc_display_monitor_set_usable Used to mark builtin monitors as unusable if the lid is closed * cc_display_monitor_get_ui_* Get the UI number and strings for display
Created attachment 367640 [details] [review] display: Draw monitors using CSS Updates the design (removing the background) and ports the drawing to use CSS.
Review of attachment 367635 [details] [review]: A nitpick ::: panels/display/cc-display-config.c @@ +108,3 @@ 0, NULL, NULL, NULL, G_TYPE_NONE, 0); + g_signal_new ("position", Nitpick: "position-changed" would be a better name for the signal.
Review of attachment 367636 [details] [review]: A few comments below. I'd like to suggest a major reorganization of the cc-display-arrangement.c file: 1. Structs and enums 2. G_DEFINE_TYPE() 3. Static functions 4. Callbacks 5. class_init() 6. init() 7. Public functions (in the same order they appear at the .h file) ::: panels/display/cc-display-arrangement.c @@ +42,3 @@ + gdouble drag_anchor_x; + gdouble drag_anchor_y; +}; Final classes don't need a Private instance. You can just use the CcDisplayArrangement structure here, and remove all the private-related code. @@ +90,3 @@ + +static void +apply_rotation_to_geometry (CcDisplayMonitor *output, int *w, int *h) Nitpick: one argument per line. @@ +106,3 @@ +/* get_geometry */ +static void +get_scaled_geometry (CcDisplayConfig *config, CcDisplayMonitor *output, int *x, int *y, int *w, int *h) Nitpick: one argument per line. @@ +323,3 @@ + GList *outputs, *l; + + g_return_val_if_fail (priv->config, FALSE); g_assert() in static functions, g_return{,_val}_if_fail() in public functions. @@ +498,3 @@ + return FALSE; + + g_return_val_if_fail (priv->drag_active == FALSE, FALSE); Ditto. @@ +554,3 @@ + SNAP_DIR_X = 1 << 0, + SNAP_DIR_Y = 1 << 1, +}; Please move this to above G_DEFINE_TYPE. And also, typedef enum { ... } SnapDirection. @@ +555,3 @@ + SNAP_DIR_Y = 1 << 1, +}; +#define SNAP_DIR_BOTH (SNAP_DIR_X | SNAP_DIR_Y) This can be a regular enum value too. @@ +569,3 @@ + gint mon_x, gint mon_y, + gint new_x, gint new_y, + gdouble *dist_x, gdouble *dist_y) Nit: mind the argument alignment. @@ +590,3 @@ + gint mon_x, gint mon_y, + gint new_x, gint new_y, + gboolean snapped) Ditto.
Review of attachment 367637 [details] [review]: Sweet. What are these "more cleanups"? Could you also do that in a separate patch?
Review of attachment 367638 [details] [review]: Sure
Review of attachment 367639 [details] [review]: Looks good to me.
Review of attachment 367640 [details] [review]: I'd like to have a design feedback on the styles here, but otherwise looks good. ::: panels/display/cc-display-panel.c @@ +2342,3 @@ { CcDisplayPanelPrivate *priv; + GtkCssProvider *provider; Use g_autoptr()
Created attachment 367696 [details] [review] display: Add signal to monitor notifying about position changes This is in preparation to a new arrangement widget.
Created attachment 367697 [details] [review] display: Add new arrangement widget and hook it up This commits adds a new arrangement widget, refactoring the existing code and addressing a number of issues: * Forced snapping made laying out more than 2 monitors hard * Random gaps would be shown between monitors * The scaling was fixed and usually tiny
Created attachment 367698 [details] [review] display: Remove unused old display arrangement code Remove the code that has become unused with the new arrangement widget. There are more possible cleanups as there is some code duplication between cc-display-panel.c and cc-display-arrangment.c at this point.
Created attachment 367699 [details] [review] display: Remove incorrect comment The comment was not updated when the workaround in the code was removed.
Created attachment 367700 [details] [review] display: Move output utility functions into CcDisplayMonitor This adds the following API: * cc_display_config_get_ui_sorted_monitors Returns the monitors in UI order * cc_display_config_count_useful_monitors Counts the useful monitors (active and usable) * cc_display_monitor_is_useful Checks if a monitor is active and usable * cc_display_monitor_is_useable Check if a monitor is marked as useable * cc_display_monitor_set_usable Used to mark builtin monitors as unusable if the lid is closed * cc_display_monitor_get_ui_* Get the UI number and strings for display
Following changes: * Addressed the review comments * Folded the CSS update into the widget directly (splitting that with the code re-shuffling was just a bit of a pain) * UI CSS has been updated as discussed with Jimmac - This includes adding support for margins
I've tried these patches yesterday. The main issue I see with the new behaviour is that it's possible to drag a display so that it is separated from any others, this is then interpreted as an error state, for which we show a generic message. Really, it shouldn't be possible for a user to set an invalid arrangement. Preventing errors of this type is a basic UX principle. I've done a set of mockups which detail one way that invalid arrangements could be avoided, here: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/displays/arrangement-widget.png Other approaches might also be possible.
Adding the required logic to implement the physics for all this is too much effort for us currently. What we could do is to basically increase the snapping distance to infinity for the case of two monitors. That would avoid the regression that Allan is identifying while still improving the currently broken use cases.
if i may chime in on this one.. i am on an 8 monitor setup and my issue isnt so much that snapping but i cant even grab a monitor as its only a pixel wide or so.. i also cant tell which monitor is which.. see this screenshot: https://www.screencast.com/t/NQpUJDp8Gp so please, also consider a way to have a min size of a monitor or so. thank you, Andreas
Hi Andreas, that would already be a lot better with this patchset, as the full width would be used properly. However, still not ideal. My guess is that for your use case, we would need a way for the user to get a larger preview. For example by detaching the arrangement widget into a separate window rather than embedding it.
Hi Benjamin, thank you for your quick reply.. a larger preview might be better, i agree.. but having it at least pickable is all i'd ask for.. i'll give it a shot once its available on a live cd (no installed linux yet, need to figure out some other stuff that depends on windows, unfortunately..) also - i dont know if this is part of the applet, though - please notice that it doesnt show the "monitor number" on my displays.. so i cant possible see what (e.g.) Monitor 5 is idk if this is a different bug or not..
(In reply to Andreas Wunder from comment #50) > also - i dont know if this is part of the applet, though - please notice > that it doesnt show the "monitor number" on my displays.. so i cant possible > see what (e.g.) Monitor 5 is Weird. Is that even a gnome-shell running? The display numbers are rendered by gnome-shell and gnome-control-center calls a DBus API to show them. > idk if this is a different bug or not.. Anyway, if this is a gnome-shell, then it is a bug. Either in g-c-c or gnome-shell, hard to tell.
in that particular case it was running within budgie.. i was asking them abt the monitor arrangement issue and they said budgie is using the gnome applet, hence i am here. so, judged by your reply, it looks like this is a budgie issue then.. however, they also said in v11 of their desktop they want to introduce their own control panel so it probably doesnt make a lot of sense to fix something deprecated. thank you for letting me know! cheers Andreas
This bug is now being tracked in [1] and the code review is happening at [2]. Please use those channels to discuss about the current patches. [1] https://gitlab.gnome.org/GNOME/gnome-control-center/issues/20 [2] https://gitlab.gnome.org/GNOME/gnome-control-center/merge_requests/13
(In reply to Benjamin Berg from comment #47) > Adding the required logic to implement the physics for all this is too much > effort for us currently. What we could do is to basically increase the > snapping distance to infinity for the case of two monitors. > > That would avoid the regression that Allan is identifying while still > improving the currently broken use cases. Would that mean that the issues I identified would still be present for more than two displays? I can see if I can come up with some simpler design guidelines, if that would help...
(In reply to Allan Day from comment #54) > Would that mean that the issues I identified would still be present for more > than two displays? Yes, I am proposing to special case the two monitor behaviour, because it is easy to impose restrictions in that case. There are a lot of good reasons to merge this patchset from an engineering point of view (also improving the design). The issue you identified exists, and I agree that it is a regression in the case of two monitors. However, in the case of three or more monitors this "issue" actually becomes a feature. Not because it is a great solution, but because it is a major improvement on the status quo. I was hoping to get a blessing from the design time (even if grudgingly) that it was a reasonable compromise to move forward.
(In reply to Benjamin Berg from comment #55) ... > Yes, I am proposing to special case the two monitor behaviour, ... > However, in the case of three or more monitors this "issue" actually becomes > a feature. Not because it is a great solution, but because it is a major > improvement on the status quo. It doesn't seem like a good solution if the UI makes it easy (almost encourages) people to create broken configurations. I don't see how restricting the "fix" to more than two displays resolves that. > I was hoping to get a blessing from the design time (even if grudgingly) > that it was a reasonable compromise to move forward. It shouldn't be a matter of compromise - the goal is to collectively come up with the best design we can. You said that the design I came up with would be too much work to implement. If you could provide some information on what the technical constraints are, I could take another look.
(In reply to Allan Day from comment #56) > It shouldn't be a matter of compromise - the goal is to collectively come up > with the best design we can. Allan, I found the feedback from you and Jakub while working on this patchset very helpful. Talking to Jakub very much improved the visual design, and you did identify an important regression and proposed some interesting ideas to improve the UX. However, I do not think that the staying with the status quo indefinitely is a decent solution. A quick list of issues with the status quo: * UI interactions with 3 or more monitors area a pain or may even be impossible * UI design is outdated * Monitors have random gaps in between them while arranging (0-2 pixel) * UI does not indicate which monitor is being configured below * Code is ancient and so often re-structured to be effectively unmaintainable * Monitors are tiny because of calculation errors (gets worse with more monitors) Identified issues with the new code: * Monitors do not snap in a way that ensures valid configurations (invalid configurations can be created but not applied) I proposed a possible fix for that issue for the case of two monitors. The case of three or more monitors is already special, because it is currently broken. I strongly believe that the possible new UI issues–at least when limited to 3 or more monitors–are far less sever than the status quo.
(In reply to Benjamin Berg from comment #57) > (In reply to Allan Day from comment #56) > > It shouldn't be a matter of compromise - the goal is to collectively come up > > with the best design we can. > > Allan, I found the feedback from you and Jakub while working on this > patchset very helpful. Talking to Jakub very much improved the visual > design, and you did identify an important regression and proposed some > interesting ideas to improve the UX. > > However, I do not think that the staying with the status quo indefinitely is > a decent solution. > > A quick list of issues with the status quo: > * UI interactions with 3 or more monitors area a pain or may even be > impossible > * UI design is outdated > * Monitors have random gaps in between them while arranging (0-2 pixel) > * UI does not indicate which monitor is being configured below > * Code is ancient and so often re-structured to be effectively > unmaintainable > * Monitors are tiny because of calculation errors (gets worse with more > monitors) > > Identified issues with the new code: > * Monitors do not snap in a way that ensures valid configurations > (invalid configurations can be created but not applied) > > I proposed a possible fix for that issue for the case of two monitors. The > case of three or more monitors is already special, because it is currently > broken. I strongly believe that the possible new UI issues–at least when > limited to 3 or more monitors–are far less sever than the status quo. I'm suggesting that we try and find a new option - something different from the current implementation, your suggestion, and the mockups I previously did.