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

adding vertical scrolling based on fixed visual textInput compared to hiddenInput #589

Merged
merged 26 commits into from
Apr 29, 2024

Conversation

hanbollar
Copy link
Contributor

@hanbollar hanbollar commented Apr 15, 2024

Linking

related to #590

Problem

Description of the problem including potential code and/or screenshots as an example

Solution

Quick explanation of change to be done

Breaking Change

If this is a breaking change describe the before and after and why the change was necessary

Notes

Notes and any associated research or links


Required to Merge

  • PASS - all necessary actions must pass (excluding the auto-skipped ones)
  • TEST IN HEADSET - main dev-testing-example and any of the other examples still work as expected
  • VIDEO - if this pr changes something visually - post a video here of it in headset-MR and/or on desktop (depending on what it affects) for the reviewer to reference.
Untitled.mov
  • TITLE - make sure the pr's title is updated appropriately as it will be used to name the commit on merge
    - [ ] BREAKING CHANGE
    • DOCUMENTATION: This includes any changes to html tags and their components
      • make a pr in the documentation repo that updates the manual docs to match the breaking change
      • link the pr of the documentation repo here: #pr
      • that pr must be approved by @lobau
    • SAMPLES/INDEX.HTML: This includes any changes (html tags or otherwise) that must be done to our landing page submodule as an effect of this pr's updates
      • make a pr in the mrjs landing page repo that updates the landing page to match the breaking change
      • link the pr of the landing page repo here: #pr
      • that pr must be approved by @hanbollar

Signed-off-by: hanbollar <[email protected]>
Signed-off-by: hanbollar <[email protected]>
Signed-off-by: hanbollar <[email protected]>
Copy link

render bot commented Apr 15, 2024

@hanbollar
Copy link
Contributor Author

hanbollar commented Apr 15, 2024

so far on this pr - have some scrolling working by actual key input presses and text update

to fix for vertical happiness:

  • text deletion struggles with the update for the change in visual scroll
  • cursor movement works until it doesnt
  • the special case where startLineIndex and endLineIndex for textObj are the same

in a future pr:

  • horizontal scrolling and word wrap

@hanbollar hanbollar mentioned this pull request Apr 15, 2024
29 tasks
@hanbollar
Copy link
Contributor Author

so far on this pr - have some scrolling working by actual key input presses and text update

to fix for vertical happiness:

  • text deletion struggles with the update for the change in visual scroll
  • cursor movement works until it doesnt
  • the special case where startLineIndex and endLineIndex for textObj are the same

in a future pr:

  • horizontal scrolling and word wrap

progress:

  • text deletion struggles with the update for the change in visual scroll
  • cursor movement works until it doesnt
  • the special case where startLineIndex and endLineIndex for textObj are the same

outstanding todo:

  • weird case where doing an input key after doing a cursor movement even though no textobj scroll should happen can shift textObj line display by 1, but only on that first cursor direction after a textobj shift --> prob an off by one error somewhere

@hanbollar
Copy link
Contributor Author

so far on this pr - have some scrolling working by actual key input presses and text update
to fix for vertical happiness:

  • text deletion struggles with the update for the change in visual scroll
  • cursor movement works until it doesnt
  • the special case where startLineIndex and endLineIndex for textObj are the same

in a future pr:

  • horizontal scrolling and word wrap

progress:

  • text deletion struggles with the update for the change in visual scroll
  • cursor movement works until it doesnt
  • the special case where startLineIndex and endLineIndex for textObj are the same

outstanding todo:

  • weird case where doing an input key after doing a cursor movement even though no textobj scroll should happen can shift textObj line display by 1, but only on that first cursor direction after a textobj shift --> prob an off by one error somewhere

outstanding todo:

  • fix offby one maxendline index being too short only on keypress, but fine on scrolling

Signed-off-by: hanbollar <[email protected]>
Signed-off-by: hanbollar <[email protected]>
Signed-off-by: hanbollar <[email protected]>
Signed-off-by: hanbollar <[email protected]>
Signed-off-by: hanbollar <[email protected]>
Signed-off-by: hanbollar <[email protected]>
@hanbollar
Copy link
Contributor Author

so far on this pr - have some scrolling working by actual key input presses and text update
to fix for vertical happiness:

  • text deletion struggles with the update for the change in visual scroll
  • cursor movement works until it doesnt
  • the special case where startLineIndex and endLineIndex for textObj are the same

in a future pr:

  • horizontal scrolling and word wrap

progress:

  • text deletion struggles with the update for the change in visual scroll
  • cursor movement works until it doesnt
  • the special case where startLineIndex and endLineIndex for textObj are the same

outstanding todo:

  • weird case where doing an input key after doing a cursor movement even though no textobj scroll should happen can shift textObj line display by 1, but only on that first cursor direction after a textobj shift --> prob an off by one error somewhere

outstanding todo:

  • fix offby one maxendline index being too short only on keypress, but fine on scrolling
  • fix offby one maxendline index being too short only on keypress, but fine on scrolling

Signed-off-by: hanbollar <[email protected]>
@hanbollar
Copy link
Contributor Author

hanbollar commented Apr 19, 2024

primary task after this pr:

  • handle word wrapping and horizontal scrolling nicely - note: textobj partially handles it already, but not always in the best way
  • make sure up/down arrows actually hit the end of the line (check the video from the pr header, they might not actually be there when doing up/down cursor only)

a few lingering todos from this pr for cleanup when doing the horizontal scrolling addition:

note these are not dependent on this pr, these are nice-to-have code-cleanups that need to happen before exposure though

after:

once all those are resolved then can expose again and start on going down more of 'fully supported' feature list

@hanbollar
Copy link
Contributor Author

hanbollar commented Apr 19, 2024

@michaelthatsit - solved the off-by-1 error, this is now ready for review

check out my todo list for notes (doing it written before the review this time instead of answering /oh yea i swear that'll be handled lol/ as part of the review to help with the process a bit)

btw when reading through start with TextInputEntity before looking at TextAreaEntity/TextFieldEntity changes so that the comments make more sense

lmk if any questions ~

@hanbollar hanbollar changed the title WIP - adding scrolling based on fixed visual textInput compared to hiddenInput adding vertical scrolling based on fixed visual textInput compared to hiddenInput Apr 19, 2024
Copy link
Member

@michaelthatsit michaelthatsit left a comment

Choose a reason for hiding this comment

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

lgtm! and glad you got the lingering cursor issue when switching input fields, forgot to mention it last time.

@hanbollar hanbollar merged commit 1d329c3 into main Apr 29, 2024
6 checks passed
@hanbollar hanbollar deleted the hb-textinput-scrolling branch April 29, 2024 17:45
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.

2 participants