Re: [PATCH] cmd: set: Introduce tree-print set sub-command

[Jose E. Marchesi]

Hi Carlo!

    One typical (noob-ish) use for GNU poke is just to parse the content of
    a binary file for visual inspection after having defined the binary
    structure in a pk file.

    Unfortunately the usual output from something like '$<data> @ 0#B'
    isn't really user friendly and it is massively difficult to make sense
    of the un-spaced wall of text in output.

Agreed :)
    
    This patch wants to be an attempt to prettify a bit the displayed data
    introducing a new set command to display the output data in a tree-fied
    form (deep or shallow).

This is very nice stuff!  See a few notes below.

    For example:
    
    | (poke) .set tree-print shallow
    | ...
    | St_Key_Hdr {
    |    dw_magic=0x0U,
    |    n_total_size=0x0U,
    |    by_version=0x0UB,
    |    by_key_type=0x0UB,
    |    by_head_size=0x0UB,
    |    sz_pad=0x0UB,
    |    rsv=0x0U,
    |    item_hdr=St_Item_Hdr
    |  }

What about:

 item_hdr=St_Item_Hdr {...}

That makes it more explicit that St_Item_Hdr is a struct, and also makes
it easier to glimpse that it can be further expanded.  WDYT?

    | (poke) .set tree-print deep
    | ...
    | St_Key_Hdr {
    |    dw_magic=0x0U,
    |    n_total_size=0x0U,
    |    by_version=0x0UB,
    |    by_key_type=0x0UB,
    |    by_head_size=0x0UB,
    |    sz_pad=0x0UB,
    |    rsv=0x0U,
    |    item_hdr=St_Item_Hdr {
    |      sz_pad_1=[0x0UB,0x0UB,0x0UB,0x0UB],
    |      sz_pad_2=[0x0UB,0x0UB,0x0UB,0x0UB],
    |      n_raw_offset=0x0UH#B,
    |      n_raw_size=0x0UH,
    |      sz_pad_3=[0x0UB,0x0UB,0x0UB,0x0UB]
    |    }
    |  }

It would be nice to be able to define the maximum deep level, when
tree-print is "deep".  Something like:

(poke) .set tree-print-deep-level 3

Where the default coul be 0, meaning "no limit".

Also, it would be nice to be able to define the indentation step, in
number of spaces, like:

(poke) .set tree-print-indent 3

Where the default could be 3, like in your examples (which by the way
look very nice :)).

WDYT about adding these settings?
     
    +static int
    +pk_cmd_set_tree_print (int argc, struct pk_cmd_arg argv[], uint64_t uflags)
    +{
    +  /* set tree-print {no,deep,shallow}  */
    +
    +  const char *arg;
    +
    +  if (argc != 1)
    +    assert (0);

Why not assert (argc == 1)?

    +
    +  arg = PK_CMD_ARG_STR (argv[0]);
    +
    +  if (*arg == '\0')
    +    {
    +      enum pk_tree_mode tree_mode = pvm_tree_print (poke_vm);
    +
    +      switch (tree_mode)
    +        {
    +        case PK_TREE_NO:      pk_puts ("no\n"); break;
    +        case PK_TREE_DEEP:    pk_puts ("deep\n"); break;
    +        case PK_TREE_SHALLOW: pk_puts ("shallow\n"); break;

PK_TREE_* seems to be very generic, and we may need it for something
else.  What about using, generally, another prefix such as PK_TREEP_*
for everything related to the tree printing?

    +        default:
    +          assert (0);
    +        }
    +    }
    +  else
    +    {
    +      enum pk_tree_mode tree_mode;
    +
    +      if (STREQ (arg, "no"))
    +        tree_mode = PK_TREE_NO;
    +      else if (STREQ (arg, "deep"))
    +        tree_mode = PK_TREE_DEEP;
    +      else if (STREQ (arg, "shallow"))
    +        tree_mode = PK_TREE_SHALLOW;
    +      else
    +        {
    +          pk_term_class ("error");
    +          pk_puts ("error: ");
    +          pk_term_end_class ("error");
    +          pk_puts (" tree_mode should be one of `no' or `deep' or 
`shallow'.\n");


I know I often forget to do so myself, but please use gettext to
internationalize strings for the user:

pk_puts (_(" tree_mode should be one of `no' or `deep' or `shallow'.\n"));

    diff --git a/src/pk-term.h b/src/pk-term.h
    index 80827a0b..8a20adcc 100644
    --- a/src/pk-term.h
    +++ b/src/pk-term.h
    @@ -23,6 +23,13 @@
     
     #include <textstyle.h>
     
    +enum pk_tree_mode
    +  {
    +    PK_TREE_NO,
    +    PK_TREE_DEEP,
    +    PK_TREE_SHALLOW
    +  };
    +

Hm, I'm not sure this enumeration belongs to src/pk-term.h.  It is
the pvm subsystem that uses this, not services from pk-term.  What about
moving the definition there?

     /* Initialize and finalize the terminal subsystem.  */
     void pk_term_init (int argc, char *argv[]);
     void pk_term_shutdown (void);
    @@ -40,6 +47,9 @@ extern void pk_puts (const char *str);
     /* Print a formatted string to the terminal.  */
     extern void pk_printf (const char *format, ...);
     
    +/* Print a formatted string as indentation.  */
    +extern void pk_printf_indent (unsigned int lvl);

I find the comment confusing.  This function is supposed to change the
indentation level of the output emitted by both pk_printf and pk_puts,
right?  Why including `printf' in the name?  Woulnd't it be better to
call it something like pk_term_indent?

Also, we are gonna need a little update for the manual.

It looks very good! :)