Merge lp:~veger/lightdm/lp-1197999 into lp:lightdm

[Maarten Bezemer]

Hereby the modifications to add the --screen and --fullscreen options to the dm-tools command

[Robert Ancell (robert-ancell)]

Hi Maarten,

Thanks for this patch.

Could you make some simple changes to it:

1. Instead of the if (n_options == 1), could you iterate over the remaining options so there's no code duplication? i.e.

for (i = 0; i < n_options; i++)
{
   if (strcmp (options[i], "--fullscreen") == 0)
       ...
   else if (strcmp (options[i], "--screen") == 0)
       ...
   else
       ... // show warning and exit
}

2. Could you use a string or enum to avoid the magic numbers for screen_mode

i.e. you could use:

if (dimensions == NULL)
   ... // Use Xephyr
else if (strcmp (dimensions, "fullscreen") == 0)
   ... // Use Xephyr -fullscreen
else
   ... // Use Xephyr -screen DIMENSIONS

review: Needs Fixing

[lp:~veger/lightdm/lp-1197999]

1866. By Maarten Bezemer on 2014-01-06

As per review comments:
 * Remove code duplication when parsing the given options
 * Avoid magic numbers, use dimensions string to select the correct Xephyr command

------------- This line and the following will be ignored --------------

modified:
  src/dm-tool.c

[Maarten Bezemer (veger)]

Using a for loop to remove code duplications does not work (IMHO), as the amount of options needs to be checks (so garbage after the options is detected). But I think I managed to get it to a minimum this way.

[Robert Ancell (robert-ancell)]

Sorry, one last change - you don't need the debian/changelog entry - that will be added when it is released. Other than that, good to go, thanks!

[lp:~veger/lightdm/lp-1197999]

1867. By Maarten Bezemer on 2014-01-06

Remove debian/changelog entry

[Maarten Bezemer (veger)]

I do not know whether you get alerted when a new commit is added. Hence this comment:

I removed the changes to debian/changelog

[Robert Ancell (robert-ancell)]

Great! Just waiting confirmation on contributor agreement before this can be merged.

review: Approve

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'src/dm-tool.c'
--- src/dm-tool.c	2013-11-07 07:54:55 +0000
+++ src/dm-tool.c	2014-01-06 22:58:22 +0000
@@ -132,14 +132,14 @@
                         "  --session-bus     Use session D-Bus\n"
                         "\n"
                         "Commands:\n"
-                        "  switch-to-greeter                   Switch to the greeter\n"
-                        "  switch-to-user USERNAME [SESSION]   Switch to a user session\n"
-                        "  switch-to-guest [SESSION]           Switch to a guest session\n"
-                        "  lock                                Lock the current seat\n"
-                        "  list-seats                          List the active seats\n"
-                        "  add-nested-seat                     Start a nested display\n"
-                        "  add-local-x-seat DISPLAY_NUMBER     Add a local X seat\n"
-                        "  add-seat TYPE [NAME=VALUE...]       Add a dynamic seat\n");
+                        "  switch-to-greeter                                    Switch to the greeter\n"
+                        "  switch-to-user USERNAME [SESSION]                    Switch to a user session\n"
+                        "  switch-to-guest [SESSION]                            Switch to a guest session\n"
+                        "  lock                                                 Lock the current seat\n"
+                        "  list-seats                                           List the active seats\n"
+                        "  add-nested-seat [--fullscreen|--screen DIMENSIONS]   Start a nested display\n"
+                        "  add-local-x-seat DISPLAY_NUMBER                      Add a local X seat\n"
+                        "  add-seat TYPE [NAME=VALUE...]                        Add a dynamic seat\n");
             return EXIT_SUCCESS;
         }
         else if (strcmp (arg, "-v") == 0 || strcmp (arg, "--version") == 0)
@@ -390,6 +390,7 @@
     else if (strcmp (command, "add-nested-seat") == 0)
     {
         gchar *path, *xephyr_command, **xephyr_argv;
+        gchar *dimensions = NULL;
         GMainLoop *loop;
 
         path = g_find_program_in_path ("Xephyr");
@@ -399,6 +400,25 @@
             return EXIT_FAILURE;
         }
 
+        if (n_options > 0)
+        {
+            /* Parse the given options */
+            if (strcmp (options[0], "--fullscreen") == 0 && n_options == 1)
+            {
+                dimensions = "fullscreen";
+            }
+            else if (strcmp (options[0], "--screen") == 0 && n_options == 2)
+            {
+                dimensions = options[1];
+            }
+            else
+            {
+                g_printerr ("Usage add-nested-seat [--fullscreen|--screen DIMENSIONS]\n");
+                usage ();
+                return EXIT_FAILURE;
+            }
+        }
+
         /* Get a unique display number.  It's racy, but the only reliable method to get one */
         xephyr_display_number = 0;
         while (TRUE)
@@ -419,7 +439,18 @@
         /* Wait for signal from Xephyr is ready */
         signal (SIGUSR1, xephyr_signal_cb);
 
-        xephyr_command = g_strdup_printf ("Xephyr :%d", xephyr_display_number);
+        if (dimensions == NULL)
+        {
+            xephyr_command = g_strdup_printf ("Xephyr :%d ", xephyr_display_number);
+        }
+        else if (strcmp (dimensions, "fullscreen") == 0)
+        {
+            xephyr_command = g_strdup_printf ("Xephyr :%d -fullscreen", xephyr_display_number);   
+        }
+        else
+        {
+            xephyr_command = g_strdup_printf ("Xephyr :%d -screen %s", xephyr_display_number, dimensions); 
+        }
         if (!g_shell_parse_argv (xephyr_command, NULL, &xephyr_argv, &error) ||
             !g_spawn_async (NULL, xephyr_argv, NULL,
                             G_SPAWN_DO_NOT_REAP_CHILD | G_SPAWN_SEARCH_PATH | G_SPAWN_STDOUT_TO_DEV_NULL | G_SPAWN_STDERR_TO_DEV_NULL,