Re: [PR] GUACAMOLE-288: Add support for multi-monitor connections. [guacamole-server]

["neandrake (via GitHub)" <gi...@apache.org>]

neandrake commented on code in PR #560:
URL: https://github.com/apache/guacamole-server/pull/560#discussion_r1966143334


##########
src/protocols/rdp/channels/disp.c:
##########
@@ -191,6 +193,15 @@ void guac_rdp_disp_update_size(guac_rdp_disp* disp,
 
     int width = disp->requested_width;
     int height = disp->requested_height;
+    int monitors_count = disp->requested_monitors;
+
+    /* Prevent opening too many monitors than allowed */
+    if (settings->max_secondary_monitors + 1 < monitors_count)
+        monitors_count = settings->max_secondary_monitors + 1;

Review Comment:
   This makes it seem like `max_secondary_monitors` is 0-based instead of 1-based. Is that the case? It should be 1-based since it's a count and not an index.



##########
src/protocols/rdp/channels/disp.c:
##########
@@ -191,6 +193,15 @@ void guac_rdp_disp_update_size(guac_rdp_disp* disp,
 
     int width = disp->requested_width;
     int height = disp->requested_height;
+    int monitors_count = disp->requested_monitors;
+
+    /* Prevent opening too many monitors than allowed */
+    if (settings->max_secondary_monitors + 1 < monitors_count)

Review Comment:
   This condition is odd and might be simpler to read as
   ```c
   int requested_monitors = disp->requested_monitors;
   
   if (requested_monitors > settings->max_secondary_monitors) {
   //...
   }
   ```



##########
src/protocols/rdp/channels/disp.c:
##########
@@ -204,7 +215,7 @@ void guac_rdp_disp_update_size(guac_rdp_disp* disp,
 
     /* Do NOT send requests unless the size will change */
     if (rdp_inst != NULL
-            && width == guac_rdp_get_width(rdp_inst)
+            && width * monitors_count == guac_rdp_get_width(rdp_inst)
             && height == guac_rdp_get_height(rdp_inst))
         return;

Review Comment:
   This makes the assumption that monitors are horizontally aligned and have the same resolution. Is that a constraint for this change? It's probably a common setup it would be nice to be flexible in its support.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org