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

[TextEdit] Expose all auto-wrap modes. #74813

Merged
merged 1 commit into from
Jun 15, 2023

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Mar 12, 2023

Fixes #74256
Fixes #74812

@bruvzg bruvzg added this to the 4.x milestone Mar 12, 2023
@bruvzg bruvzg force-pushed the text_edit_autowrap branch 2 times, most recently from b920ed1 to 3ef8906 Compare March 21, 2023 07:09
@bruvzg bruvzg marked this pull request as ready for review March 21, 2023 07:17
@bruvzg bruvzg requested review from a team as code owners March 21, 2023 07:17
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

This looks like a good feature change, but it does break compatibility. So we need to make sure existing projects won't be broken when opened in 4.1.

@YuriSizov YuriSizov modified the milestones: 4.x, 4.1 Mar 21, 2023
Copy link
Member

@Paulb23 Paulb23 left a comment

Choose a reason for hiding this comment

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

I think there are two types of wrapping here, the text server for how to wrap, and TextEdit when to wrap. For example if adding a new wrap mode at the guidelines rather then the control boundary. Shouldn't we separate these into two properties?

@YuriSizov
Copy link
Contributor

I think that would just affect the width parameter somewhere. So it's not necessary to introduce some special wrapping mode for it, just a flag?

@Paulb23
Copy link
Member

Paulb23 commented Mar 25, 2023

No it needs to be wrapped, as there could be an option to allow mixed i.e fix column or viewport whichever is smallest, that cannot be conveyed with a width parameter (See VSCode). That was the intended purpose of this property.

@YuriSizov
Copy link
Contributor

fix column or viewport whichever is smallest, that cannot be conveyed with a width parameter

That still sounds like something that is conveyed with a width.

So we have two things to handle here:

  • What's the bounding box for the text (i.e. where to wrap),
  • And how to handle text wrapping (i.e. how to wrap).

The control can decide on the bounding box whichever way it wants. It can use its own rect, it can add extra constraints like the guide line, etc. From what I'm seeing, the enum that has been removed in this PR, LineWrappingMode, does not implement any of that yet. Instead if provides options similar to the TextServer::AutowrapMode enum, but more limited.

If the intend for the existing enum is to be extended to support options that you describe, then we can keep it. But then the new enum should still be added and exposed to the TextEdit API to allow defining the wrapping behavior itself.

But then the existing enum remains with a conflicting option, LINE_WRAPPING_NONE, which duplicates the text server behavior option. If we remove LINE_WRAPPING_NONE, then the LineWrappingMode enum is left with a single option for defining boundaries, and that means it doesn't do anything at this point. So IMO we should just replace the enum like this PR does, and then reintroduce it in some way later, as you implement those extended wrapping boundary options.

@Paulb23
Copy link
Member

Paulb23 commented Apr 2, 2023

Yes the intent was to prevent breaking compatibility during 4.0, leaving room for the functionality to be extended.

I think I like the idea of moving this to a separate API an leaving the current enum with one option, as I don't think TextEdit has a way to override the TextServer enum. That way we can add set_autowrap_mode methods which would align this functionality with the other control nodes i.e Label, Label3D and RichTextLabel.

@bruvzg bruvzg changed the title [TextEdit] Expose all auto-wrap modes, use TextServer auto-wrap enum instead of custom one. [TextEdit] Expose all auto-wrap modes. Jun 13, 2023
@akien-mga akien-mga requested a review from Paulb23 June 13, 2023 07:13
Comment on lines +3830 to +3833
CHECK(text_edit->get_first_visible_line() == (visible_lines / 2) + 6);
CHECK(text_edit->get_v_scroll() == (visible_lines + (visible_lines / 2)) + 1);
CHECK(text_edit->get_last_full_visible_line() == (visible_lines) + 5);
CHECK(text_edit->get_last_full_visible_line_wrap_index() == 0);
Copy link
Member Author

Choose a reason for hiding this comment

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

I do see any logic in these calculations, so I have only adjusted values to match real ones. Default TextEdit wrapping behavior should not be change, and the only reason this test was passing - set_line_wrapping_mode was not calling queue_redraw(); and updating wrapping until something else triggered redraw.

@bruvzg
Copy link
Member Author

bruvzg commented Jun 13, 2023

Updated to use separate property.

@akien-mga akien-mga merged commit 95a9089 into godotengine:master Jun 15, 2023
@akien-mga
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

TextEdit wrapmode not work TextEdit doesn't wrap the first word of a line
4 participants