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

Add text based cursor movement helpers #15779

Merged

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Jul 31, 2023

COOKED_READ_DATA is a little special and requires cursor navigation
based on the raw (buffered) text contents instead of what's in the
text buffer. This requires the introduction of new helper functions
to implement such cursor navigation. They're made part of TextBuffer
as these helpers will get support graphemes in the future.
It also helps keeping it close to TextBuffer as the cursor
navigation should optimally behave identical between the two.

Part of #8000.

@lhecker lhecker added the Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. label Jul 31, 2023
@@ -441,11 +441,16 @@ void TextBuffer::_PrepareForDoubleByteSequence(const DbcsAttribute dbcsAttribute
}
}

void TextBuffer::ConsumeGrapheme(std::wstring_view& chars) noexcept
size_t TextBuffer::GraphemeNext(const std::wstring_view& chars, size_t position) noexcept
Copy link
Member

Choose a reason for hiding this comment

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

Now, what is position. A column or a string index?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, good point. I forgot to add function documentation. 🙂
(It's a string index, since it's for text-based navigation.)

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

I am weirded out that they're in TextBuffer but I think i understand why

@lhecker
Copy link
Member Author

lhecker commented Jul 31, 2023

I am weirded out that they're in TextBuffer but I think i understand why

Once the implementation for these gets beyond simple surrogate pairs, it'll probably be more obvious. 😅 After all, both ROW and COOKED_READ_DATA will then use the same underlying text parsing algorithm. It's all still in flux, but I think I'll end up writing a TextAnalyzer class/namespace in the future which replaces CodepointWidthDetector. It'll then be used by the two classes instead of these new functions. But since such a class/namespace doesn't exist yet, I figured the next best thing is to at least keep it close to TextBuffer.

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Approving. Please be sure to add the function comment though.

@lhecker lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 31, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot merged commit a4340af into main Aug 1, 2023
15 of 17 checks passed
@microsoft-github-policy-service microsoft-github-policy-service bot deleted the dev/lhecker/8000-cmdline-prep1 branch August 1, 2023 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. AutoMerge Marked for automatic merge by the bot when requirements are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants