godot icon indicating copy to clipboard operation
godot copied to clipboard

Fix most of `TextEdit` visual issues.

Open WhalesState opened this issue 1 year ago • 11 comments

  • Fixes: https://github.com/godotengine/godot/issues/97099
  • Fixes: https://github.com/godotengine/godot/issues/42342
  • Fixes: https://github.com/godotengine/godot/issues/32996

Additonally:

  • Fixes Line highlighter start and end positions.
  • Respect the scroll bars minimum size.

Fixes was applied one by one to fix each issue separetly, with a test after each change to make sure the selected issue was fixed before moving to the next one.

Please add this to the 4.4 milestone, since most of the fixes are applied to the DRAW notification.

For testing, also check ShaderEditor, ScriptEditor and CodeEdit.

Thanks in advance.

WhalesState avatar Sep 21 '24 22:09 WhalesState

oh, I forgot to track this.

TextEdit::get_line_wrap_count: Index p_line = -4 is out of bounds (text.size() = 55).
  <C++ Source>   scene\gui\text_edit.cpp:5659 @ TextEdit::get_line_wrap_count()

Edit: Fixed. I will track the issue in the tests now.

WhalesState avatar Sep 21 '24 22:09 WhalesState

I shouldn't have touched this mine field! 😆

Calculations for width are alright but for height are alwrong.

WhalesState avatar Sep 22 '24 01:09 WhalesState

While working on this, I have realized that respecting the top/bottom content margins is tricky, since it will result in an immediate clipping when the margins are exceeded, also we still have a background color which is a rect that is drawn manually over the stylebox.

So maybe we can extend from Panel instead of Control and we draw everything inside an internal Control child, this will allow smooth clipping and easier calculations without considering any margins.

WhalesState avatar Sep 22 '24 03:09 WhalesState

For this pull request it's enough to respect the scroll bars when they appear and the content margins, also to not draw the readonly style over the normal style.

I will try to fix some minimap issues appears on long scripts in another pull request, then at last i will try to do something for RTL issues, I couldn't find any solutions for fixing text clipping because drawing to another child control may break compatibility and will result in a lot of changes for CodeEdit and TextEdit, I wish there was something like RenderingServer::start_draw_clipping(RID p_canvas_item, Rect2 p_clipping_rect) and RenderingServer::end_draw_clipping(RID p_canvas_item).

WhalesState avatar Sep 28 '24 10:09 WhalesState

For the text clipping, see:

  • https://github.com/godotengine/godot/issues/42342
  • https://github.com/godotengine/godot/issues/32996
  • https://github.com/godotengine/godot/pull/87033

From that LineEdit PR, it seems the solution is to have a separate RID to use for the text. The RenderingServer has methods like canvas_item_set_custom_rect.

Also can you update the description? Since it looks like the RTL issues are going to be later.

Does this close the proposal too?

  • https://github.com/godotengine/godot-proposals/issues/10774

kitbdev avatar Sep 28 '24 16:09 kitbdev

From that LineEdit PR, it seems the solution is to have a separate RID to use for the text. The RenderingServer has methods like canvas_item_set_custom_rect.

This is what i was searching for, Thank You!

Does this close the proposal too?

Yes, since it's now drawing only the readonly stylebox when it's not editable.

WhalesState avatar Sep 28 '24 16:09 WhalesState

@MewPurPur Those changes may affect GodSVG, I have checked it's source code, and i see it can save some line of codes in your project.

  • The VScrollBar now respects the content margins.
  • The text now is clipped to the content margins.
  • The focus style box now can be used without any issues.

Note: I have removed any uncontrolled margins to be replaced later with theme h_separation and v_separation which will only take effect when the scroll bars are visible.

I'd like to know if there are any issues that we can consider to make the TextEdit more usable.

WhalesState avatar Sep 29 '24 05:09 WhalesState

Cool! Thanks for checking. I do have some other grievances with TextEdit but they are unrelated.

MewPurPur avatar Sep 29 '24 07:09 MewPurPur

I really hate those unit tests. Just fixed the first/last line scroll offsets to make them fully visible when adjusting the viewport to them and now i have to deal with the tests again :)))))))

WhalesState avatar Sep 29 '24 10:09 WhalesState

I just have used floor(text_edit->get_v_scroll()) instead since my tests are broken in my hard fork due to removing a lot of classes.

./tests/scene/test_text_edit.h:7496: ERROR: CHECK( text_edit->get_v_scroll() == visible_lines ) is NOT correct!
  values: CHECK( 21.3704 == 21 )

./tests/scene/test_text_edit.h:7517: ERROR: CHECK( text_edit->get_v_scroll() == 32.0 ) is NOT correct!
  values: CHECK( 32.3704 == 32 )

./tests/scene/test_text_edit.h:7601: ERROR: CHECK( text_edit->get_v_scroll() == 5 ) is NOT correct!
  values: CHECK( 5.37037 == 5 )

./tests/scene/test_text_edit.h:7657: ERROR: CHECK( text_edit->get_v_scroll() == (visible_lines + (visible_lines / 2)) + 1 ) is NOT correct!
  values: CHECK( 32.0741 == 32 )

./tests/scene/test_text_edit.h:7805: ERROR: CHECK( text_edit->get_v_scroll() == 0 ) is NOT correct!
  values: CHECK( 0.37037 == 0 )

./tests/scene/test_text_edit.h:7814: ERROR: CHECK( text_edit->get_v_scroll() == 20 ) is NOT correct!
  values: CHECK( 20.3704 == 20 )

./tests/scene/test_text_edit.h:7823: ERROR: CHECK( text_edit->get_v_scroll() == 20 ) is NOT correct!
  values: CHECK( 20.3704 == 20 )

./tests/scene/test_text_edit.h:7864: ERROR: CHECK( text_edit->get_v_scroll() == 1 ) is NOT correct!
  values: CHECK( 1.37037 == 1 )

./tests/scene/test_text_edit.h:7873: ERROR: CHECK( text_edit->get_v_scroll() == 21 ) is NOT correct!
  values: CHECK( 21.3704 == 21 )

./tests/scene/test_text_edit.h:7882: ERROR: CHECK( text_edit->get_v_scroll() == 21 ) is NOT correct!
  values: CHECK( 21.3704 == 21 

This is the second fix that have made the tests fails, when scrolling to the first or last line, i have insured that they become fully visible (if they aren't), similar to other text editors.

https://github.com/user-attachments/assets/1151fc6c-210f-4b32-9a0d-b1ee38d6e226

WhalesState avatar Sep 29 '24 11:09 WhalesState

There is an issue with hovering and clicking on the minimap at the left and bottom sides:

Fixed.

WhalesState avatar Oct 03 '24 22:10 WhalesState

Hey, just a heads up, this seems to have introduced a bug where overwriting the text with a bigger block, doesn't update the TextEdit/CodeEdit length, which effects scrolling. My assumption is that either a function isn't triggered, or the function lacks a check and update.

AlyceOsbourne avatar Nov 15 '24 19:11 AlyceOsbourne

The class is like a maze and maintaining it is so hard, I have spent a week working on this and it will be better if anyone can break it down into smaller PRs.

WhalesState avatar Jan 12 '25 11:01 WhalesState