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

TextEditor: Prevent autoscroll looping over #18781

Merged
merged 1 commit into from
Jun 4, 2023

Conversation

AhmedTheGeek
Copy link
Contributor

@AhmedTheGeek AhmedTheGeek commented May 12, 2023

When a text file has only 1 line with long text auto-scroll to the top will no longer loop over and set the cursor to the end of the line.

Before the fix:

Screencast.from.2023-05-12.14-09-59.webm

After:

Screencast.from.2023-05-12.14-10-44.webm

@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label May 12, 2023
Comment on lines 191 to 195
if (position.y() <= 0) {
column_index = 0;
line_index = 0;
return IterationDecision::Break;
}

Choose a reason for hiding this comment

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

i see it works but it still makes me wonder; iʼd guess iʼd go for y ≤ 0 yielding line → 0 and if y < 0 then additionally column → 0… 🤔

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 don't quite understand why this happens, but once y is -1, -2... column gets set to line length.

Choose a reason for hiding this comment

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

maybe we should not rely on that to avoid mistakes when this mysterious logic changes? 🤔

but thatʼs on you (and maintainers). 😄

When a text file has only 1 line with long text
autoscroll to the top will no longer loop over
and set the cursor to the end of the line.
@@ -181,7 +181,7 @@ TextPosition TextEditor::text_position_at_content_position(Gfx::IntPoint content
switch (m_text_alignment) {
case Gfx::TextAlignment::CenterLeft:
for_each_visual_line(line_index, [&](Gfx::IntRect const& rect, auto& view, size_t start_of_line, [[maybe_unused]] bool is_last_visual_line) {
if (is_multi_line() && !rect.contains_vertically(position.y()) && !is_last_visual_line)
if (is_multi_line() && !rect.contains_vertically(position.y()) && !is_last_visual_line && position.y() >= 0)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@konradekk I believe this was the reason the column index kept resetting to the line length after y got to values less than 0. We needed to keep our IterationDecision set to continue only if position.y value is greater than or equal to 0.

When that condition wasn't there it just kept repeating the loop forever.

Copy link

@konradekk konradekk left a comment

Choose a reason for hiding this comment

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

looks very concise; the maintainer should probably appreciate that… letʼs wait for their assessment!

@gmta gmta merged commit fd9dbf1 into SerenityOS:master Jun 4, 2023
@github-actions github-actions bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label Jun 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants