Bug 23837

Created attachment 29378 [details] [review]
Make array-printing case more readable

Created attachment 29379 [details] [review]
Print byte arrays as nicely-formatted hex.

Created attachment 29380 [details] [review]
Print ASCIIfied version of byte arrays where possible

Comment on attachment 29378 [details] [review]
Make array-printing case more readable

Won't this just add a "," at the end of existing elements?  I don't see how we're removing a newline here.

Comment on attachment 29379 [details] [review]
Print byte arrays as nicely-formatted hex.


>+
>+  /* Each byte takes 3 cells (two hexits, and a space), except the last one. */
>+  columns = (80 - ((depth + 1) * INDENT)) / 3;

I suppose if you were feeling ambitious you could check isatty (1) and get the width, but...80 is probably fine.

>+      if (current_type != DBUS_TYPE_INVALID)
>+        {
>+          if (n == columns)
>+            {
>+              printf ("\n");
>+              indent (depth + 1);
>+              n = 0;
>+            }
>+          else
>+            {
>+              printf (" ");
>+            }
>+        }
>+    }
>+
>+  printf ("\n");

This will give a double newline if the number of bytes equals the number of columns, no?

Otherwise looks fine.

Comment on attachment 29380 [details] [review]
Print ASCIIfied version of byte arrays where possible

Might it be better to detect the case where it's entirely ASCII, and just print it as-is?

In the mixed case you can probably copy&paste it into some hex-viewing tool (assuming it has some way to take hex-print as input I guess)

(In reply to comment #4)
> (From update of attachment 29378 [details] [review])
> Won't this just add a "," at the end of existing elements?  I don't see how
> we're removing a newline here.

Ah, the commit message is misleading. This is not meant to change behaviour at all; it just made the code easier for me to read.

(In reply to comment #5)
> I suppose if you were feeling ambitious you could check isatty (1) and get the
> width, but...80 is probably fine.

I didn't want to have to cope with the terminal resizing while dbus-monitor is running, which is why I just hard-coded 80. :-)

> >+      if (current_type != DBUS_TYPE_INVALID)
> >+        {
> >+          if (n == columns)
> >+            {
> >+              printf ("\n");
> >+              indent (depth + 1);
> >+              n = 0;
> >+            }
> >+          else
> >+            {
> >+              printf (" ");
> >+            }
> >+        }
> >+    }
> >+
> >+  printf ("\n");
> 
> This will give a double newline if the number of bytes equals the number of
> columns, no?

I don't think so: this block does not run for the last byte. The iterator is advanced (updating current_type, that is) just before this block.

Suggestions for a less misleading name for the variable? Or maybe I should just document it better. I had to re-read it a few times to figure out how it works. :)

(In reply to comment #6)
> (From update of attachment 29380 [details] [review])
> Might it be better to detect the case where it's entirely ASCII, and just print
> it as-is?
> 
> In the mixed case you can probably copy&paste it into some hex-viewing tool
> (assuming it has some way to take hex-print as input I guess)

I agree; I'll change this. (I guess it's actually pretty rare to pass a mixed byte-array over D-Bus. The ay cases I see in practice are file paths (Telepathy uses ay for those because they're not necessarily UTF-8) and avatars.)

Created attachment 30880 [details] [review]
Make array-printing code easier to follow

Previously dbus_message_iter_get_arg_type() was called twice: once in
the loop condition to update 'current_type', and once to check if the
loop will run again. This patch moves updating current_type to the end
of the loop body.

Created attachment 30881 [details] [review]
Print byte arrays as nicely-formatted hex.

Created attachment 30882 [details] [review]
Print all-printable-ASCII byte arrays as strings

In practice, ay seems to be used mostly for binary data (in which case,
hex output is fine) or for Unix file paths (because they may be
non-UTF-8) and similar human-readable strings. So let's print the latter
similarly to strings.

I've updated the commit message for the second patch, and replaced the third patch by one which just prints out the ASCII if possible, and otherwise just prints the hex without the column of possibly-ascii on the RHS.

Comment on attachment 30880 [details] [review]
Make array-printing code easier to follow

Looks good.

Comment on attachment 30881 [details] [review]
Print byte arrays as nicely-formatted hex.


>+  printf("array of bytes [\n");
>+
>+  indent(depth + 1);

Space between identifier and paren.

>+  printf ("\n");
>+  indent(depth);
>+  printf("]\n");
> }

Ditto.

Otherwise looks good.

Created attachment 30963 [details] [review]
Print byte arrays as nicely-formatted hex.

An updated version of the second patch, adding the missing spaces.

I merged the branch with the whitespace errors fixed. Thanks for the review!