[Linux] composing delta fixes

[Renzo-Olivares]

Fixes a few issues with the Linux text editing delta implementation that was causing some issues when composing CJK text.

im_preedit_changed_cb dispatches a delta with the delta range set as the current composing region causing a crash when composing because the new composing region range might not yet exist in the oldText that the delta is applied to. We should instead send the composing region before the change to the text as that will be the correct range that is being modified in the oldText.

im_preedit_end_cb dispatches a delta with an empty oldText, a not empty deltaText, and a delta range of (-1,-1) causing a crash when composing ends because the delta range will never be within the range of the text. We should instead send the current text value as the oldText and not send a deltaRange or deltaText since a (-1,-1) range essentially means nothing in the text changed.

im_commit_cb does not account for the case when we were previously composing when the text was committed. This causes the dispatched delta to have a delta range that always inserts at the current the selection, when it should instead replace the previous composing range. We should account for the case when we are previously composing before committing the text, and dispatch a delta with the previous composing region.

Fixes flutter/flutter#113909

Pre-launch Checklist

• I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
• I read the [Tree Hygiene] wiki page, which explains my responsibilities.
• I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
• I listed at least one issue that this PR fixes in the description above.
• I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on writing and running engine tests.
• I updated/added relevant documentation (doc comments with ///).
• I signed the [CLA].
• All existing and new tests are passing.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[cbracken]

Possibly more succinct:

flutter::TextRange replace_range = was_composing ? composing_before_change : selection_before_change;
std::unique_ptr<flutter::TextEditingDelta> delta =
    std::make_unique<flutter::TextEditingDelta>(text_before_change, replacement_range, text);
  

[cbracken]

Avoid new/delete in C++ code. Use std::unique_ptr<Foo>, which you can create with std::make_unique<Foo>(constructor, args, go, here);

[justinmc]

@cbracken What's the reasoning behind this? (for my own education)

[cbracken]

Generally always use RAII-based smart pointers (primarily unique_ptr) unless it's impossible, they're guaranteed to clean themselves up at end of scope whether it's a return or throw, and they're effectively zero overhead.

In this case there's only one way out of the scope, but there's a function call in there that could do something crazy like throw (but almost certainly doesn't 😉). The more ways out of a scope the more deletes you'll need to make sure not to forget. Smart pointers guarantee cleanup so why even risk it?

In modern c++ new/delete are generally considered bad practice unless absolutely necessary. In our codebase there are maybe two spots I've found where we couldn't use either unique_ptr or shared_ptr, and we still have a new/delete, but we should avoid adding any more.

More reading in the C++ FAQ and C++ Core Guidelines.

[cbracken]

Suggested change

	gboolean wasComposing = priv->text_model->composing();
	gboolean was_composing = priv->text_model->composing();

[cbracken]

I'm surprised this works. The type of the base/extent positions are size_t, which is unsigned. I'm going to take a quick look at what options we have for modelling this properly.

[cbracken]

Here's an incomplete prototype of something we could use to do this safely:
https://github.com/flutter/engine/compare/main...cbracken:flutter_engine:add-invalid-range?expand=1

If this seems useful, I could commit it, but when I added TextRange originally my thought was to see how far along we could make it without modelling things using invalid positions like -1 since some platforms (macOS, iOS) don't model any positions with negative numbers, so we're going to need to do translation between models somewhere regardless and TextRange was cleaner without the concept of negative values.

If we think this is something that we absolutely need to model in the embedders, I could clean this up and update the existing TextRange-using embedders to use it.

[Renzo-Olivares]

I think in this case it may be correct to use composing_before_change. So this may not be needed. Thanks for checking that out!

[Renzo-Olivares]

correction, a delta range of (-1,-1), is needed but we should just use TextEditingDelta(const std::u16string & text) for this case.

[justinmc]

LGTM, but I'm interested if @cbracken has a better way to represent the (-1,-1) TextRange.

Thanks for fixing these, I probably introduced these bugs when I wrote this 😁 .

[justinmc]

@cbracken What's the reasoning behind this? (for my own education)

[cbracken]

Thanks for patching! Added a few comments/suggestions.

[cbracken]