Use LineCanvas to draw TableView borders #2574
Conversation
|
Should TableView.Border be responsible for the outer border? |
…l or empty string Also: * Add settable header/cell symbols to pad around values * Fix All/NoLines in TableEditor scenario to apply to HeaderThroughline
Style.AlwaysUseNormalColorForVerticalCellLines is obeyed now only if cell lines are disabled
This comment was marked as outdated.
This comment was marked as outdated.
This is a great question. My initial reaction is yes. If tableView.Border has a + thickness and !LineStyle.None then any grid lines should draw -1 past the left and top bounds and +1 past the right and bottom bounds (using tableView.LineCanvas). This will make the border flow naturally and automatically. However, there are edge cases
I need to think on this more. Would love your thoughts. |
You hit the nail on the head. I had the same thoughts, and the same worries. EDIT: Some additional concerns:
|
…zz/Terminal.Gui into tableview_linecanvasborders
|
@tig Yeah, doesn't look like that merge was sufficient. I'll update this to the latest v2_develop for what it might be worth... I'd like to play with some of the changes you guys have been doing in the meantime, in any case. |
Also: * Add BorderColor to TableStyle * Move symbols to TableStyle * Rename LineStyle to BorderStyle (at the risk of confusion with the View's border) * Comment out AlwaysUseNormalColorForVerticalCellLines feature
|
OK, now we're back to where this PR was in May. I did finish the BorderColor style that I never got around to checking in (though the UICatalog option should switch to the new Color Picker). However the remaining issues mentioned above haven't changed:
|
* TODO: Fix new unit tests * Add new AddEmptyColumn (when ExpandLastColumn is false) added as a new option (default true) * Add new InvertSelectedCell (the entire cell vs. existing option for first character only) * Add new HeaderSeparatorSymbol distinct from SeparatorSymbol for regular cells
|
Finally fixed 1 from the list in the previous comment, but I'm still stuck on the other two. I added some new color unit tests, but I guess I'm not entirely sure how these work, and they're failing. |
| /// The symbol to add after each cell value and header value to visually seperate values (if not using vertical gridlines) | ||
| /// The symbol to add after each header value to visually seperate values (if not using vertical gridlines) | ||
| /// </summary> | ||
| public char HeaderSeparatorSymbol { get; set; } = ' '; |
There was a problem hiding this comment.
Per our guidelines and just because it's a plain ol' good idea, how about defining constants for these instead of the char literal?
Fast way to do that would be to highlight the ' ' in one spot, press ctrl-alt-c, to invoke the resharper introduce constant refactoring, choose the option that says "replace x instances", which will cover any it thinks are relevant within the same scope, and then pick a name for the constant such as SingleSpace (or anything else that would make sense and also not be a potential conflict with any future strings or other types with a similar value/purpose).
And of course not necessarily in this PR. Just making note of a place for us to dogfood our guidelines.
There was a problem hiding this comment.
I think that if this is to be a constant it would make more sense as DefaultHeaderSeparatorSymbol rather than a global find and replace.
I think that approach would also work better with ConfigurationManager which has some precedent for changing defaults via settings files
| (!Style.ShowHorizontalHeaderOverline || !Style.ShowHorizontalHeaderUnderline)) { | ||
| if (current.Width - colName.Length > 0 | ||
| && Style.ShowHorizontalHeaderThroughline | ||
| && (!Style.ShowHorizontalHeaderOverline || !Style.ShowHorizontalHeaderUnderline)) { |
There was a problem hiding this comment.
Micro-optimization opportunity (also not in scope for this PR):
By DeMorgan's theorem, (!Style.ShowHorizontalHeaderOverline || !Style.ShowHorizontalHeaderUnderline) is equivalent to !(Style.ShowHorizontalHeaderOverline && Style.ShowHorizontalHeaderUnderline)
Each short-circuits if Style.ShowHorizontalHeaderOverline is false, but an extra unary ! is avoided with the && form if it is true, as there's only one in the latter case.
This is probably also applied by the compiler for optimized builds, though.
But a thought about that:
In places where debug and release builds may have different compiled output, it's a good idea to try to make them be the same.
There was a problem hiding this comment.
Haha, I don't need no stinkin' truth tables. I wasn't aware there was a difference in compiled output in this case. In fact, I guess I sort of always assumed that if there was, that a disjunction (with the potential for a "short-circuit") would be better, since (potentially) having to look up the additional value in memory would be much more expensive than doing the negation.
There was a problem hiding this comment.
Haha well as stated it is a micro-optimization and, in cases like that, my opinion is that clarity of intent is much more important than shaving off one operator method call in a subset of cases.
In this particular instance, they are identical except for the single situation I called out, so it's truly a super-micro-optimization.
But, for future reference, a lot of commentary I make, unless I put it in an actual change request review, are purely for visibility of opportunities, concepts, etc for discussion or for consideration for future work.
If I see an actual problem that I think should be addressed before merge, I'll be pretty clear about it and request a change in the review.
The only thing about the code in this PR that came close to that for me is that we strongly prefer switches (either expressions or statements as appropriate) for control flow logic, when possible, both for style and performance reasons.
But that's IMO not important enough, at this stage, to block a PR like this one. 😊
There was a problem hiding this comment.
Oh and thanks for working on this.
Do you need any assistance, with regard to #2574 (comment) or anything else for this?
There was a problem hiding this comment.
Thanks. Any assistance would be appreciated, and as I won't have time to work on this for the next few days there shouldn't be any duplication of effort.
There was a problem hiding this comment.
Done, but only by dint of allowing users to make the strange choice of using all three lines.
Also, regarding switch-case, I don't use them unless there are at least 3 options [I assume that's standard?]; here there are a few 2-option if-else's and 2-option if-else-if's without a default else.
Of course that's not to say that some cleanup couldn't be done; this still retains much of the way the original version was structured.
…erline even though it's a strange choice
For my purposes, I wanted to add a way to save one or two lines in the table column header while still setting off the header, so I added Style.ShowHorizontalHeaderThroughline that is displayed if either or both of the overline and underline are disabled (see image below). It got pretty ugly, but since I also wanted to play with my LineCanvas style additions with TableView, I took it on myself to switch TableView to LineCanvas. This simplifies things quite a bit, but I'm not sure if I've gone about this the wrong way. I also added styles to apply different LineStyles to different portions of the table (header outside/inside and regular cell outside/inside). Anyway, I thought I'd share FWIW.
Also, By design, Style.ShowHorizontalScrollIndicators are now displayed on whatever is the lowest header line.
Issues:
Pull Request checklist:
CTRL-K-Dto automatically reformat your files before committing.dotnet testbefore commit///style comments)