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

Improve TextEdit and CodeEdit documentation #97656

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

kitbdev
Copy link
Contributor

@kitbdev kitbdev commented Sep 30, 2024

  • fixes Auto Brace Complete editor setting is incorrectly described #98260
  • The descriptions for get_visible_line_count and get_total_visible_line_count in particular seemed to be swapped.
  • Removes Sets whether and Sets if.
  • Uses [member ProjectSettings.input/...] for actions (and found ui_text_unindent is actually supposed to be ui_text_dedent).
  • Adds references to related methods, members, and theme items.
  • Adds details in some places that were lacking.

@kitbdev kitbdev requested a review from a team as a code owner September 30, 2024 17:51
@AThousandShips AThousandShips added this to the 4.4 milestone Sep 30, 2024
@AThousandShips AThousandShips requested a review from a team September 30, 2024 19:00
Copy link

@Loff3 Loff3 left a comment

Choose a reason for hiding this comment

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

I've suggested a few small changes to the wording that might help improve clarity.

</description>
</method>
<method name="unindent_lines">
<return type="void" />
<description>
Unindents selected lines, or in the case of no selection the caret line by one. Same as performing "ui_text_unindent" action.
Unindents all lines that are selected or have a caret on them. Uses spaces or a tab depending on [member indent_use_spaces]. Same as performing the [member ProjectSettings.input/ui_text_dedent] action. See [method indent_lines].
Copy link

@Loff3 Loff3 Oct 14, 2024

Choose a reason for hiding this comment

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

Suggested change
Unindents all lines that are selected or have a caret on them. Uses spaces or a tab depending on [member indent_use_spaces]. Same as performing the [member ProjectSettings.input/ui_text_dedent] action. See [method indent_lines].
Unindents all selected lines or lines containing a caret. Uses spaces or a tab depending on [member indent_use_spaces]. Equivalent to performing the [member ProjectSettings.input/ui_text_dedent] action. See [method indent_lines].

Copy link
Contributor Author

@kitbdev kitbdev Oct 16, 2024

Choose a reason for hiding this comment

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

For lines that are selected or have a caret on them I made it this way to match other descriptions for consistency.
Also in your suggestion, it should be and instead of or to avoid ambiguity.

@kitbdev
Copy link
Contributor Author

kitbdev commented Oct 17, 2024

Added auto brace completion description in Editor Settings as well.

Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Thank you very, very much for changing these for the better. These descriptions were written really awkwardly, but there could be work to do still.

@@ -1045,7 +1045,7 @@
<param index="0" name="gutter" type="int" />
<param index="1" name="overwritable" type="bool" />
<description>
Sets the gutter to overwritable. See [method merge_gutters].
If [code]true[/code], the gutter lines can be overridden when using [method merge_gutters]. See [method is_gutter_overwritable].
Copy link
Contributor

Choose a reason for hiding this comment

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

You used "overwritable" elsewhere too, plus that's part of name of the actual method.

Suggested change
If [code]true[/code], the gutter lines can be overridden when using [method merge_gutters]. See [method is_gutter_overwritable].
If [code]true[/code], the gutter lines can be overwritable when using [method merge_gutters]. See [method is_gutter_overwritable].

Also worth keeping in mind that this makes the subject way more ambiguous. "the gutter lines" could be referring to all of the gutters, but this method requires a gutter index to be passed.

Copy link
Contributor Author

@kitbdev kitbdev Nov 12, 2024

Choose a reason for hiding this comment

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

"overwritable" means "can be overridden", so I'm not sure this suggestion is right. Also I am trying to describe the scenario when it may be overridden. When true it is always "overwritable".

I'll change "the gutter lines" to "lines of the gutter at the given index".

Edit: actually it's more accurate to say "line data of the gutter at the given index can be overridden..."
Its stuff like the icon, color, and metadata that are stored per line that get overridden.

@kitbdev kitbdev force-pushed the doc-textedit-improve branch 2 times, most recently from 53c9037 to fb3eb53 Compare November 12, 2024 22:27
@kitbdev
Copy link
Contributor Author

kitbdev commented Nov 12, 2024

Rebased and applied suggestions.
I think I got everywhere that I already touched for the gutter at index or given line. I changed a few others, but there is more that I didn't change.
Also added descriptions for a few more methods like CodeEdit::can_fold_line and TextEdit::get_indent_level.

@Mickeon Mickeon self-requested a review November 12, 2024 23:15
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Get ready for a huge second pass revision by yours truly just to ensure we don't have to do this again for a loooong while.

I am really happy about this and I must congratulate you for the hard work.

@kitbdev kitbdev force-pushed the doc-textedit-improve branch from fb3eb53 to d467b3a Compare November 14, 2024 22:18
@Mickeon Mickeon self-requested a review November 14, 2024 23:43
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

No beating around the bush. All of this is a huge improvement.

@Repiteo Repiteo merged commit cf541f0 into godotengine:master Nov 18, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 18, 2024

Thanks!

@kitbdev kitbdev deleted the doc-textedit-improve branch November 18, 2024 18:07
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.

Auto Brace Complete editor setting is incorrectly described
6 participants