Add support for distinct outer and inner unicode line styles

[gagern]

This is a follow-up to #207 and caters for one of the motivating examples of
#115, namely the example with double outer boundary but single inner lines.

This change does not allow all the customization options of that latter pull request at this stage, but instead reserves the actual representation of the style as an implementation detail which can evolve as needed, while only exposing few selected combinations to the caller.

[olekukonko]

Thank you so much for the contribution, I like this however I think we should seriously consider a theme approach to this will make the code

• more readable
• easy to customise
• people can now create crazy and different type of themes
• They can grow to have colour, align etc. associated to the theme directly

What do you think ?

[olekukonko]

am soon keen about backwards compatibility, fine it's less than 1 week since we introduced this function however is this not a breaking change?

[gagern]

This is not a breaking change, this is an existing test being adjusted somewhat to make it more useful.

In other words, the SetUnicodeHV function stays and retains its existing semantics. The SetUnicodeOuterInner function is a new addition. By using the latter and selecting different line styles for both we can see more clearly in these tests which of the characters use the boundary symbol and which use the inner symbol.

[olekukonko]

I was about to even work on these names soon, they are not clear enough and also lack comments

[gagern]

Added a comment locally, will be there after the next force push.

[olekukonko]

{row, column, center, center, center, center, center, center, center, center, center, row, column, center, center, center, center, center, center, center, center is actually giving me a headache ... it makes is difficult to understand and easy to introduce error for other contributors

Is there no way we can have a composition approach? or theme where the time is a struct will label items while the.build can now return the required slice or we use the theme structure directly

[gagern]

Composition would only be useful if we could have fallbacks. Which means having an interface and probably one function call per symbol. A struct with labels can work, see my other comment around the trade-offs of making this a struct.

[olekukonko]

t.syms[symESb] must we use map, can we not have a theme struct?

[gagern]

It's actually a slice, not a map. Here are my thoughts on the trade-offs:

1. Slice allows doing arithmetic on the offsets. So currently you can get from a non-boundary symbol to the corresponding boundary symbol by adding 11 to the symbol constant. I thought I would need that, but it turns out I don't.
2. There are a number of places in this change here where I compute the index in one place, and then look it up in a another. That would no longer work with a struct, so we would always have to directly resolve to the symbol itself.
3. If we add any form of character-changing API, that API might benefit from being able to refer to symbol roles as opposed to the current assignment for a given role. Maybe a pointer into a struct can serve that purpose, but it's less obvious.
4. The current approach for turning the concise string representation into the slice is a straight-forward loop. With a struct it's harder to get the mapping right.

[gagern]

Thank you so much for the contribution, I like this however I think we should seriously consider a theme approach to this

This could mean one of several things.

1. Switch internal code from []string slice to a type theme struct { symEW string; symNS string; … }. So we could use struct member names in a few more places, replacing slice indices, and would no longer be able to initialize the struct from a string in a loop.

2. Make a struct like above, but also include other styling settings in there. borders would be an obvious candidate, but many others might qualify as well. Then the question is what you want to include there.

3. Add a public API for finer granularity customization. As before, I don't think this is mature enough to open up too widely. If we had added public API in Add support for Unicode box drawing characters #207 for customizing the characters then this request here would not have been possible that easily.

will make the code

• more readable

In my opinion the readability of the code of this library currently suffers mainly from lack of abstraction and consistency. There are at least 4 different places drawing horizontal rows. Some pieces of code draw the verfical separator before the corresponding cell, some after. If we could make it so that the code first turns the data, headers and footers into some arrangement of cell and boundary objects, and then have a single place to emit these cells to the output, I believe things would become a lot more consistent. That's a major rewrite, though.

Short of such a rewrite, I don't see how a theme would make the code significantly more readable. I'm not entirely happy with my choice of names for the symbol index constants, but I don't see using a struct field notation instead of slice subscript notation as a fundamental difference for this.

• easy to customise

Internally or externally? Internally I don't see how anything would be easier than adding another string constant with the agreed number of characters. The fact that we can easily extend the number of characters as we identify new roles makes this very flexible. Externally, allowing too much customization will make maintaining compatibility hard, so I would argue for more time to make sure we really expose a useful API.

• people can now create crazy and different type of themes

By tweaking structs? I would argue against that. Have the users call functions. It's a lot easier to ensure backwards compatibility with those: you can populate any new internal data structure in a compatible way by updating the function implementation you control. So from that perspective I would suggest we try to identify the right set of functions to expose, and I would do that incrementally.

• They can grow to have colour, align etc. associated to the theme directly

Colour for boundaries is an interesting idea. I can't see more than one colour being used, since the crossings can only have a single colour each. At the moment, a single function like SetLineColor sounds best. Again I don't think putting this into a “theme” concept would help.

One thing I'm wondering is whether we might want

type Theme themeImpl
type themeImpl func(*Table) error

func (t *Table) SetTheme(theme Theme) error {
  theme(t)
}

func Combine(first, second Theme) Theme {
  return func(t *Table) error {
    if err := first(t); err != nil {
      return err
    }
    return second(t)
  }
}

now a Theme internally is anything that can tweak a table configuration, but externally it is some opaque type which can stand on its own, and you can have code that builds themes independent of the tables to which these themes get applied. If you are eager to get the idea of themes out in the wild, I would start with that and then take it from there.

[gagern]

I've added another commit to this PR here for polishing the lines for auto-merged cells. Having the test case for that in the file should serve as a guard against some overly simplistic theme implementation. If we can get a theme API in place that maintains that test case, I assume chances are it's good enough. Would suggest doing so in a later commit, though, outside this PR here and after some discussion of what the API should be in the first place.

[gagern]

Toying with the theme struct idea for purely internal use, and trying to get more descriptive names at the cost of brevity, I've come up with the following:

// Assuming boundary is double lines, and inner grid is single lines, then we
// expect this struct to contain
//
// lineStyle{
//   topBorder:    {"╔", "╒", "╤", "═", "╕", "╗"},
//   topInner:     {"╓", "┌", "┬", "─", "┐", "╖"},
//   crossing:     {"╟", "├", "┼", "─", "┤", "╢"},
//   vertical:     {"║", "│", "│", "?", "│", "║"},
//   bottomInner:  {"╙", "└", "┴", "─", "┘", "╜"},
//   bottomBorder: {"╚", "╘", "╧", "═", "╛", "╝"},
// }
//
// The vertical.horizontal entry (maked "?") is unused.
//
// Essentially the topBorder / bottomBorder rows are for the actual border,
// while the topInner / bottomInner are for cells which are adjacent to a
// multi-column cell and thus should not have protruding crossings.
// The crossing entry is for drawing inner horizontal lines while the vertical
// entry is for the vertical separators in rows which contain cell content.
//
// Consistency requires that
//   topInner.horizontal == crossing.horizontal == bottomInner.horizontal
// and
//   vertical.leftInner == vertical.crossing == vertical.rightInner
// because some lines may continue while others end in a merged cell, so that
// different crossings need to get connected with consistent lines between them.
// Conversely there is no requirement that
//   topBorder.horizontal == bottomBorder.horizontal
// or
//   vertical.leftBorder == vertical.rightBorder
// since border styles never mix so it is fine to have distinct symbols there.
type lineStyle struct {
	topBorder    horizontalLineStyle
	topInner     horizontalLineStyle
	crossing     horizontalLineStyle
	vertical     horizontalLineStyle
	bottomInner  horizontalLineStyle
	bottomBorder horizontalLineStyle
}
type horizontalLineStyle struct {
	leftBorder  string
	leftInner   string
	crossing    string
	horizontal  string
	rightInner  string
	rightBorder string
}

Would you consider that an improvement? Can you think of a good API that would guarantee the stated invariants in a reasonable way, without having 31 different functions for the 31 distinct symbols described above?

Note that the table above contains combinations (namely the corners between border and non-border cells, i.e. between single and double lines in the example) which are currently unused. So we have 6×6=36 symbols in that struct, 1 of them is unused as commented, 4 of them are implied by the stated constraints, 8 of them are unused in current or near future envisioned code, leaving 23 symbols which is 2 more than my current theme strings contain because my themes assume the two possible constraints which the comment above states need not necessarily hold.

Personally I'm not convinced the above is a win, and in any case I would treat switching to that as a separate commit. if you insist, I can make it part of this same PR here.

[olekukonko]

I think this is lovely, however, we must look at this more critically and ensure most use cases are covered, yes you are correct
I want

func (t *Table) SetTheme(theme Theme) error {
  theme(t)
}

I also think Rich(row []string, colours []Colors) was not well thought implementation because you should be able to add a theme. Also, you should be able to add a theme for a particular row or column which the current implementation does not support. eg. Just imagine a table where you can underline colour a column red or the bottom lines are colour red with or background colour to empathize certain information

I have been wanting to change this but am not sure how to do that without introducing breaking changes here is what I think is the best approach to gradually achieve that goal

nothing serious this is just an example

// Style styling information
// Character, Color, Background etc.. each character and anything that can be used to modify it
type Style struct {
}

// Item I tem is just a composite of Style
type Item struct {
	Outter Style
	Inner  Style
}

// Border - How the Border should be composed
type Border struct {
	Top    Item
	Bottom Item
	Left   Item
	Right  Item
}

// Divider Dividers - should also have their styles
type Divider struct {
	Crossing   Style
	Horizontal Style
}

// Theme This is the theme
type Theme struct {
	Border  Border
	Divider Divider
	//Something can be here in the feature
	// For example how do you introduce padding? 
}

This is what I love the most `discussion of what the API should be in the first place .. maybe we should look at that first then deprecate some of the functionality and introduce better implementation without introducing any breaking changes. Imaging adds a role where you can attach a callback function when during render etc.

[gagern]

Is this PR a step in the right direction? Can we get it merged and continue conversation elsewhere?

I think this is lovely

What is? My PR here, or the mock internal API from #218 (comment), or something I said in an earlier comment?

I want SetTheme(theme Theme) …

In this PR here, or can that come later? As it stands, this PR here doesn't have much theme-defining capabilities. We could define functions UnicodeInnerOuter and UnicodeHorizontalVertical to define themes. If you insist I can add that to this PR, but I would rather get this PR merged as is and continue conversation after that, instead of “polluting” the package namespace with new exported symbols in a rash manner.

I also think Rich(row []string, colours []Colors) was not well thought implementation because you should be able to add a theme.

Too bad Go doesn't have proper function overloading. Nor covariant slices, so even if you define Theme to be an interface which Colors implements, you still can't pass a []Colors slice as a []Theme slice without changes to the caller. The only thing you can do without breaking current API is making this Rich(row []string, colors []Colors, themes []Themes...) and then complain in some way if you get more than one slice of themes. But without an error return, that complaining is hard, too.

It might make sense to use accessor methods, where you can construct the table row, and then call methods to style the individual cells after they have been appended but before the table is rendered. But if cells can have different styles, then you will need an answer to what happens if two adjacent cells don't agree on the type of line between them.

here is what I think is the best approach to gradually achieve that goal

Did you read my #218 (comment) above? Citing myself:

By tweaking structs? I would argue against that. Have the users call functions. It's a lot easier to ensure backwards compatibility with those: you can populate any new internal data structure in a compatible way by updating the function implementation you control.

I still stand by that: you should not define any types with public data members. All the content of your themes should be in unexported members, and all access to that should be via sensibly defined functions.

nothing serious this is just an example

Your example has 1 character for Style (according to the comment), 2×1=2 for Item, 4×2=8 for Border, 2×1=2 for Divider and 8+2=10 for Theme. I don't see how these 10 characters map to the 23+ characters in my theme approach. In particular I don't see where all the corner symbols fit in.