Make nsGtkIMModule simpler

RESOLVED FIXED in mozilla36

Status

()

Core
Widget: Gtk
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

({inputmethod})

Trunk
mozilla36
x86_64
Linux
inputmethod
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments, 1 obsolete attachment)

10.26 KB, patch
m_kato
: review+
Details | Diff | Splinter Review
5.22 KB, patch
m_kato
: review+
Details | Diff | Splinter Review
3.43 KB, patch
m_kato
: review+
Details | Diff | Splinter Review
5.20 KB, patch
m_kato
: review+
Details | Diff | Splinter Review
21.14 KB, patch
m_kato
: review+
Details | Diff | Splinter Review
4.49 KB, patch
m_kato
: review+
Details | Diff | Splinter Review
17.01 KB, patch
m_kato
: review+
Details | Diff | Splinter Review
9.08 KB, patch
m_kato
: review+
Details | Diff | Splinter Review
2.78 KB, patch
m_kato
: review+
Details | Diff | Splinter Review
2.55 KB, patch
m_kato
: review+
Details | Diff | Splinter Review
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.
Depends on: 1083098
Depends on: 1083629
Depends on: 1084302
Created attachment 8517483 [details] [diff] [review]
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.
Attachment #8517483 - Flags: review?(karlt)
Created attachment 8517486 [details] [diff] [review]
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.
Attachment #8517486 - Flags: review?(karlt)
Created attachment 8517487 [details] [diff] [review]
part.3 Remove nsGtkIMModule::CommitCompositionBy()

And also, nsGtkIMModule::CommitCompositionBy() isn't necessary since it just calls DispatchCompsotionEventsForCOmmit().
Attachment #8517487 - Flags: review?(karlt)
Created attachment 8517491 [details] [diff] [review]
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().
Attachment #8517491 - Flags: review?(karlt)
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().
Attachment #8517524 - Flags: review?(karlt)
Created attachment 8517525 [details] [diff] [review]
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.
Attachment #8517525 - Flags: review?(karlt)
(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.
Created attachment 8517529 [details] [diff] [review]
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.
Attachment #8517529 - Flags: review?(karlt)
Created attachment 8517531 [details] [diff] [review]
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().
Attachment #8517531 - Flags: review?(karlt)
Created attachment 8517535 [details] [diff] [review]
part.9 Remove nsGtkIMModule::IsVirtualKeyboardOpened() because it always returns false

nsGtkIMModule::IsVirtualKeyboardOpened() was implemented for Maemo. So, we don't need this method.
Attachment #8517535 - Flags: review?(karlt)
Created attachment 8517538 [details] [diff] [review]
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.
Attachment #8517538 - Flags: review?(karlt)
Created attachment 8517895 [details] [diff] [review]
part.10 nsGtkIMModule should use NS_INLINE_DECL_REFCOUNTING

The old patch caused bustage of Bd, Bo and S due to public destructor defined.
Attachment #8517538 - Attachment is obsolete: true
Attachment #8517538 - Flags: review?(karlt)
Attachment #8517895 - Flags: review?(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.
(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 on attachment 8517483 [details] [diff] [review]
part.1 Create nsGtkIMModule::DispatchCompositionEventsForCommit()

See comment 1 for the detail.
Attachment #8517483 - Flags: review?(karlt) → review?(m_kato)
Comment on attachment 8517486 [details] [diff] [review]
part.2 Remove aIsCommit of nsGtkIMModule::DispatchCompositionChangeEvent()

See comment 2 for the detail.
Attachment #8517486 - Flags: review?(karlt) → review?(m_kato)
Comment on attachment 8517487 [details] [diff] [review]
part.3 Remove nsGtkIMModule::CommitCompositionBy()

See comment 3 for the detail.
Attachment #8517487 - Flags: review?(karlt) → review?(m_kato)
Comment on attachment 8517491 [details] [diff] [review]
part.4 Use IMEContext::MaybeEditable() instead of nsGtkIMModule::IsEditable()

See comment 4 for the detail.
Attachment #8517491 - Flags: review?(karlt) → review?(m_kato)
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.
Attachment #8517524 - Flags: review?(karlt) → review?(m_kato)
Comment on attachment 8517525 [details] [diff] [review]
part.6 Make nsGtkIMModule::CreateTextRangeArray() stateless

See comment 6 for the detail.
Attachment #8517525 - Flags: review?(karlt) → review?(m_kato)
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.
Attachment #8517529 - Flags: review?(karlt) → review?(m_kato)
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.
Attachment #8517531 - Flags: review?(karlt) → review?(m_kato)
Comment on attachment 8517535 [details] [diff] [review]
part.9 Remove nsGtkIMModule::IsVirtualKeyboardOpened() because it always returns false

See comment 10 for the detail.
Attachment #8517535 - Flags: review?(karlt) → review?(m_kato)
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.
Attachment #8517895 - Flags: review?(karlt) → review?(m_kato)
Attachment #8517483 - Flags: review?(m_kato) → review+
Attachment #8517486 - Flags: review?(m_kato) → review+
Attachment #8517487 - Flags: review?(m_kato) → review+
Attachment #8517491 - Flags: review?(m_kato) → review+
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...
Attachment #8517535 - Flags: review?(m_kato) → review+
Attachment #8517895 - Flags: review?(m_kato) → review+
(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 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
Attachment #8517524 - Flags: review?(m_kato) → review+
Attachment #8517525 - Flags: review?(m_kato) → review+
Attachment #8517529 - Flags: review?(m_kato) → review+
Attachment #8517531 - Flags: review?(m_kato) → review+
Thank you, Kato-san.
Thank you, Kato-san!
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
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
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1097238

Updated

3 years ago
Depends on: 1143197
You need to log in before you can comment on or make changes to this bug.