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.

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

Fix most of TextEdit visual issues. #97301

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

WhalesState
Copy link
Contributor

@WhalesState WhalesState commented Sep 21, 2024

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 WhalesState requested a review from a team as a code owner September 21, 2024 22:27
@WhalesState

This comment was marked as resolved.

@WhalesState
Copy link
Contributor Author

I shouldn't have touched this mine field! 😆

Calculations for width are alright but for height are alwrong.

@WhalesState

This comment was marked as resolved.

@Chaosus Chaosus added this to the 4.4 milestone Sep 22, 2024
@WhalesState WhalesState force-pushed the text-edit branch 3 times, most recently from af2a9ea to c517c72 Compare September 28, 2024 10:07
@WhalesState WhalesState marked this pull request as ready for review September 28, 2024 10:08
@WhalesState WhalesState requested a review from a team as a code owner September 28, 2024 10:08
@WhalesState
Copy link
Contributor Author

WhalesState commented Sep 28, 2024

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 WhalesState force-pushed the text-edit branch 2 times, most recently from a2e2caa to 8071071 Compare September 28, 2024 11:23
scene/gui/text_edit.cpp Outdated Show resolved Hide resolved
@kitbdev
Copy link
Contributor

kitbdev commented Sep 28, 2024

For the text clipping, see:

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?

@WhalesState
Copy link
Contributor Author

WhalesState commented Sep 28, 2024

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 WhalesState marked this pull request as draft September 28, 2024 16:43
@WhalesState WhalesState force-pushed the text-edit branch 4 times, most recently from c69badd to 8671283 Compare September 28, 2024 20:12
@WhalesState WhalesState marked this pull request as ready for review September 28, 2024 20:13
@WhalesState WhalesState requested a review from a team as a code owner September 28, 2024 20:13
@WhalesState
Copy link
Contributor Author

WhalesState commented Sep 29, 2024

@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.

@MewPurPur
Copy link
Contributor

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

@WhalesState
Copy link
Contributor Author

WhalesState commented Sep 29, 2024

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
Copy link
Contributor Author

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.

scroll.adjust.mp4

Copy link
Contributor

@kitbdev kitbdev left a comment

Choose a reason for hiding this comment

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

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

te_fix_visual_issues_minimap_issue

tests/scene/test_text_edit.h Outdated Show resolved Hide resolved
@@ -7445,7 +7445,7 @@ TEST_CASE("[SceneTree][TextEdit] viewport") {
text_edit->set_line_as_last_visible(visible_lines * 2);
MessageQueue::get_singleton()->flush();
CHECK(text_edit->get_first_visible_line() == visible_lines);
CHECK(text_edit->get_v_scroll() == visible_lines);
CHECK(floor(text_edit->get_v_scroll()) == visible_lines);
Copy link
Contributor

@kitbdev kitbdev Oct 3, 2024

Choose a reason for hiding this comment

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

For most of these you can add the fractional part to visible_lines. It should be _get_visible_lines_offset(), but since it isn't exposed, something like:

const float v_offset = 1.0 - ((text_edit->get_size().y - text_edit->get_minimum_size().y) / text_edit->get_line_height() - visible_lines);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have found that (text_edit->get_v_scroll_bar()->get_max() - floor(text_edit->get_v_scroll_bar()->get_max())) fixes it, also it's similar to the change that have caused the issue set_v_scroll(get_scroll_pos_for_line(first_line, next_line.y) + (v_scroll->get_max() - floor(v_scroll->get_max()))); in void TextEdit::set_line_as_last_visible(int p_line, int p_wrap_index).

@WhalesState
Copy link
Contributor Author

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

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants