ChimeHQ / Neon

A Swift library for efficient, flexible content-based text styling
BSD 3-Clause "New" or "Revised" License
335 stars 18 forks source link

Direct text storage attribute modifications #33

Closed mattmassicotte closed 1 year ago

mattmassicotte commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

cc @DivineDominion

DivineDominion commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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:

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Ok, here's what I've done.

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 commented 1 year ago

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

DivineDominion commented 1 year ago

Works fine with the manual selection 👍