Skip to content
This repository has been archived by the owner on Sep 4, 2024. It is now read-only.

Fixes issue rendering when node is collapsed and text has new lines #794

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

netonjm
Copy link
Contributor

@netonjm netonjm commented Mar 6, 2018

Fixes issue rendering when node is collapsed and text has new lines
It happens when text doesn't exceed the maxwidth and we have
multiple lines because the new lines

@netonjm netonjm self-assigned this Mar 6, 2018
@netonjm netonjm requested review from slluis and sevoku March 6, 2018 19:05
@@ -48,7 +48,7 @@ public TreeViewEvents ()
tree.Columns.Add (col);

var node = store.AddNode ().SetValue (nodeField, new TextNode ("Root 1"));
var child = node.AddChild ().SetValue (nodeField, new TextNode ("Very long text. Very long text. Very long text. Very long text. Very long text. Very long text. Very long text. Very long text."));
var child = node.AddChild ().SetValue (nodeField, new TextNode ($"Very long text with NewLines. {Environment.NewLine} Very long text with NewLines.{Environment.NewLine} Very long text. Very long text. Very long text. Very long text."));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just use "\n", no need to embed a call to Environment.NewLine.

status.LastCalculatedHeight = textSize.Height;
//in cases when there are multiple lines and they don't execeed the max width we need to care height include the expand
double height = textSize.Height;
if (!status.Expanded && textSize.Height > defaultHeight) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified. If there are multiple lines, just measure the height of the first line.

public ExpandableTextCellView ()
{
var defaultLayout = new TextLayout() { Text = "A" };
defaultHeight = defaultLayout.GetSize().Height;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work correctly. Because it doesn't take all possible glyphs into account. For example the height of A is smaller than the height of Ag. It's not possible to measure the final height at this point.
Better measure the first line, as proposed here: https://github.com/mono/xwt/pull/794/files#r172651546

@@ -147,9 +164,16 @@ protected override void OnDraw (Context ctx, Rectangle cellArea)
layout.SetBackground (Colors.LightBlue, Math.Min (selectionStart, selectionEnd), Math.Abs (selectionEnd - selectionStart));

// Text doesn't fit. We need to render the expand icon
if (textSize.Width > cellArea.Width || textSize.Height > cellArea.Height) {

if (textSize.Width > cellArea.Width || textSize.Height > cellArea.Height) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this check again?

Copy link
Contributor Author

@netonjm netonjm Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ups! probably do it at the end of the day could be a great reason :)

It happens when text doesn't exceed the maxwidth and we have
multiple lines because the new lines
Copy link
Member

@sevoku sevoku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's better now 👍

@Therzok
Copy link
Contributor

Therzok commented May 16, 2018

What should we do about this PR?

Base automatically changed from master to main March 9, 2021 14:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants