[PATCH] weston-editor: Don't copy the preedit string before inserting it

[Silvan Jegen]

Hi Bryce

And thanks for the review!

On Fri, Nov 18, 2016 at 1:20 AM, Bryce Harrington <bryce at osg.samsung.com> wrote:
> On Thu, Nov 17, 2016 at 09:43:05PM +0100, Silvan Jegen wrote:
>> Signed-off-by: Silvan Jegen <s.jegen at gmail.com>
>> ---
>>  clients/editor.c | 8 +-------
>>  1 file changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/clients/editor.c b/clients/editor.c
>> index 6805d8a..1ed3eec 100644
>> --- a/clients/editor.c
>> +++ b/clients/editor.c
>> @@ -944,16 +944,10 @@ text_entry_reset_preedit(struct text_entry *entry)
>>  static void
>>  text_entry_commit_and_reset(struct text_entry *entry)
>>  {
>> -     char *commit = NULL;
>> -
>>       if (entry->preedit.commit)
>> -             commit = strdup(entry->preedit.commit);
>> +             text_entry_insert_at_cursor(entry, entry->preedit.commit, 0, 0);
>>
>>       text_entry_reset_preedit(entry);
>> -     if (commit) {
>> -             text_entry_insert_at_cursor(entry, commit, 0, 0);
>> -             free(commit);
>> -     }
>
> This essentially swaps the order of text_entry_reset_preedit() and
> text_entry_insert_at_cursor().  Does this cause any side effects?
>
> text_entry_insert_at_cursor() calls text_entry_update_layout(), which
> will behave differently if there is a preedit in effect with and without
> this patch.  I'm not super familiar with this code, but on a quick skim
> it looks like with this patch applied, any existing pre-edits will be
> applied and finalized before the reset, whereas previously the pre-edits
> would be discarded?  Am I interpreting this correctly?  If this is the

You are right. I missed that text_entry_reset_preedit() also resets
entry->preedit.text (not only preedit.commit). When I change the
function call order like this, text_entry_update_layout() will
concatenate the entry->text and the preedit.text and commit it which
results in the not-yet-committed pre-edit string being applied.


> case, and if that change of behavior is desireable, make sure the
> behavioral change (and rationale for why it's being done) is documented
> in the changelog.  If I'm not interpreting it correctly, accept my
> apologies and I look forward to your elucidation (which probably also
> would be worth referencing in the changelog entry).

I tested the change and did not notice any regression (i. e. was not
tripped up by any strange/unexpected behavior). Looking at the code
now however, text_input_leave() calls text_entry_commit_and_reset()
which would mean that the not-committed pre-edit text would get
applied when the text entry area loses focus which is almost
definitely not what one would want.

Please ignore this patch because it would result in undesirable behaviour.


Cheers,

Silvan