Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for multiline cells to Tree #61714

Merged
merged 1 commit into from
Apr 25, 2023
Merged

Add support for multiline cells to Tree #61714

merged 1 commit into from
Apr 25, 2023

Conversation

dalexeev
Copy link
Member

@dalexeev dalexeev commented Jun 5, 2022

Confirming a change to a multiline cell is done with Ctrl+Enter instead of Enter.

My tree CSV editor

Test project: gd4-tree-multiline-items.zip

Closes godotengine/godot-proposals#3632.

@dalexeev dalexeev requested review from a team as code owners June 5, 2022 11:16
@KoBeWi
Copy link
Member

KoBeWi commented Jun 5, 2022

Confirming a change to a multiline cell is done with Ctrl+Enter instead of Enter.

Should be the opposite.

@KoBeWi KoBeWi added this to the 4.0 milestone Jun 5, 2022
@dalexeev dalexeev force-pushed the tree-multiline-items branch 2 times, most recently from 2f21498 to 8c8f099 Compare July 22, 2022 05:01
@reduz
Copy link
Member

reduz commented Aug 6, 2022

@KoBeWi this is up to you to decide what to do

@dalexeev
Copy link
Member Author

Confirming a change to a multiline cell is done with Ctrl+Enter instead of Enter.

Should be the opposite.

Fixed.

@KoBeWi this is up to you to decide what to do

@KoBeWi If you are going to rewrite Tree in the near future, then this PR can probably be closed. Just don't forget about multiline cells, please.

@KoBeWi
Copy link
Member

KoBeWi commented Sep 8, 2022

Needs rebase.
Also the text changes should likely be reviewed by @bruvzg

bruvzg
bruvzg reviewed Sep 8, 2022
View reviewed changes
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Changes seems fine. It's probably should additionally expose TextServer::AutowrapMode for the cell to have more control.

KoBeWi
KoBeWi reviewed Sep 9, 2022
View reviewed changes
doc/classes/TreeItem.xml Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Sep 9, 2022 •
edited
Loading

I tested this and:

  • newline should be Shift+Enter not Ctrl+Enter (it's common in different apps, mainly chat) EDIT: Ah it uses an existing action. I'd change it, but technically it's unrelated, so should be done in another PR.
  • multiline cells are unusable with default settings:
    image
    The size is too small. Maybe scroll_fit_content_height property is enough to fix that. Or at least docs should mention how to make the cell higher.

KoBeWi
KoBeWi reviewed Sep 9, 2022
View reviewed changes
scene/gui/tree.cpp Outdated Show resolved Hide resolved
KoBeWi
KoBeWi reviewed Sep 9, 2022
View reviewed changes
scene/gui/tree.h Outdated Show resolved Hide resolved
KoBeWi
KoBeWi reviewed Sep 9, 2022
View reviewed changes
scene/gui/tree.h Outdated Show resolved Hide resolved
KoBeWi
KoBeWi reviewed Sep 9, 2022
View reviewed changes
scene/gui/tree.cpp
@@ -1722,7 +1739,7 @@ void Tree::draw_item_rect(TreeItem::Cell &p_cell, const Rect2i &p_rect, const Co

if (rtl) {
Point2 draw_pos = rect.position;
draw_pos.y += Math::floor((rect.size.y - p_cell.text_buf->get_size().y) / 2.0);
draw_pos.y += Math::floor(p_draw_ofs.y) - _get_title_button_height();
Copy link
Member

@KoBeWi KoBeWi Sep 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Should this really use title height? It's a method for drawing cells (I think) and titles can be disabled, making their height 0.

EDIT:
Seems to work correctly though, so idk.

Copy link
Member Author

Choose a reason for hiding this comment

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

The title height is used for the initial drawing offset. If the title is hidden, then there is no offset. Perhaps this value is already taken into account in some variable, but I could not fix the offset error until I used this function. The Tree class is very large and needs to be refactored.

KoBeWi
KoBeWi reviewed Sep 9, 2022
View reviewed changes
doc/classes/TreeItem.xml Outdated Show resolved Hide resolved
KoBeWi
KoBeWi reviewed Sep 9, 2022
View reviewed changes
scene/gui/tree.h Outdated Show resolved Hide resolved
@dalexeev
Copy link
Member Author

  • multiline cells are unusable with default settings

The height of the editor is equal to the height of the line. If all cells are empty, Tree draws a line with a very small height. Perhaps this is a Tree problem? Maybe we need Tree to draw empty lines one line high?

  • Maybe scroll_fit_content_height property is enough to fix that.

In this case, the editor can overflow through the line without stretching it in height. I don't think this is what we need.

@KoBeWi
Copy link
Member

KoBeWi commented Sep 10, 2022

The height of the editor is equal to the height of the line. If all cells are empty, Tree draws a line with a very small height. Perhaps this is a Tree problem? Maybe we need Tree to draw empty lines one line high?

Well, if the cell is editable, it's not very useful when it's tiny. Maaybe having cell edit without other cells isn't very common, but for multiline edits the height should definitely match the edited text.

In this case, the editor can overflow through the line without stretching it in height.

You could try using minimum_size_changed signal to update the cell height.

@dalexeev
Copy link
Member Author

You could try using minimum_size_changed signal to update the cell height.

The problem is that until the change is applied, the text is only in the TextEdit, not in the Tree cell. So, the line height remains small. Changing the contents of a cell before the change is applied is wrong.

@KoBeWi
Copy link
Member

KoBeWi commented Sep 10, 2022

Then not sure what to do about the cell. IMO it's not fine if you have scrollable TextEdit, but then the text properly expands when you accept it. You could maybe change the cell text, but revert if it the user cancels editing text.

@dalexeev
Copy link
Member Author

dalexeev commented Sep 10, 2022 •
edited
Loading

I made the minimum height the same size as the height of one empty line:

You could maybe change the cell text, but revert if it the user cancels editing text.

I don't think this is a good solution since editing is a lengthy process and user code can get errors if it reads the text of the cells (the value can be new before the signal, and roll back if the user undoes the changes).

Ideally, Tree should be able to draw currently edited (not yet applied) values, distinguish between visual and logical content. But now it is difficult to implement.

@dalexeev
Copy link
Member Author

  • newline should be Shift+Enter not Ctrl+Enter (it's common in different apps, mainly chat)

In some applications this is Shift+Enter, in others Ctrl+Enter (for example, LibreOffice Calc).

KoBeWi
KoBeWi approved these changes Sep 10, 2022
View reviewed changes
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Can't say if the code is 100% correct, but looks ok and it works.

YuriSizov
YuriSizov reviewed Sep 12, 2022
View reviewed changes
doc/classes/TreeItem.xml Outdated Show resolved Hide resolved
@akien-mga akien-mga modified the milestones: 4.0, 4.1 Feb 10, 2023
@akien-mga
Copy link
Member

Needs a rebase, then I think it should be mergeable?

@dalexeev
Copy link
Member Author

Rebased.

@akien-mga akien-mga merged commit 1958d98 into godotengine:master Apr 25, 2023
@akien-mga
Copy link
Member

Thanks!

@dalexeev dalexeev deleted the tree-multiline-items branch April 25, 2023 17:55
@akien-mga akien-mga mentioned this pull request Apr 27, 2023
Closed
@bvigario
Copy link

I believe this breaks set_column_clip_content, at least when using text. I'm using this method in order to display ellipsis when text overflows the column width. Is there a way to get around this that I'm not seeing?

@dalexeev
Copy link
Member Author

See #76532 (comment). I added this to my TODO list and will try to fix it.

@dalexeev dalexeev mentioned this pull request Jun 3, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YuriSizov YuriSizov YuriSizov left review comments

@KoBeWi KoBeWi KoBeWi approved these changes

@bruvzg bruvzg Awaiting requested review from bruvzg

Assignees
No one assigned
Labels
enhancement topic:gui
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.

Add support for multi-line Tree column titles
7 participants
@dalexeev @KoBeWi @reduz @akien-mga @bvigario @bruvzg @YuriSizov