Use LineCanvas to draw TableView borders #2574
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -299,30 +299,31 @@ public override void OnDrawContent (Rect contentArea) | |
| bg = Style.BorderColor.Background; | ||
| } | ||
| Driver.SetAttribute (new Attribute(fg, bg)); | ||
|
|
||
| var lineWidth = width; | ||
|
|
||
| if (!Style.ExpandLastColumn && Style.AddEmptyColumn) { | ||
| lineWidth = contentArea.Width; | ||
| } | ||
| RenderCellLines (lineWidth, Table.Rows, columnsToRender); | ||
|
|
||
| foreach (var p in grid.GetMap (Bounds)) { | ||
| this.AddRune (p.Key.X, p.Key.Y, p.Value); | ||
| } | ||
|
|
||
| int hh = GetHeaderHeightIfAny (); | ||
|
|
||
| // render arrows | ||
| if (Style.ShowHorizontalScrollIndicators) { | ||
| if (hh > 0 && MoreColumnsToLeft ()) { | ||
| scrollLeftPoint = new Point (0, hh); | ||
| AddRuneAt (Driver, 0, scrollLeftPoint.Value.Y - 1, CM.Glyphs.LeftArrow); | ||
| } | ||
| if (hh > 0 && MoreColumnsToRight (columnsToRender)) { | ||
| scrollRightPoint = new Point (lineWidth - 1, hh); | ||
| AddRuneAt (Driver, scrollRightPoint.Value.X, scrollRightPoint.Value.Y - 1, CM.Glyphs.RightArrow); | ||
| } | ||
| } | ||
|
|
||
| // render the header contents | ||
| if (Style.ShowHeaders && hh > 0) { | ||
|
|
@@ -340,17 +341,15 @@ public override void OnDrawContent (Rect contentArea) | |
| var colStyle = Style.GetColumnStyleIfAny (current.Column); | ||
| var colName = table.ColumnNames [current.Column]; | ||
|
|
||
| if (!Style.ShowVerticalHeaderLines && current.X > 0) { | ||
| AddRune (current.X - 1, yh, (Rune)Style.HeaderSeparatorSymbol); | ||
| } | ||
|
|
||
| Move (current.X, yh); | ||
|
|
||
| if (current.Width - colName.Length > 0 | ||
Nutzzz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| && 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, Each short-circuits if Style.ShowHorizontalHeaderOverline is false, but an extra unary 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. |
||
|
|
||
| if (colName.Sum (c => ((Rune)c).GetColumns ()) < current.Width) { | ||
| Driver.AddStr (colName); | ||
|
|
@@ -361,20 +360,28 @@ public override void OnDrawContent (Rect contentArea) | |
| Driver.AddStr (TruncateOrPad (colName, colName, current.Width, colStyle, padChar)); | ||
| } | ||
|
|
||
| if (!Style.ShowVerticalHeaderLines | ||
| && current.Column == columnsToRender.First().Column + columnsToRender.Length - 1 | ||
| && current.X + current.Width - 1 <= lineWidth) { | ||
| AddRune (current.X + current.Width - 1, yh, (Rune)Style.HeaderSeparatorSymbol); | ||
| } | ||
|
|
||
| if (!Style.ExpandLastColumn) { | ||
| if (!Style.AddEmptyColumn) { | ||
| if (i == columnsToRender.Length - 1) { | ||
| for (int j = current.X + current.Width; j < Bounds.Width; j++) { | ||
| Driver.SetAttribute (GetNormalColor ()); | ||
| AddRune (j, yh, (Rune)Style.BackgroundSymbol); | ||
| if (Style.ShowHorizontalHeaderOverline) { | ||
| AddRune (j, yh - 1, (Rune)Style.BackgroundSymbol); | ||
| } | ||
| if (Style.ShowHorizontalHeaderUnderline) { | ||
| AddRune (j, yh + 1, (Rune)Style.BackgroundSymbol); | ||
| } | ||
| } | ||
| } | ||
| } else if (!Style.ShowVerticalHeaderLines) { | ||
| AddRune (Bounds.Width - 1, yh, (Rune)Style.HeaderSeparatorSymbol); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -394,11 +401,16 @@ public override void OnDrawContent (Rect contentArea) | |
|
|
||
| // No more data | ||
| if (rowToRender >= Table.Rows) { | ||
| if (rowToRender == Table.Rows | ||
| && Style.ShowHorizontalBottomline | ||
| && !Style.ExpandLastColumn | ||
| && !Style.AddEmptyColumn) { | ||
| var start = columnsToRender [^1]; | ||
| for (int i = start.X + start.Width; i < Bounds.Width; i++) { | ||
| AddRune (i, Table.Rows + hh, (Rune)Style.BackgroundSymbol); | ||
| } | ||
| } | ||
|
|
||
| continue; | ||
| } | ||
|
|
||
|
|
@@ -468,7 +480,16 @@ private void RenderCellLines (int width, int height, ColumnToRender [] columnsTo | |
| lineStyle = Style.OuterHeaderBorderStyle; | ||
| } | ||
| grid.AddLine (new Point (col.X - 1, 0), row, Orientation.Vertical, lineStyle); | ||
|
|
||
| // left side of empty column | ||
| if (col.Column == columnsToRender.First().Column + columnsToRender.Length - 1 | ||
| && !Style.ExpandLastColumn | ||
| && Style.AddEmptyColumn) { | ||
| grid.AddLine (new Point (col.X + col.Width - 1, 0), row, Orientation.Vertical, lineStyle); | ||
| } | ||
| } | ||
|
|
||
| // right side | ||
| grid.AddLine (new Point (width - 1, 0), row, Orientation.Vertical, Style.OuterHeaderBorderStyle); | ||
| } | ||
| } | ||
|
|
@@ -485,6 +506,13 @@ private void RenderCellLines (int width, int height, ColumnToRender [] columnsTo | |
| lineStyle = Style.OuterBorderStyle; | ||
| } | ||
| grid.AddLine (new Point (col.X - 1, row - 1), height - RowOffset + 1, Orientation.Vertical, lineStyle); | ||
|
|
||
| // left side of empty column | ||
| if (col.Column == columnsToRender.First().Column + columnsToRender.Length - 1 | ||
| && !Style.ExpandLastColumn | ||
| && Style.AddEmptyColumn) { | ||
| grid.AddLine (new Point (col.X + col.Width - 1, row - 1), height - RowOffset + 1, Orientation.Vertical, lineStyle); | ||
| } | ||
| } | ||
| grid.AddLine (new Point (width - 1, row - 1), height - RowOffset + 1, Orientation.Vertical, Style.OuterBorderStyle); | ||
| } | ||
|
|
@@ -596,9 +624,23 @@ private void RenderRow (int row, int rowToRender, ColumnToRender [] columnsToRen | |
| RenderCell (cellColor, render, isPrimaryCell); | ||
|
|
||
| // Style.AlwaysUseNormalColorForVerticalCellLines is no longer possible after switch to LineCanvas | ||
| // except when vertical cell lines are disabled (though a Style.SeparatorSymbol could be used) | ||
|
|
||
| if (!Style.ShowVerticalCellLines) { | ||
| if (isSelectedCell && FullRowSelect) { | ||
| color = focused ? rowScheme.Focus : rowScheme.HotNormal; | ||
| } else { | ||
| color = Enabled ? rowScheme.Normal : rowScheme.Disabled; | ||
| } | ||
| Driver.SetAttribute (color); | ||
|
|
||
| if (current.X > 0) { | ||
| AddRune (current.X - 1, row, (Rune)Style.SeparatorSymbol); | ||
| } | ||
| if (current.X + current.Width - 1 < Bounds.Width) { | ||
| AddRune (current.X + current.Width - 1, row, (Rune)Style.SeparatorSymbol); | ||
| } | ||
|
|
||
| if (isSelectedCell) { | ||
| color = focused ? rowScheme.Focus : rowScheme.HotNormal; | ||
| } else { | ||
|
|
@@ -607,10 +649,17 @@ private void RenderRow (int row, int rowToRender, ColumnToRender [] columnsToRen | |
| Driver.SetAttribute (color); | ||
| } | ||
|
|
||
| if (!Style.ExpandLastColumn) { | ||
| if (!Style.AddEmptyColumn) { | ||
| if (i == columnsToRender.Length - 1) { | ||
| for (int j = current.X + current.Width; j < Bounds.Width; j++) { | ||
| Driver.SetAttribute (GetNormalColor ()); | ||
| AddRune (j, row, (Rune)Style.BackgroundSymbol); | ||
| } | ||
| } | ||
| } else if (!Style.ShowVerticalCellLines) { | ||
| Driver.SetAttribute (Enabled ? rowScheme.Normal : rowScheme.Disabled); | ||
| AddRune (Bounds.Width - 1, row, (Rune)Style.SeparatorSymbol); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -631,16 +680,18 @@ protected virtual void RenderCell (Attribute cellColor, string render, bool isPr | |
| // If the cell is the selected col/row then draw the first rune in inverted colors | ||
| // this allows the user to track which cell is the active one during a multi cell | ||
| // selection or in full row select mode | ||
| if ((Style.InvertSelectedCell || Style.InvertSelectedCellFirstCharacter) && isPrimaryCell) { | ||
|
|
||
| if (render.Length > 0) { | ||
| // invert the color of the current cell for the first character | ||
| Driver.SetAttribute (new Attribute (cellColor.Background, cellColor.Foreground)); | ||
| Driver.AddRune ((Rune)render [0]); | ||
|
|
||
| if (render.Length > 1) { | ||
| if (!Style.InvertSelectedCell) { | ||
| Driver.SetAttribute (cellColor); | ||
| } | ||
| Driver.AddStr (render[1..]); | ||
| } | ||
| } | ||
| } else { | ||
|
|
@@ -1728,7 +1779,7 @@ private int CalculateMaxCellWidth (int col, int rowsToRender, ColumnStyle colSty | |
| /// <returns></returns> | ||
| private string GetRepresentation (object value, ColumnStyle colStyle) | ||
| { | ||
| if (value is null || value == DBNull.Value) { | ||
| return string.IsNullOrEmpty(Style.NullSymbol) ? " " : Style.NullSymbol; | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 asSingleSpace(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that if this is to be a constant it would make more sense as
DefaultHeaderSeparatorSymbolrather 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