1083067 - Make nsGtkIMModule simpler

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Currently, nsGtkIMModule respect native composition events ("preedit_end" signals) for dispatching NS_COMPOSITION_END. This becomes the implementation complicated.

On the other platforms, typically, when composition string becomes empty or committed, NS_COMPOSITION_END is also dispatched. So, it's no problem to change the behavior as so.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Create nsGtkIMModule::DispatchCompositionEventsForCommit()

This is a preparation of bug 1077345.

Dispatching NS_COMPOSITION_END has to follow NS_COMPOSITION_CHANGE whose TextRange information is null. So, these events should be represented by an event. That is what I'm working on bug 1077345.

For making the patch for bug 1077345 simpler, nsGtkIMModule should have a method which dispatches both NS_COMPOSITION_CHANGE without TextRange information and NS_COMPOSITION_END at committing composition.

This patch creates nsGtkIMModule::DispatchCompositionEventsForCommit() for it.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 Remove aIsCommit of nsGtkIMModule::DispatchCompositionChangeEvent()

By the part.1, DispatchCompositionChangeEvent() doesn't need to take aIsCommit as its argument since NS_COMPOSITION_CHANGE event for commit is dispatched by DispatchCompositionEventsForCommit() and DispatchCompositionChangeEvent() is now used only when composing string is updated.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.3 Remove nsGtkIMModule::CommitCompositionBy()

And also, nsGtkIMModule::CommitCompositionBy() isn't necessary since it just calls DispatchCompsotionEventsForCOmmit().

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.4 Use IMEContext::MaybeEditable() instead of nsGtkIMModule::IsEditable()

nsGtkIMModule::IsEnabled() is now bad name since IMEState::IsEnabled() returns different value when the enabled state is PLUGIN.

So, IMEState should have new method for GTK widget. It should be named as  MaybeEditable().

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.5 Each IM signal handlers should take GtkIMContext which is being handled

This is a follow up patch of bug 1083098.

For making GetContext() clearer what it does, let's rename it to GetCurrentContext().

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.6 Make nsGtkIMModule::CreateTextRangeArray() stateless

nsGtkIMModule::CreateTextRangeArray() depends on mDispatchedCompositionString but this isn't good due to stateful. The caller should give it with its argument. Then, we can make it stateless.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #5)
> Created attachment 8517524 [details] [diff] [review]
> part.5 Each IM signal handlers should take GtkIMContext which is being
> handled
> 
> This is a follow up patch of bug 1083098.
> 
> For making GetContext() clearer what it does, let's rename it to
> GetCurrentContext().

Oops, this explanation is for part.7. part.5 makes IME signal handlers take GtkIMContext which is specified by signal.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.7 Rename GetContext() to GetCurrentContext() for making clearer the meaning of its result

This renames GetContext() to GetCurrentContext(). The new name is better for explaining what it does.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.8 Create nsGtkIMModule::GetActiveContext() which prefers a composition context if there is

Some methods should use composing context rather than current context if there is a composition.

While there is a composition, nsGtkIMModule should store the composing context and GetActiveContext() should return it if there is. Otherwise, they should use the result of GetCurrentContext().

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.9 Remove nsGtkIMModule::IsVirtualKeyboardOpened() because it always returns false

nsGtkIMModule::IsVirtualKeyboardOpened() was implemented for Maemo. So, we don't need this method.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.10 nsGtkIMModule should use NS_INLINE_DECL_REFCOUNTING

nsGtkIMModule is refcountable class but its AddRef() and Release() are implemented without NS_INLINE_DECL_REFCOUNTING. This is bad if some rules of refcounting will be changed. So, we should use the macro.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.10 nsGtkIMModule should use NS_INLINE_DECL_REFCOUNTING

The old patch caused bustage of Bd, Bo and S due to public destructor defined.

Comment 13

[Karl Tomlinson (:karlt)]

Is there someone else who might be able to review these changes?
I have a backlog of requests and fear that I won't be able to review these soon, sorry.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Karl Tomlinson (:karlt) from comment #13)
> Is there someone else who might be able to review these changes?
> I have a backlog of requests and fear that I won't be able to review these
> soon, sorry.

Okay, Kato-san will help this. I'll request the reviews to him. But if you find some bad points, let me know.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8517483 [details] [diff] [review]
part.1 Create nsGtkIMModule::DispatchCompositionEventsForCommit()

See comment 1 for the detail.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8517486 [details] [diff] [review]
part.2 Remove aIsCommit of nsGtkIMModule::DispatchCompositionChangeEvent()

See comment 2 for the detail.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8517487 [details] [diff] [review]
part.3 Remove nsGtkIMModule::CommitCompositionBy()

See comment 3 for the detail.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8517491 [details] [diff] [review]
part.4 Use IMEContext::MaybeEditable() instead of nsGtkIMModule::IsEditable()

See comment 4 for the detail.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8517524 [details] [diff] [review]
part.5 Each IM signal handlers should take GtkIMContext which is being handled

See comment 7 for the detail.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8517525 [details] [diff] [review]
part.6 Make nsGtkIMModule::CreateTextRangeArray() stateless

See comment 6 for the detail.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8517529 [details] [diff] [review]
part.7 Rename GetContext() to GetCurrentContext() for making clearer the meaning of its result

See comment 8 for the detail.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8517531 [details] [diff] [review]
part.8 Create nsGtkIMModule::GetActiveContext() which prefers a composition context if there is

See comment 9 for the detail.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8517535 [details] [diff] [review]
part.9 Remove nsGtkIMModule::IsVirtualKeyboardOpened() because it always returns false

See comment 10 for the detail.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8517895 [details] [diff] [review]
part.10 nsGtkIMModule should use NS_INLINE_DECL_REFCOUNTING

See comment 11 and comment 12 for the detail.

Comment 25

[Makoto Kato [:m_kato]]

Comment on attachment 8517524 [details] [diff] [review]
part.5 Each IM signal handlers should take GtkIMContext which is being handled

Review of attachment 8517524 [details] [diff] [review]:
-----------------------------------------------------------------

Although you add a parameter as GtkIMContext, is it really needed for logging?

::: widget/gtk/nsGtkIMModule.cpp
@@ +952,1 @@
>  

Is this aContext for logging only? If not logging mode, it seems to be unnecessary...

@@ +1098,2 @@
>                     const nsAString& aCommitString)
>  {

Is this aContext for logging only?  If not logging mode, it seems to be unnecessary...

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Makoto Kato (:m_kato) from comment #25)
> Comment on attachment 8517524 [details] [diff] [review]
> part.5 Each IM signal handlers should take GtkIMContext which is being
> handled
> 
> Review of attachment 8517524 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Although you add a parameter as GtkIMContext, is it really needed for
> logging?
> 
> ::: widget/gtk/nsGtkIMModule.cpp
> @@ +952,1 @@
> >  
> 
> Is this aContext for logging only? If not logging mode, it seems to be
> unnecessary...
> 
> @@ +1098,2 @@
> >                     const nsAString& aCommitString)
> >  {
> 
> Is this aContext for logging only?  If not logging mode, it seems to be
> unnecessary...

Yes. I think that Dispatch*() should have aContext its argument for preventing future's bug. If other developers will use Get(Current)Context() or GetActiveContext(), it may cause a bug due to mismatching context. I'd like to keep them there, but if you don't like them strongly, I'll remove them.

Comment 27

[Makoto Kato [:m_kato]]

Comment on attachment 8517524 [details] [diff] [review]
part.5 Each IM signal handlers should take GtkIMContext which is being handled

Review of attachment 8517524 [details] [diff] [review]:
-----------------------------------------------------------------

agreed

Comment 28

[Karl Tomlinson (:karlt)]

Thank you, Kato-san.

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you, Kato-san!

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da7bac23b9f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b269294a816
https://hg.mozilla.org/integration/mozilla-inbound/rev/58c2cccc6aea
https://hg.mozilla.org/integration/mozilla-inbound/rev/43e7ac06b9b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/068fc29a0189
https://hg.mozilla.org/integration/mozilla-inbound/rev/b458a9cf7416
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a5b0e13ba63
https://hg.mozilla.org/integration/mozilla-inbound/rev/081b41591d86
https://hg.mozilla.org/integration/mozilla-inbound/rev/4188de0daebf
https://hg.mozilla.org/integration/mozilla-inbound/rev/1996f71aedd5

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/da7bac23b9f2
https://hg.mozilla.org/mozilla-central/rev/7b269294a816
https://hg.mozilla.org/mozilla-central/rev/58c2cccc6aea
https://hg.mozilla.org/mozilla-central/rev/43e7ac06b9b8
https://hg.mozilla.org/mozilla-central/rev/068fc29a0189
https://hg.mozilla.org/mozilla-central/rev/b458a9cf7416
https://hg.mozilla.org/mozilla-central/rev/7a5b0e13ba63
https://hg.mozilla.org/mozilla-central/rev/081b41591d86
https://hg.mozilla.org/mozilla-central/rev/4188de0daebf
https://hg.mozilla.org/mozilla-central/rev/1996f71aedd5