Re: [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  wrote:
> On Thu, Nov 17, 2016 at 09:43:05PM +0100, Silvan Jegen wrote:
>> Signed-off-by: Silvan Jegen 
>> ---
>>  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
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Bryce Harrington]

On Thu, Nov 17, 2016 at 09:43:05PM +0100, Silvan Jegen wrote:
> Signed-off-by: Silvan Jegen 
> ---
>  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
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).

Thanks,
Bryce

>   zwp_text_input_v1_reset(entry->text_input);
>   text_entry_update(entry);
> -- 
> 2.10.2
> 
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Silvan Jegen]

Signed-off-by: Silvan Jegen 
---
 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);
-   }
 
zwp_text_input_v1_reset(entry->text_input);
text_entry_update(entry);
-- 
2.10.2

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel