GNOME Bugzilla – Bug 750311
add support for tiled monitors
Last modified: 2015-07-01 08:46:43 UTC
Created attachment 304462 [details] [review] patch to add tiled support to gnome-desktop This builds on the changes to mutter, and encapsulates tiled monitors inside gnome-desktop. The new API added is just to get if a monitor is tiled and is to be used by gnome-control-center.
Review of attachment 304462 [details] [review]: Without seeing how this is going to be used in g-c-c, it's difficult to to a more in depth review ::: libgnome-desktop/gnome-rr.c @@ +112,3 @@ int height; int freq; /* in mHz */ + int tile_flags; why is this flags and not just a boolean? it's private so we can change it later if needed ::: libgnome-desktop/gnome-rr.h @@ +174,3 @@ gboolean gnome_rr_output_get_is_underscanning (GnomeRROutput *output); + spurious new line
All the control-center patches are here. http://cgit.freedesktop.org/~airlied/gnome-control-center/
Created attachment 305041 [details] [review] patch to add tiled support to gnome-desktop updated with comments from Rui.
Review of attachment 305041 [details] [review]: ::: libgnome-desktop/gnome-rr-output-info.c @@ +160,2 @@ /** * gnome_rr_output_info_get_geometry: The API doc for this one needs to explain that it will get the information for the whole tile, not just the output. @@ +186,3 @@ } +static void gnome_rr_output_info_set_tiled_geometry (GnomeRROutputInfo *self, int x, int y, int width, int height) A comment explaining what this does, in case somebody other than you manages to understand what it does ;) @@ +203,3 @@ + for (ht = 0; ht < self->priv->tile.max_horiz_tiles; ht++) + { + y_off = 0; If it's only used inside the loop, please define it at the beginning of the loop block. @@ +204,3 @@ + { + y_off = 0; + addx = 0; Ditto. @@ +289,3 @@ } +static void gnome_rr_output_info_set_tiled_rotation (GnomeRROutputInfo *self, GnomeRRRotation rotation) Ditto gnome_rr_output_info_set_tiled_geometry(), a little explanation as to what we need to be aware of would be nice. @@ +477,3 @@ } + +gboolean gnome_rr_output_info_is_primary_tile(GnomeRROutputInfo *self) This needs API docs. ::: libgnome-desktop/gnome-rr-private.h @@ +89,3 @@ gboolean underscanning; + + gboolean tiled; I prefer "is_tiled" ::: libgnome-desktop/gnome-rr.c @@ +351,3 @@ + /* now stick the mode into the modelist */ + a = g_ptr_array_new (); + mode = mode_new(info, 0); Space before parenthesis (and in a bunch of other places in that patch as well). Also replace the magic "0" with a #define'd constant (such as "UNDEFINED_MODE_ID") and do a comparison to that in the internal "get_tiled_info" below. @@ +1375,3 @@ + if ((tile = g_variant_lookup_value (properties, "tile", G_VARIANT_TYPE ("(uuuuuuuu)")))) + { + const guint32 *values = g_variant_get_data (tile); g_variant_get (tile, "(uuuuuuuu)", &output->tile_info.group_id, etc. @@ +1677,3 @@ + mode = gnome_rr_crtc_get_current_mode (crtc); + + if (_gnome_rr_output_get_tiled_display_size (output, &tile_w, &tile_h, &total_w, &total_h)) A little comment would help here. @@ +2013,3 @@ +gboolean +gnome_rr_mode_get_is_tiled (GnomeRRMode *mode) Needs API docs. @@ +2177,3 @@ + GnomeRRTile *tile) +{ + if (!output->tile_info.group_id) if (output->tile_info.group_id == UNDEFINED_GROUP_ID) instead.
Created attachment 305265 [details] [review] patch to add tiled support to gnome-desktop Another revision that should address all of Bastien's review The only think I was unsure off was adding the UNDEFINED_ the review mixed up UNDEFINED_MODE_ID and UNDEFINED_GROUP_ID usage, I've added the defines anyways to be used, but I'm not sure if they are really useful or not.
Review of attachment 305265 [details] [review]: Looks good to commit after fixing those nitpicks. ::: libgnome-desktop/gnome-rr-config.c @@ +167,3 @@ + output->priv->config = config; + output->priv->is_tiled = _gnome_rr_output_get_tile_info (rr_output, + &output->priv->tile); Is this aligned properly? I can't see in the review tool. ::: libgnome-desktop/gnome-rr-debug.c @@ +70,3 @@ + gnome_rr_mode_get_width (modes[i]), + gnome_rr_mode_get_height (modes[i]), + gnome_rr_mode_get_is_tiled (modes[i])); Change the format so you can do something like: gnome_rr_mode_get_is_tiled (modes[i]) ? " (tiled)" : "" ::: libgnome-desktop/gnome-rr-output-info.c @@ +118,3 @@ + * if it is the 0 tile, store the x/y offsets. + * if the tile is active, add the tile to the total w/h + * for the output depending. I can't parse that last line. @@ +217,3 @@ + * for tile 0 only. + * if all tiles are being set, then store the + * dimensions for this tile, and increase the offsets Is a full stop missing here? @@ +523,3 @@ + * @self: a #GnomeRROutputInfo + * + * Returns: TRUE if the specified output is connected to %TRUE @@ +525,3 @@ + * Returns: TRUE if the specified output is connected to + * the primary tile of a monitor or to an untiled monitor, + * FALSE is the output is connected to a secondary tile. %FALSE if
Created attachment 306455 [details] [review] patch with nitpicks fixed. please apply as I don't have commit access.
And pushed! And soname/version bumped!