Created attachment 280735 [details] [review]
Separate canonicalization of device name

Following are patches (7) which implement addition of LPD and socket printers.

This patch just moves a part of code to its own function so we can reuse it later.

Created attachment 280736 [details] [review]
Export name of selected PPD from PPD dialog

This patch adds pp_ppd_selection_dialog_get_ppd_display_name() function
which returns human-readable name of selected PPD. We will need it later.

Created attachment 280737 [details] [review]
Don't request port during creation of PpHost

This patch allows us to distinguish between situation when user entered port number and when he did not.

Created attachment 280738 [details] [review]
Add functions for searching for socket printers

This patch adds functions for searching for socket printers. It just checks whether it is possible to connect to given hostname:port.

Created attachment 280739 [details] [review]
Add functions for searching for LPD printers

This patch adds functions for detection of LPD printers. It connects to given hostname:port and requests state of a queue (we test several standard queue names). We suppose that there is a LPD printer on the address if a buffer
with zero length is returned.

Created attachment 280740 [details] [review]
Allow to add socket and LPD printers manually

This patch adds usage of the socket and LPD bits. If a port is specified by user then it is passed to the LPD's PpHost and socket's PpHost only if given uri is of "lpd" or "socket" scheme. The PpHosts use default ports otherwise.

Created attachment 280741 [details] [review]
Strip redundant strings from found devices

This last patch is little out of scope of this bug but the fixed problem is emphasized by the previous patches and the patch also depends partially on the previous patches.

It removes several common strings from driver names so that they are not so cryptic when used as printer names (users are required to specify drivers of the socket or LPD printers).
It also removes leading, trailing and recurrent dashes from the names.

Review of attachment 280735 [details] [review]:

Looks good otherwise.

::: panels/printers/pp-new-printer-dialog.c
@@ +102,3 @@
 static void     t_device_free (gpointer data);
 static TDevice *t_device_copy (TDevice *device);
