Minimise the need to batch operations in PlainEditor

[DJMcNab]

The current design of PlainEditor and PlainEditorTransaction is that if the editor is accessible, the inner layout is always valid and up-to-date (barring a caught panic in the transact method). This is not ideal for Masonry, because it forces event handling to recreate the full layout even if the old version could be reused; this potential reuse is especially prevalent in updates driven by Xilem.

This does bring in another change from Masonry, which is StyleSet; this is a set of style properties, differentiated by their kind. It can contain at most one font size, line height, etc.
This has some potentially unexpected behaviour with FontStacks, as in the other APIs they "combine", but in this format they. However, this combining behaviour is not documented as far as I can see, so I think it's reasonable.
This implemented using a hashmap of discriminants in the enum.
The current API of Arc<[StyleProperty]> is horrendously unergonomic.

To reflect this change, I've renamed PlainEditorTxn to PlainEditorDriver. Suggestions for an alternative name are welcome.

[jaredoconnell]

I'll defer to those more familiar with the code regarding the design choices, but the changes themselves look good, and I can't find any regressions. Tested with Vello editor and Masonry, Windows and MacOS. Tested accessibility on MacOS.

[tomcur]

We may also want to say support for IME preedit composing text is is introduced into PlainEditor.

[DJMcNab]

Addressed in #197. Sorry for missing this!

[nicoburns]

The changes seem broadly good. However, there seem to be at least 3 independent changes in this PR:

• Change from transaction to dirty tracking
• Addition of IME functionality
• Change of style representation

I would prefer to review and land them separately if possible, but I have also tried to add review comments to this PR as-is.

[nicoburns]

The alignment method on Layout takes a separate width parameter. If you pass None to that method AND this method then the computed width is used for alignment.

[DJMcNab]

Hmm, can you clarify how a user of PlainEditor is expected to call the alignment method on Layout? As far as I can tell, that method takes exclusive access to self, but the PlainEditor doesn't grant exclusive access to the layout.

This comment is largely discussing Masonry semantics, so I'll remove it. Using a sensible available width for alignment is something I would like to do, but it doesn't block this PR.

[nicoburns]

So I would expect PlainEditor to manage this internally (probably using layout.width() if width is None). Users of Layout can have more control.

[DJMcNab]

Sure. I don't really want to re-vendor PlainEditor in Masonry if we can avoid it, but it's not a hill I'm willing to die on.

[nicoburns]

Does Masonry need to control this? I'm not opposed to adding functionality in for that. I just thought that PlainEditor had enough information available to "just do" the right thing for the text input use case.

(we may be speaking past each other?)

[DJMcNab]

So, the case I'm thinking about is the scenario where you don't want word wrap, but have a finite available area, and want to be right-aligned (think for example, a spreadsheet app). In that case, your width used for alignment would be the width available, but the width used for layout would be None.

[nicoburns]

Hmm... if we actually had a way to disable word wrap then that might be a better way to handle that case. But maybe we should add a property to the editor in the short term?

It also brings up the case of a scrollable text field. Might we want to implement that in PlainEditor?

[nicoburns]

Would happens if one of these methods is called while composing (in release mode, so the assert is skipped)? If it's bad, should we implement some kind of fallback behaviour such as:

• Make it a noop
• Cancel compose mode then execute it as normal

I'm keen to ensure that it's impossible for the editor to end up in a corrupted state.

If it's just a "bad idea" to call these when composing but it'll work fine, then I'm not sure we ought to have asserts.

[DJMcNab]

You are liable to end up in a really janky state.

If you really want us to implement a fallback, then making them a no-op would be the right one. It is, as documented, a logic error to call them, and they can't have any meaningful semantics. Doing an early return would be some quite ugly boilerplate:

if self.editor.is_composing() {
    if cfg!(debug_assertions) {
         panic!("Can't call most PlainEditor methods whilst composing");
    } else {
         return;
    }
}

But if you want us to the check unconditionally, we may as well just make it a full assert.

Cancelling compose mode isn't something we can do at this level of semantics; to cancel compose mode, you need to tell that platform that you're doing so, and we don't know what the platform is.

[nicoburns]

You are liable to end up in a really janky state.
If you really want us to implement a fallback, then making them a no-op would be the right one.

My main point here being that if debug assertions are disabled, then the current code will lead to any such janky state (because there is no protection other than the debug assertion). Seems to me it should either be an unconditional panic or an early return noop (perhaps also logging a warning). I'd probably prefer the latter as I don't tend to like libraries panicking on me unexpectedly in produciton.

Doing an early return would be some quite ugly boilerplate:

Perhaps a good case for a macro_rules macro?

[tomcur]

I'd be happy with it being a noop or assert on release, and an assert in debug mode. The janky state can get as bad as leading to crashes when the preedit byte offsets go out of text buffer bounds.

[DJMcNab]

The changes seem broadly good. However, there seem to be at least 3 independent changes in this PR:

• Change from transaction to dirty tracking
• Addition of IME functionality
• Change of style representation

I would prefer to review and land them separately if possible, but I have also tried to add review comments to this PR as-is.

The IME functionality was available to review on its own in linebender/xilem#762, fwiw. I agree that the style change is relatively orthogonal, but both sets of changes were needed to port to PlainEditor in Masonry, so seperating them out again would from my perspective have been largely just busywork.

[nicoburns]

The IME functionality was available to review on its own in linebender/xilem#762, fwiw.

FWIW, I do not feel competent to review the IME stuff. Previous PR's looked "fine" to me, but apparently had issues on some platforms. Keen to get support merged though!

[DJMcNab]

I think the "make the operations no-ops" and "TextStyle" changes should both be follow-ups. I believe that the other review comments have been addressed.

I've re-requested review from @jaredoconnell because there are some small potential sources of behaviour change since that review.

[tomcur]

Looks good. I've tested vello_editor, which works well. I think it's fair to bump some of the other considerations to different PRs.

This does now show some funny font selection behavior, but I believe that's not introduced in this PR. Whitespace gets a different font than the Latin characters, which makes the underlines jarring as the offsets and widths are different for the letters and the whitespace. If I'm the only one seeing this maybe it's just my system (I've had weird font selection with parley/fontique on my system before).

[nicoburns]

Happy if the remaining bits are done as followups.