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

Direct text storage attribute modifications #33

Closed
mattmassicotte opened this issue Nov 18, 2023 · 13 comments
Closed

Direct text storage attribute modifications #33

mattmassicotte opened this issue Nov 18, 2023 · 13 comments

Comments

@mattmassicotte
Copy link
Contributor

#28 and #31 brings up the common problem of attribute invalidation. Using temporary attributes has a big advantage, because it gives you another "layer" of attributes.

Many users have run into issues when not using temporary attributes in the past. Let's talk about possible solutions.

@mattmassicotte
Copy link
Contributor Author

Yeah, thinking about this more, I believe we might have been rushing a little too fast. While it is easy to create your own TextSystemInterface to avoid this problem, I'm pretty sure making this the default behavior without any. means of control will result in regressions for users.

@mattmassicotte
Copy link
Contributor Author

cc @DivineDominion

@DivineDominion
Copy link
Contributor

In a regular rich text NSTextView, the typingAttributes change to whatever the cursor/insertion point is touching. This also affects font and textColor attributes -- neither are constant, but rather computed.

Since the approach of storing attributes interferes wins over the text storage's attribute fixing procedure (which IIRC incorporates temporary attributes from the layout manager), the only solution I found myself is to tell the text storage what to use by default instead.

I'll show you some code in a minute @mattmassicotte

@DivineDominion
Copy link
Contributor

Yeah, thinking about this more, I believe we might have been rushing a little too fast. While it is easy to create your own TextSystemInterface to avoid this problem, I'm pretty sure making this the default behavior without any. means of control will result in regressions for users.

Sorry for not being sufficiently diligent! It looked alright until I added more font settings, so that was a sad surprise indeed.

@mattmassicotte
Copy link
Contributor Author

Absolutely no need to apologize! I like working quickly, and it's ok for main to be unstable.

I think it may make sense to factor out these two different modes into dedicated TextSystemInterface implementations, and then have it switchable inside the TextViewSystemInterface. Or something...

@DivineDominion
Copy link
Contributor

DivineDominion commented Nov 18, 2023

I opened #34 with a proposed solution that leans fully into stored attributes.

Splitting this into two separate approaches, one for computed, temporary attributes, and one for stored attributes, sounds sensible (e.g. to enable checking on TextKit 2 updates, like for fixes to #20) and a maintenance nightmare both, though.

I'm with @MrNoodle from 2012, and @danielpunkass (ca 2017 AppKit Abusers) since MarsEdit's font is not allowed to change. This all makes sense, highlighting is not part of the document, so don't store it in the text storage.

Jakob Egger in 2017 also pointed out (in the comments) that this can be achieved by having a separate attribute storage just for the highlighter attributes. Then applying attributes won't again trigger processEditing etc. (see #32). That's why ended up doing, too, years later.

If not setting fonts is fine, by all means we should undo my changes :)

@mattmassicotte
Copy link
Contributor Author

I read though that post as well as the comment you referenced. My opinion is that the text storage should represent the actual text data. Highlighting can be derived from it, but doesn't have to be stored there just because it is convenient to do so. I'm very into the idea of a separate highlighting attribute storage.

However! I'm really interested in making this system compatible with whatever means you want to use. I don't want my opinion to matter. And, the whole point of TextViewSystemInterface is to provide a simpler means of integrating all the parts together. So, what I propose is:

  • A new TextSystemInterface-conforming type that uses temporary attributes
  • A new TextSystemInterface-conforming type that uses NSTextStorage directly
  • Modification to TextViewSystemInterface to allow a user to select which they'd like

This way, users of Neon can decide how they'd like it all to work. Plus, you can always just side-step the entire thing and make your own TextSystemInterface, if you need the extra control. This is how I use this library myself.

@mattmassicotte
Copy link
Contributor Author

mattmassicotte commented Nov 18, 2023

I also want to point out that there is comment in that blog post I'm very skeptical of:

modifying the NSTextStorage is not quite as efficient as we’d like

I bet the issue here is not that modifying the storage is slow, but that modifying it this way is causing synchronous layout work to happen, and that is slow. Layout, in general, is extremely expensive. This can be a serious performance issue even if you are really careful with your invalidation ranges, which Neon is.

This is one the reasons temporary attributes are great - they do not affect layout.

@DivineDominion
Copy link
Contributor

I bet the issue here is not that modifying the storage is slow, but that modifying it this way is causing synchronous layout work to happen, and that is slow. Layout, in general, is extremely expensive. This can be a serious performance issue even if you are really careful with your invalidation ranges, which Neon is.

That does make sense in hindsight!

Hmm I do believe we didn't try in earnest to limit layout-affecting attributes (actually mostly .font, because others like line height or paragraph styles are applied uniformly and not on the attributed string level) to be kept in the storage itself, and applying decorations like different colors as temporary attributes 🤔 I abandoned that once, earlier, but that was before the 'highlighting engine' was as powerful and sophisticated. The limiting factor was the bad parser back then.

However! I'm really interested in making this system compatible with whatever means you want to use. I don't want my opinion to matter.

While I believe that the demo now is more powerful and impressive for folks interested in getting a highlighter demo to work, the downsides are apparent.

I could implement these components in my simple code editor just as well. So it's not like I couldn't use Neon, personally, unless the convenience components have these features!


To make these changes, do you believe there's any value in separating a Neon core from the selection of convenience kickstarter components in a separate package target? (Or two, one per approach)

@mattmassicotte
Copy link
Contributor Author

I am, in fact, thinking a lot about this package and how it will be organized. More to come on that front soon! But, for the immediate problem, I think all we need to do is refactor this a little bit so the user has more control.

I think the best approach is to move this decision about highlighting into specialized TextSystemInterface implementations. Then, it will be much easier to control. Neon was built to be flexible here, so I'm planning on just making use of that.

@mattmassicotte
Copy link
Contributor Author

Ok, here's what I've done.

  • LayoutManagerSystemInterface uses temp attributes (macOS only)
  • TextLayoutManagerSystemInterface uses TK2 rendering attributes
  • TextStorageSystemInterface uses storage attributes
  • expanded TextViewHighlighter API slightly to make these possible to use
  • restored TextViewSystemInterface to original behavior, but using these new interfaces internally

I also make use of NSTextView's typingAttributes as a default styling for the TextStorageSystemInterface usage, which fixes the font misassignment.

Overall, I think this is a good change. TextViewHighlighter is now more flexible, and there are built-in solutions for the most common Apple text view configurations.

This will require a little bit of code change on your end though. You'll now need to supply a TextStorageSystemInterface to your TextViewHighlighter.

@DivineDominion
Copy link
Contributor

Wow, that was quick and very thorough! I'll check things out ASAP, thank you

@DivineDominion
Copy link
Contributor

Works fine with the manual selection 👍

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