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

Introducing StringSlice for Segment.Text #671

Closed
xoofx opened this issue Dec 26, 2021 · 4 comments
Closed

Introducing StringSlice for Segment.Text #671

xoofx opened this issue Dec 26, 2021 · 4 comments

Comments

@xoofx
Copy link

xoofx commented Dec 26, 2021

Hello Spectre.Console friends!

I'm starting to evaluate how I can use Spectre.Console for editing/rendering syntax highlighted languages and I have a suggestion to improve its support.

Is your feature request related to a problem? Please describe.

While a Segment has a Text property defined as a string, it forces to split the original text string by its styled strings and to maintain externally a map of the split strings with the original string. e.g:

var x = 5; would generate var, x, =, 5, ; and we would have to keep the character position of each splits from the original string.

Describe the solution you'd like

Instead of using a string for Segment.Text we could use a string slice (e.g a string + offset + length) which could allow to use the same original string but offer a view on it to the segment.

Most of the code using it would not change much, as the contact of this type would mostly replicate the contract of a string.

There is the type StringSegment but it would bring a new dependencies. Also I discovered that it doesn't provide a char iterator (and some other methods, e.g like Trim)

So the question is: can we use StringSegment (we can provide extension methods/enumerator on it likely) or should we introduce a new type?

Describe alternatives you've considered

Maintaining the map to the original string outside of Spectre.Console.

Additional context

I can make a draft PR if you are interested in this feature? 🙂

@patriksvensson
Copy link
Contributor

@xoofx Sounds like an excellent idea. We're aware of the problem but deliberately haven't been optimizing stuff (yet) in favor of getting things to work.

If you want to send a draft PR for this, it would be greatly appreciated.

@xoofx
Copy link
Author

xoofx commented Dec 26, 2021

Great. What should I do about the type? Use StringSegment with a dependency to Microsoft.Extensions.Primitives or add a type e.g StringSlice and copy paste the code from it?

@patriksvensson
Copy link
Contributor

I have no problem adding a dependency on Microsoft.Extensions.Primitives if supported by all the tfm:s that we support. I'll leave that decision to you, whatever you find the cleanest.

Again, really appreciate you raising this issue (and, of course, doing a PR).

@xoofx
Copy link
Author

xoofx commented Dec 29, 2021

Hey,
So I have been digging into the changes. Got them partially working but it got quite viral and many little things in the lib are by design not prepared for such a change (lots of allocations here and there, many assumptions about string concatenations e.g Segment.Merge...etc.) so even after my changes, that would not solve the issue I brought up originally... so, unfortunately, I'm not sure I would like to bring such changes to Spectre.Console myself, as they are too disruptive.

Also, after digging more into the details, I'm a bit too frustrated by the Console API itself so I have started to play with more xplat virtual terminal sequences (with mouse events...etc.) (quite related to dotnet/runtime#52374 actually) and I don't know where I'm heading 🙂

So I'm gonna close this issue, my apologize for the false hope!

@xoofx xoofx closed this as completed Dec 29, 2021
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

No branches or pull requests

2 participants