(If you like the idea, I believe porting the simple solid thin/thick lines to this 5x5 matrix approach would further simplify the code. The desired exact sizes of the 5x5 rectangles might differ for those and for the hollow characters, but the rest would be the same.)

Created attachment 256625 [details]
text description of these glyps

I don't have code yet that implements the feature, but I've done the boring bits: entered the 5x5 matrices of these glyps.

Created attachment 256626 [details]
script to generate c code from the glyps text file

Created attachment 256638 [details] [review]
Fix

I attach a fix. Please take a look and tell me if you like it or not.

One of the things I didn't realize with 5x5 matrix approach that it requires the width of the hollow to be the same as the line width of a normal single line. It could be made to any value that is either hard-codedly less-than-or-equal-to, or hard-codedly greater-than-or-equal-to, using a 7x7 matrix, but I don't think it's worth it.

In order to address this new shortcoming, I had to decrease the line width a bit. But I personally think they look better this way :) And I think all the fixes we get outweigh this shortcoming.

Created attachment 256773 [details] [review]
Fix v2

Extended boxes.txt to contain every character in the 2500-257f range. Added a reference to this file as a source code comment. Other than this, the actual source code is the same as previously.

Please let me know if you like the generic approach, but don't like that I made the single lines thinner. I have a cleaner idea to address this, rather than the one in comment 4.

Basically, first we'd compute 4 values: {light,heavy}_line_width for solid lines, and their counterparts for hollow lines (outer width, and hollow width). Then, initializing xboundaries and yboundaries would be separately if-ed based on whether the character is hollow vertically, and whether it's hollow horizontally. It's a simple module 3 calculation, with the first hollow character being the only exception.

(Although I personally prefer the thinner lines that my patch introduces, and I also like that double lines looks just like to light lines next to each other. It looks nicer to me in applications that mix the two, e.g. "mc --skin double-lines".)

Hmm. The approach looks alright, but I'd like a few changes :-)

Let's keep the characters one-by-one in the switch(), and make the drawing routine either a macro or a static inline. That way the compiler can even eliminate the branches since it knows at compile time the bitmap values.

And I don't like how the data is presented in the source code. The hex values generated by the script from the data file aren't really nice to edit / check. 

So instead I'd suggest sth. like this:

case 0x250c: /* box drawings light down and right */
  draw_graphic_character (cr, 0x250c, x, y, light_line_width,
                          GRAPHIC (0, 0, 0, 0, 0,
                                   0, 0, 0, 0, 0,
                                   0, 0, 1, 1, 1,
                                   0, 0, 1, 0, 0,
                                   0, 0, 1, 0, 0));
  break;

where the GRAPHIC macro just assembles the 0/1 values into a guint.

Storing hex values: I decided on this after realizing that 0b... is a gcc extension. To edit/check the values, all it takes is a hex<->bin conversion, or to check the source data file and the generator script in the linked bugreport (i.e. this one). (If you only want to check, you'd probably verify the runtime behavior by simply cat'ing boxes.txt vte.) A strong factor in this design decision was that the Unicode standard is not going to change, so most likely after an initial verification this data will never ever have to be touched by anyone. It's also clear that there's no way someone could hide malicious code there or make vte crash, the worst you can do by altering those numbers is to have faulty glyphs.

Your approach of inlining the matrix gives 8 lines for a single character definition, this would be ~1000 lines of source code in total, plus an ugly definition of GRAPHIC. I even find the current code too long, I wouldn't be comfortable with anything longer than that or anything that's so highly repetative.

To give gcc a chance of optimizing away some if's, the definition of draw_graphic_character couldn't be a 5x5 loop, it would have to be 25 independent if's for each bit of graphic, again resulting in a long repetative definition for that.

I see you're trying to optimize for speed, sacrificing both source code compactness and memory footprint. I don't think speed is an issue (fwiw, neither is memory footprint), these characters are most often drawn by interactive applications. If speed is an issue, probaby the bitmap approach should be ditched and all these characters should be defined as the union of a few rectangles (along this 5x5 matrix), maybe allowing blackout as well, which is a huge step towards the current design - not that I'm necessarily against it, but defining (and verifying) those rectangles wouldn't be as straightforward as the bitmaps.