+static gchar   *canonize_device_name (PpNewPrinterDialog *dialog,

canonicalize.

Review of attachment 280736 [details] [review]:

Looks fine.

Review of attachment 280737 [details] [review]:

::: panels/printers/pp-host.h
@@ +35,3 @@
 #define PP_HOST_GET_CLASS(o) (G_TYPE_INSTANCE_GET_CLASS ((o), PP_TYPE_HOST, PpHostClass))
 
+#define PP_HOST_DEFAULT_PORT      -1

I prefer UNSET_PORT

::: panels/printers/pp-new-printer-dialog.c
@@ +1639,3 @@
         {
           gchar *host = NULL;
+          gint   port = PP_HOST_DEFAULT_PORT;

I'd rather you set that in parse_uri.

Review of attachment 280738 [details] [review]:

Some explanation as to what "socket printers" are would be useful. To me, that knows what a socket is, a socket printer is a bit of a bizarre term.

Wikipedia or authoritative link would be useful in the code as well as in the commit message. Substitute for "JetDirect" if that's the only thing it supports.

::: panels/printers/pp-host.h
@@ +35,3 @@
 #define PP_HOST_GET_CLASS(o) (G_TYPE_INSTANCE_GET_CLASS ((o), PP_TYPE_HOST, PpHostClass))
 
+#define PP_HOST_DEFAULT_PORT             -1

again UNSET_PORT

Review of attachment 280739 [details] [review]:

::: panels/printers/pp-host.c
@@ +656,3 @@
+              else
+                {
+                  g_warning ("%s\n", error->message);

Won't that...

@@ +662,3 @@
+          else
+            {
+              g_warning ("%s\n", error->message);

...or that, print loads of warnings for some remote devices?

@@ +702,3 @@
+
+  address = g_strdup_printf ("%s:%d", priv->hostname, port);
+  if (address != NULL && address[0] != '/')

Instead of having one huge conditional that goes to the end of the function, flip the condition around and exit early.

@@ +762,3 @@
+                                                    found_queue);
+              /* Translators: The found device is a Line Printer Daemon printer */
+              device->device_name = g_strdup (_("LPD Printer"));

Is there any reason why the "LPD" name needs to be in the user visible name?

@@ +794,3 @@
+  g_simple_async_result_set_check_cancellable (res, cancellable);
+  g_simple_async_result_set_op_res_gpointer (res, data, (GDestroyNotify) gsd_data_free);
+  g_simple_async_result_run_in_thread (res, _pp_host_get_lpd_devices_thread, 0, cancellable);

We should start using GTask instead for those threads.

Review of attachment 280740 [details] [review]:

"Allow adding"

Looks fine otherwise.

::: panels/printers/pp-new-printer-dialog.c
@@ +1469,3 @@
+  else
+    {
+      if (error->domain != G_IO_ERROR ||

g_error_matches()

@@ +1524,3 @@
+  else
+    {
+      if (error->domain != G_IO_ERROR ||

Ditto.

Review of attachment 280741 [details] [review]:

::: panels/printers/pp-new-printer-dialog.c
@@ +702,3 @@
   g_strcanon (name, ALLOWED_CHARACTERS, '-');
 
+  /* Remove common strings found in driver names */

I'd really like to have self-tests for those functions. See test-hostname in the shell/ sub-directory for example.

This would make it easier to avoid crashes or regressions.

Created attachment 281288 [details] [review]
Separate canonicalization of device name

Hi,

thank you for the reviews.


(In reply to comment #8)
> Review of attachment 280735 [details] [review]:
> 
> Looks good otherwise.
> 
> ::: panels/printers/pp-new-printer-dialog.c
> @@ +102,3 @@
>  static void     t_device_free (gpointer data);
>  static TDevice *t_device_copy (TDevice *device);
> +static gchar   *canonize_device_name (PpNewPrinterDialog *dialog,
> 
> canonicalize.

I've changed the name.

Created attachment 281289 [details] [review]
Don't request port during creation of PpHost

(In reply to comment #10)
> Review of attachment 280737 [details] [review]:
> 
> ::: panels/printers/pp-host.h
> @@ +35,3 @@
>  #define PP_HOST_GET_CLASS(o) (G_TYPE_INSTANCE_GET_CLASS ((o), PP_TYPE_HOST,
> PpHostClass))
> 
> +#define PP_HOST_DEFAULT_PORT      -1
> 
> I prefer UNSET_PORT

I've changed it to PP_HOST_UNSET_PORT.


> ::: panels/printers/pp-new-printer-dialog.c
> @@ +1639,3 @@
>          {
>            gchar *host = NULL;
> +          gint   port = PP_HOST_DEFAULT_PORT;
> 
> I'd rather you set that in parse_uri.

Done.

Created attachment 281292 [details] [review]
Make pp_devices_list_free() generally available

I've rewritten the code for detection of LPD and JetDirect printers using GTask. This is needed by the rewrite.

Created attachment 281293 [details] [review]
Allow to add AppSocket/HP JetDirect printers

(In reply to comment #11)
> Review of attachment 280738 [details] [review]:
> 
> Some explanation as to what "socket printers" are would be useful. To me, that
> knows what a socket is, a socket printer is a bit of a bizarre term.
> 
> Wikipedia or authoritative link would be useful in the code as well as in the
> commit message. Substitute for "JetDirect" if that's the only thing it
> supports.

I meant AppSocket and HP JetDirect printers here. I've added some links with an explanation to the code and to the commit message. I've also replaced "socket" by "jetdirect" in names of functions since AppSocket origins in it.


> ::: panels/printers/pp-host.h
> @@ +35,3 @@
>  #define PP_HOST_GET_CLASS(o) (G_TYPE_INSTANCE_GET_CLASS ((o), PP_TYPE_HOST,
> PpHostClass))
> 
> +#define PP_HOST_DEFAULT_PORT             -1
> 
> again UNSET_PORT

Done.

I've rewritten this code using GTask.

Created attachment 281294 [details] [review]
Add functions for searching for LPD printers

(In reply to comment #12)
> Review of attachment 280739 [details] [review]:
> 
> ::: panels/printers/pp-host.c
> @@ +656,3 @@
> +              else
> +                {
> +                  g_warning ("%s\n", error->message);
> 
> Won't that...
> 
> @@ +662,3 @@
> +          else
> +            {
> +              g_warning ("%s\n", error->message);
> 
> ...or that, print loads of warnings for some remote devices?

Yes, it could, I've removed the g_warning()s.


> @@ +702,3 @@
> +
> +  address = g_strdup_printf ("%s:%d", priv->hostname, port);
> +  if (address != NULL && address[0] != '/')
> 
> Instead of having one huge conditional that goes to the end of the function,
> flip the condition around and exit early.

Done.


> @@ +762,3 @@
> +                                                    found_queue);
> +              /* Translators: The found device is a Line Printer Daemon
> printer */
> +              device->device_name = g_strdup (_("LPD Printer"));
> 
> Is there any reason why the "LPD" name needs to be in the user visible name?

This helps user to distinguish between different protocols used for a printer on the same host.


> @@ +794,3 @@
> +  g_simple_async_result_set_check_cancellable (res, cancellable);
> +  g_simple_async_result_set_op_res_gpointer (res, data, (GDestroyNotify)
> gsd_data_free);
> +  g_simple_async_result_run_in_thread (res, _pp_host_get_lpd_devices_thread,
> 0, cancellable);
> 
> We should start using GTask instead for those threads.

I've rewritten this using GTask.

Created attachment 281296 [details] [review]
Allow adding of JetDirect and LPD printers

(In reply to comment #13)
> Review of attachment 280740 [details] [review]:
> 
> "Allow adding"
> 
> Looks fine otherwise.

Done.


> ::: panels/printers/pp-new-printer-dialog.c
> @@ +1469,3 @@
> +  else
> +    {
> +      if (error->domain != G_IO_ERROR ||
> 
> g_error_matches()

Done.


> @@ +1524,3 @@
> +  else
> +    {
> +      if (error->domain != G_IO_ERROR ||
> 
> Ditto.

Done.

Review of attachment 281288 [details] [review]:

We'd also need a test case for this new function. The code isn't easy to follow, and regressions and misbehaviour looks likely.

Review of attachment 281289 [details] [review]:

Looks good.

Review of attachment 281292 [details] [review]:

Looks fine.

Review of attachment 281293 [details] [review]:

Looks good.

::: panels/printers/pp-host.c
@@ +516,3 @@
+}
+
+/*

You can remove the empty comment lines at the top and at the bottom of this comment block.

Review of attachment 281294 [details] [review]:

::: panels/printers/pp-host.c
@@ +613,3 @@
+          input = g_io_stream_get_input_stream (G_IO_STREAM (connection));
+
+          length = g_snprintf (buffer, BUFFER_LENGTH, "\2%s\n", queue_name);

Can you explain where this comes from?
Not sure what "\2" is supposed to do there.

Review of attachment 281296 [details] [review]:

Looks good.

(In reply to comment #21)
> Review of attachment 281288 [details] [review]:
> 
> We'd also need a test case for this new function. The code isn't easy to
> follow, and regressions and misbehaviour looks likely.

The function is just a move of a code which changed just a little since 3.6 to its own function. Moving it to a separate file (or pp-utils.*) would mean to export TDevice structure and pass several private members of PpNewPrinterDialog to it.

I can add comments to individual steps of the function but basically it gathers possible candidates for the name of the new printer (being it make-and-model, original name of the queue on the remote server or just device info), removes leading and trailing whitespaces and replace disallowed characters by '-'. After that it just looks whether the name has been used by an already installed printer or a printer detected by the dialog before this one. If it was already used then it appends "-%d" with adequate index.

(In reply to comment #27)
> (In reply to comment #21)
> > Review of attachment 281288 [details] [review] [details]:
> > 
> > We'd also need a test case for this new function. The code isn't easy to
> > follow, and regressions and misbehaviour looks likely.
> 
> The function is just a move of a code which changed just a little since 3.6 to
> its own function. Moving it to a separate file (or pp-utils.*) would mean to
> export TDevice structure and pass several private members of PpNewPrinterDialog
> to it.
> 
> I can add comments to individual steps of the function but basically it gathers
> possible candidates for the name of the new printer (being it make-and-model,
> original name of the queue on the remote server or just device info), removes
> leading and trailing whitespaces and replace disallowed characters by '-'.
> After that it just looks whether the name has been used by an already installed
> printer or a printer detected by the dialog before this one. If it was already
> used then it appends "-%d" with adequate index.

I'd still like it commented, and a test created for it.

Created attachment 282050 [details] [review]
Merge TDevice into PpPrintDevice

This will allow us to create a test for canonicalize_device_name() later.

Created attachment 282051 [details] [review]
Separate canonicalization of device name

(In reply to comment #21)
> Review of attachment 281288 [details] [review]:
> 
> We'd also need a test case for this new function. The code isn't easy to
> follow, and regressions and misbehaviour looks likely.

I've moved the function to pp-utils.c|h. The test case is in the last patch.

Created attachment 282052 [details] [review]
Export name of selected PPD from PPD dialog (updated version)

Created attachment 282053 [details] [review]
Don't request port during creation of PpHost (updated version)

Created attachment 282054 [details] [review]
Make pp_devices_list_free() generally available (updated version)

Created attachment 282055 [details] [review]
Allow to add AppSocket/HP JetDirect printers (updated version)

Created attachment 282061 [details] [review]
Add functions for searching for LPD printers

(In reply to comment #25)
> Review of attachment 281294 [details] [review]:
> 
> ::: panels/printers/pp-host.c
> @@ +613,3 @@
> +          input = g_io_stream_get_input_stream (G_IO_STREAM (connection));
> +
> +          length = g_snprintf (buffer, BUFFER_LENGTH, "\2%s\n", queue_name);
> 
> Can you explain where this comes from?
> Not sure what "\2" is supposed to do there.

This is a command which sends job to the LPD queue which in turn returns status of this. If the returned value is 0 then it accepted the job. But I forgot to abort the job, the attached aborts it.
I've added a comment which points to the section 5.2 in RFC 1179 above the line.
I took this from system-config-printer (probe_printer.py - probe_queue()).

Created attachment 282062 [details] [review]
Allow adding of JetDirect and LPD printers (updated version)

Created attachment 282063 [details] [review]
Add function shift_string_left() and its test

This patch adds function shift_string_left() and its test. We will need the function in next patch.

Created attachment 282065 [details] [review]
Strip redundant strings from found devices (updated version)

(In reply to comment #14)
> Review of attachment 280741 [details] [review]:
> 
> ::: panels/printers/pp-new-printer-dialog.c
> @@ +702,3 @@
>    g_strcanon (name, ALLOWED_CHARACTERS, '-');
> 
> +  /* Remove common strings found in driver names */
> 
> I'd really like to have self-tests for those functions. See test-hostname in
> the shell/ sub-directory for example.
> 
> This would make it easier to avoid crashes or regressions.

The test is present in the next patch.

Created attachment 282066 [details] [review]
Add test for canonicalize_device_name()

This patch adds the requested test for canonicalize_device_name().

Review of attachment 282050 [details] [review]:

Looks good.

Review of attachment 282051 [details] [review]:

Alright.

Review of attachment 282061 [details] [review]:

Looks good otherwise.

::: panels/printers/pp-host.c
@@ +611,3 @@
+          input = g_io_stream_get_input_stream (G_IO_STREAM (connection));
+
+          /* RFC 1179, section 5.2 */

A little bit more verbose would be nice, such as "The protocol is explained in ..."

Review of attachment 282063 [details] [review]:

Looks good.

Review of attachment 282065 [details] [review]:

Looks fine.

Review of attachment 282066 [details] [review]:

Looks good!

Thank you for all the reviews, I appreciate it. 


(In reply to comment #42)
> Review of attachment 282061 [details] [review]:
> 
> Looks good otherwise.
> 
> ::: panels/printers/pp-host.c
> @@ +611,3 @@
> +          input = g_io_stream_get_input_stream (G_IO_STREAM (connection));
> +
> +          /* RFC 1179, section 5.2 */
> 
> A little bit more verbose would be nice, such as "The protocol is explained in
> ..."

I've modified the comment. It is little more verbose now.

Regards

Marek

This bug should be fixed in current master now. Please reopen, if it is not.

Regards

Marek