Skip to content

fix(viewport): normalize method names#763

Merged
caarlos0 merged 5 commits intomasterfrom
scrolls
Mar 27, 2025
Merged

fix(viewport): normalize method names#763
caarlos0 merged 5 commits intomasterfrom
scrolls

Conversation

@caarlos0
Copy link
Copy Markdown
Contributor

@caarlos0 caarlos0 commented Mar 26, 2025

  • make sure all scrolling methods are called Scroll<direction>
  • renamed ViewUp et al to what they actually do (PageUp, HalfPageUp, etc)
  • remove ResetHorizontalScroll as we have SetXOffset
  • deprecated other methods using the deprecated high performance renderer

Maybe SetXOffset and SetYOffset should be unified in SetScroll(x, y)? Decided not to do this.

Also, should horizontal scroll be disabled by default? (currently isn't) - ✅ done

@caarlos0 caarlos0 added the enhancement New feature or request label Mar 26, 2025
@caarlos0 caarlos0 self-assigned this Mar 26, 2025
@caarlos0 caarlos0 requested a review from Copilot March 27, 2025 04:11
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR normalizes the naming of viewport scrolling methods and deprecates legacy functions in favor of the new naming conventions. Key changes include:

  • Renaming methods (e.g. ViewDown/ViewUp to PageDown/PageUp) and internally routing to ScrollDown/ScrollUp.
  • Updating deprecation comments and removing legacy horizontal scrolling methods.
  • Adjusting test cases to align with the new method names and expected behavior.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
viewport/viewport.go Normalizes scrolling method names and updates deprecation comments.
viewport/viewport_test.go Updates tests to use the new ScrollLeft/ScrollRight methods and expectations.
textarea/textarea.go Updates repositionView to call ScrollUp/ScrollDown instead of legacy methods.
textarea/textarea_test.go Updates tests to call ScrollDown in place of deprecated methods.
Comments suppressed due to low confidence (2)

viewport/viewport.go:198

  • [nitpick] The PR introduces both 'PageDown' and 'ScrollDown' naming for scrolling functions; consider further unification to avoid potential confusion among users, as the PR description hints at possibly unifying these names.
return m.ScrollDown(m.Height)

viewport/viewport_test.go:287

  • The expected output for the last visible line in TestVisibleLines changed from '...' to an empty string; please verify that this behavior change is intentional.
if list[lastItem] != "" {

it might be a breaking change.

let's enable it by default on v2, and remove the deprecated methods as
well.

cc/ @meowgorithm
@caarlos0 caarlos0 requested a review from Copilot March 27, 2025 18:22
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR normalizes the method names for scrolling operations in the viewport and textarea packages. Key changes include renaming scrolling methods (e.g. replacing MoveLeft/MoveRight with ScrollLeft/ScrollRight, and ViewUp/ViewDown with PageUp/PageDown), deprecating legacy high performance rendering functions, and updating tests accordingly.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
viewport/viewport_test.go Updated tests to use new Scroll* and Page* methods and removed deprecated calls
viewport/viewport.go Renamed and deprecated scrolling methods; updated inline documentation
textarea/textarea_test.go Adjusted tests to call ScrollDown instead of LineDown
textarea/textarea.go Updated scrolling calls to use ScrollUp/ScrollDown as needed
Comments suppressed due to low confidence (2)

viewport/viewport_test.go:17

  • [nitpick] Consider using the SetHorizontalStep method instead of setting m.horizontalStep directly to align test code with production code conventions.
m.horizontalStep = defaultHorizontalStep // remove on v2

viewport/viewport_test.go:296

  • Verify that expecting an empty string as the last item is intentional; the previous test expected '...' which might indicate a mismatch in behavior versus test expectations.
if list[lastItem] != "" {

Copy link
Copy Markdown
Member

@meowgorithm meowgorithm left a comment

Choose a reason for hiding this comment

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

This is SO NICE!

@caarlos0 caarlos0 merged commit 39668ec into master Mar 27, 2025
22 checks passed
@caarlos0 caarlos0 deleted the scrolls branch March 27, 2025 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants