Thread: proposal (9.5) : psql unicode border line styles

On 28.6.2014 21:29, Pavel Stehule wrote:
> Hello
> 
> rebase for 9.5
> 
> test:
> \pset linestyle unicode \pset border 2
> \pset unicode_header_linestyle double
> 
> \l
> 
> Regards
> 
> Pavel

I did a quick review of the patch today:

* it applies cleanly to current HEAD (no failures, small offsets)
* compiles and generally seems to work just fine

Two questions:

(1) Shouldn't the new options be listed in '\?' (as possible names for
"pset")? I mean, here:
 \pset [NAME [VALUE]]     set table output option                        (NAME :=
{format|border|expanded|fieldsep|fieldsep_zero|footer|null|
numericlocale|recordsep|recordsep_zero|tuples_only|title|tableattr|pager})


(2) I noticed this piece of code:

+typedef enum unicode_linestyle
+{
+    UNICODE_LINESTYLE_SINGLE = 0, /* to make sure someone initializes this */
+    UNICODE_LINESTYLE_DOUBLE = 1
+} unicode_linestyle;

Why are the values defined explicitly? These values are set by the
compiled automatically, so why set them manually? Only a few of the
other enums are defined explicitly, and most of them have to do that to
define different values (e.g. 0x01, 0x02, 0x04, ...).

I don't understand how the comment "to make sure someone initializes
this" explains the purpose?


regards
Tomas

On 23 Červenec 2014, 7:36, Pavel Stehule wrote:
>
> updated version is in attachment

OK, thanks. The new version seems OK to me.

Tomas

Pavel,

* Pavel Stehule (pavel.stehule@gmail.com) wrote:
> 2014-07-23 8:38 GMT+02:00 Tomas Vondra <tv@fuzzy.cz>:
> > OK, thanks. The new version seems OK to me.
>
> Thank you

I've started looking over the patch and went back through the previous
thread about it.  For my part, I'm in favor of adding this capability,
but I'm not terribly happy about how it was done.  In particular,
get_line_style() seems pretty badly hacked around, and I don't really
like having the prepare_unicode_format call underneath it allocating
memory and then passing back up the need to free that memory via a new
field in the structure.  Also, on a quick glance, are you sure that the
new 'unicode' output matches the same as the old 'unicode' did (with
pg_utf8format)?

I would think we'd simply set up a structure which is updated when the
linestyle is changed, which is surely going to be much less frequently
than the request for which linestyle to use happens, and handle all of
the line styles in more-or-less the same way rather than doing something
completely different for unicode than for the others.
Thanks,
    Stephen

Pavel,

* Pavel Stehule (pavel.stehule@gmail.com) wrote:
> I removed dynamic allocation  and reduced patch size.

This is certainly better, imv, though there are a couple of minor
issues (extra semi-colons, extraneous whitespace, get_line_style was
still changed to non-const, even though it doesn't need to be now).

> What I tested a old unicode style is same as new unicode style. There
> nothing was changed .. some fields are specified in refresh_utf8format
> function

I don't particularly like this (having these fields set in
refresh_utf8format to hard-coded strings in the function), why not have
those handled the same as the rest, where the strings themselves are in
the unicode_style structure?

The rest looks pretty good.  Need to step out for a bit but I'll look at
making the above changes when I get back if I don't hear anything.
Thanks,
    Stephen

* Pavel Stehule (pavel.stehule@gmail.com) wrote:
> 2014-09-11 16:42 GMT+02:00 Stephen Frost <sfrost@snowman.net>:
> > I don't particularly like this (having these fields set in
> > refresh_utf8format to hard-coded strings in the function), why not have
> > those handled the same as the rest, where the strings themselves are in
> > the unicode_style structure?
> >
>
> I am not sure if I understand well.
>
> With refresh_utf8format I can do shortly 6 possible combinations - or more
> (when it will be requested)
>
> I have no idea how to write as rest without repeating all 6 combinations -
> what was one noticed issue of some older variant, where I designed
> unicode1, unicode2, ...
>
> Any idea, tip how to it?

All I was suggesting was pulling these strings out of the function:

+   /* ↵ */
+   popt->header_nl_right = "\342\206\265";
+
+   popt->nl_left = " ";
+
+   /* ↵ */
+   popt->nl_right = "\342\206\265";
+
+   /* … */
+   popt->wrap_left = "\342\200\246";
+   popt->wrap_right = "\342\200\246";

and adding them to unicode_style and then referencing them there,
similar to how the rest of printTextFormat popt (by the way- don't
really like that variable name, particularly as it's used elsewhere with
a very different meaning..  why not 'format' or 'ptf'?) is
set up in refresh_utf8format, that's all.
Thanks,
    Stephen

Pavel,

* Pavel Stehule (pavel.stehule@gmail.com) wrote:
> Any idea, tip how to it?

Attached is what I had been thinking.

Thoughts?

    Thanks!

        Stephen

* Pavel Stehule (pavel.stehule@gmail.com) wrote:
> I though about it, and we cannot to drop it now. These symbols are
> necessary, because we don't support line between rows.
>
> I am thinking so 05 patch should be final

Ok.  I'm going to play with it a bit more but barring issues, should get
it committed today.
Thanks!
    Stephen

* Stephen Frost (sfrost@snowman.net) wrote:
> * Pavel Stehule (pavel.stehule@gmail.com) wrote:
> > I though about it, and we cannot to drop it now. These symbols are
> > necessary, because we don't support line between rows.
> >
> > I am thinking so 05 patch should be final
>
> Ok.  I'm going to play with it a bit more but barring issues, should get
> it committed today.

Alright, pushed with some additional cleanup, and also cleaned up the
border documentation a bit.
Thanks!
    Stephen

On 2014-09-18 12:47:25 +0200, Pavel Stehule wrote:
> -    output = PageOutput(81, pager);
> +    output = PageOutput(87, pager);
>  
>      fprintf(output, _("List of specially treated variables.\n"));
>  
> @@ -364,6 +364,10 @@ helpVariables(unsigned short int pager)
>                       "                     column width of left aligned data type in latex format\n"));
>      fprintf(output, _("  title              set the table title for any subsequently printed tables\n"));
>      fprintf(output, _("  tuples_only        if set, only actual table data is shown\n"));
> +    fprintf(output, _("  unicode_border_linestyle\n"));
> +    fprintf(output, _("  unicode_column_linestyle\n"));
> +    fprintf(output, _("  unicode_header_linestyle\n"
> +                     "                     set the style of unicode line drawing [single, double]\n"));
>  
>      fprintf(output, _("\nEnvironment variables:\n"));
>      fprintf(output, _("Usage:\n"));

Either the current line count is wrong, or you added the wrong number of
new lines to PageOutput(). Your patch only adds four \n, while you
increased from 81 to 87.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

Pavel,

* Pavel Stehule (pavel.stehule@gmail.com) wrote:
> true, sorry, I have a different wording in first design
>
> fixed

Pushed, thanks!
Stephen