Please note that I won't have much time to work on vte in the future, I'd like to restrict it to the necessary minimum (e.g. followup fix if a patch of mine is buggy, or something like that). If you really convince me why your proposed changes would make it significantly better then I can do them, but right now I don't see it. If it's just that your personal taste is different from mine, feel free to rework my patch, or to throw it away completely and fix the bug the way you'd fix it, I won't mind, I just suggested the solution I personally find the cleanest :)

Created attachment 265616 [details] [review]
v3, with compile time code generator

ChPe, how about this one?

Or we can include the box_drawing.h (the one which is generated now and is kinda human readable) and avoid generating code at compile time?

Created attachment 265617 [details] [review]
v3, with compile time code generator

(forgot to git add the new files)

Created attachment 265801 [details] [review]
v4 (updated to git head)

Review of attachment 265801 [details] [review]:

::: src/Makefile.am
@@ +124,3 @@
+
+box_drawing.h: box_drawing.txt
+	./box_drawing_generate.sh < $< > $@

$(AM_V_GEN) $(srcdir)/box_drawing_....

::: src/box_drawing_generate.sh
@@ +20,3 @@
+/* Definition of most of the glyphs in the 2500..257F range as 5x5 bitmaps
+   (bits 24..0 in the obvious order), see bug 709556 and ../doc/boxes.txt */
+static gint vte_box_drawing_bitmaps[128] = {

static const guint32 const vte_box_drawing_bitmaps[128] = {...

::: src/vte.c
@@ +9608,3 @@
 
+	if (c >= 0x2550 && c <= 0x256c) {
+		heavy_line_width = 3 * light_line_width;

Should move that down into the case: for 0x2550..0x256c.

@@ +9610,3 @@
+		heavy_line_width = 3 * light_line_width;
+	} else {
+		heavy_line_width = light_line_width + 2;

Doesn't this differing heavy_line_width make the two versions incompatible ?

@@ +9731,3 @@
+        case 0x257f: /* box drawings heavy up and light down */
+	{
+		gint bitmap = vte_box_drawing_bitmaps[c - 0x2500];

const guint32 bitmap...

Review of attachment 265801 [details] [review]:

::: src/box_drawing_generate.sh
@@ +1,2 @@
+#!/bin/bash
+

Missing copyright & licence (LGPL2+) header :-)

::: src/vte.c
@@ +9604,3 @@
         })
 
+        light_line_width = (terminal->char_width + 1) / 5;

Why?

Review of attachment 265801 [details] [review]:

The main quesion is are you okay with the current overall design? And is it any better than just including the generated box_drawing.c file? (After all, the glyphs are kinda visually verifiable in that file too.)

::: src/box_drawing_generate.sh
@@ +20,3 @@
+/* Definition of most of the glyphs in the 2500..257F range as 5x5 bitmaps
+   (bits 24..0 in the obvious order), see bug 709556 and ../doc/boxes.txt */
+static gint vte_box_drawing_bitmaps[128] = {

Does the second "const" make any sense?

::: src/vte.c
@@ +9604,3 @@
         })
 
+        light_line_width = (terminal->char_width + 1) / 5;

See commment #4.

@@ +9610,3 @@
+		heavy_line_width = 3 * light_line_width;
+	} else {
+		heavy_line_width = light_line_width + 2;

It makes the double lines and the thick lines not compatible. Double lines and single lines are expected to be used together (there are glyphs containing both) and they look good if the both of the lines as well as the gap of the double lines has the same width as a thin line. I dont expect thick lines and double lines being used together (there are no glyphs where these two lines meet), and aligning them would require the thick line to be 3x as wide as a thin, which is IMO way too thick.

(In reply to comment #14)
> Review of attachment 265801 [details] [review]:
> 
> The main quesion is are you okay with the current overall design? 

It's ok.

> And is it any
> better than just including the generated box_drawing.c file? (After all, the
> glyphs are kinda visually verifiable in that file too.)

Licence requirement. The txt file clearly is the source in the preferred form for modification :-)

> ::: src/box_drawing_generate.sh
> @@ +20,3 @@
> +/* Definition of most of the glyphs in the 2500..257F range as 5x5 bitmaps
> +   (bits 24..0 in the obvious order), see bug 709556 and ../doc/boxes.txt */
> +static gint vte_box_drawing_bitmaps[128] = {
> 
> Does the second "const" make any sense?

Hmm I guess that one was one too many :-)

> ::: src/vte.c
> @@ +9604,3 @@
>          })
> 
> +        light_line_width = (terminal->char_width + 1) / 5;
> 
> See commment #4.

I don't get it. With this change it's simply doing a different rounding (sometimes down, sometimes up) instead of always up? (Note that light_line_width is an int, not a double.)
 
> @@ +9610,3 @@
> +        heavy_line_width = 3 * light_line_width;
> +    } else {
> +        heavy_line_width = light_line_width + 2;
> 
> It makes the double lines and the thick lines not compatible. Double lines and
> single lines are expected to be used together (there are glyphs containing
> both) and they look good if the both of the lines as well as the gap of the
> double lines has the same width as a thin line. I dont expect thick lines and
> double lines being used together (there are no glyphs where these two lines
> meet), and aligning them would require the thick line to be 3x as wide as a
> thin, which is IMO way too thick.

Hmm ok.

I more and more tend to think that we should just ship the currently auto-generated box_drawing.h file -- after all, this could have easily been the file I created manually at the first place (by copy-pasting the repetative bits, and manually entering the 0 and 1 digits). Removing the generator script would remove complexity from the make procedure and remove an ugly-as-hell shell script around a really insignificant part of the code.

Also defining the array in .h is not good, it can't be included from multiple .c files as it'd cause linking problems. I should move the definition to .c and have a declaration only in .h.

(Too all your review comments where I didn't respond: I'm okay with that and will fix.)

(In reply to comment #15)
> Licence requirement. The txt file clearly is the source in the preferred form
> for modification :-)

I don't get your concerns here. The file contains the looks of some Unicode glyphs. These glyphs are never going to change.

> > +        light_line_width = (terminal->char_width + 1) / 5;
> > 
> > See commment #4.
> 
> I don't get it. With this change it's simply doing a different rounding
> (sometimes down, sometimes up) instead of always up? (Note that
> light_line_width is an int, not a double.)

I don't understand this up/down rounding. I simply made a certain value a bit smaller (at least, that was my intent). So far e.g. with the 6x13 font I'm using the single line was 2px wide. So far thin and thick lines were integer wide, but double lines weren't, here ugly antialiasing kicked in (cf. division by 3 in the old version). With my approach, the double is is a single line + gap of a single line's width + another single line, making it necessarily 3x as wide in total than a single line. With 6x13 cells and 2px wide single lines this can't work anymore. Even with 8x15 (or so) cells I decided to remain 1px, because in midnight commander the two parallel double lines would be 4 parallel lines at equal distance and that still doesn't look good IMO. So I start a line width of 2px at 9px wide charcells only.

With my preferred 6x13 font size, vte drew ~1px wide line before we had built-in drawing for these glyphs. Then it became 2px which IMHO looked very ugly in mc. With my change it's a side effect that I get back the 1px line which I personally preferred, although I'm not doing this because this is my personal preference, but rather because it's the only way it can look nice with double line characters.

Okay, I got it, of course "(char_width + 4) / 5" means div by 5, rounding upwards. I explained my intent, and I guess simply "char_width / 5" would have a clearer semantics: rounding downwards. That way with 9px wide char cells you'd still get 1px lines, only at 10px cells would it grow to 2px. That way, with parallel double lines (e.g. "mc -S double-lines") the gap between characters would always be at least 2x as wide as the gap inside the characters.

Created attachment 271802 [details] [review]
v5: review comments addressed - complicated version

Created attachment 271803 [details] [review]
v5: review comments addressed - simple version

> +    if (c >= 0x2550 && c <= 0x256c) {
> +        heavy_line_width = 3 * light_line_width;
> 
> Should move that down into the case: for 0x2550..0x256c.

I can't see how this could be done, could you please show me how you mean it?

Other than this, I believe all your comments are addressed now.

I attached two versions, the "complicated" which generates the .c file from the .txt, and the "simple" which just includes the C code.

As I've mentioned, I'm firmly in favor of the simple version, it's just easier to maintain (should it ever need any maintainence which is unlikely because the glyphs will never change). The whole build procedure is simpler, we don't overcomplicate the code around this really insignificant issue. I've also made some manual cleanups to the file, e.g. replacing the glyphs that are skipped by a single 0 rather than a macro with 25 0s.

> Licence requirement. The txt file clearly is the source in the preferred form
> for modification :-)

The txt file is much more fragile. A small mistake (e.g. a glyph is defined by 4 lines instead of 5) and the script produces something that's totally faulty. Editing the C code provides much better guard against such mistakes.