912951 - supportsSwitching API should return the real value

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

Description

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

This is a follow-up bug for Bug 885692.
Right now, we have InputMethodManager::supportsSwitching(), but right now it would always return true.

We would nee to make it reflect the actual value, such as "false" when there is only one layout enabled.

Comment 1

[Jan Jongboom [:janjongboom] (inactive)]

hmm, at the moment this info lives in keyboard_manager, not in Gecko. Should we do some chrome event wizardry to ask the keyboard manager? Or move logic into Gecko? I think I'd like to have it in Gaia but don't know what others think.

You can give the bug to me btw when we have clearance.

Comment 2

[Yuan Xulei [:yxl] (Leave from May 18)]

To resolve the bug, the InputMethod API of gecko needs a way to get the number of enabled keyboard layouts from gaia.

As I said in Bug 910713 comment2, one possible solution is:

Make gaia send a mozContent event to notify gecko when the keyboard layout number has changed.

Comment 3

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

(In reply to Yuan Xulei [:yxl] from comment #2)
> To resolve the bug, the InputMethod API of gecko needs a way to get the
> number of enabled keyboard layouts from gaia.
> 
> As I said in Bug 910713 comment2, one possible solution is:
> 
> Make gaia send a mozContent event to notify gecko when the keyboard layout
> number has changed.

Yes, I think this is the way to go.
Jan, can you help on this? 
Or I'll take a look at how to implement this.

Comment 4

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Gaia patch

f? for kgrandon for the desktop helper part.

Comment 5

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Gecko patch

Please note that this bug depends on bug 906096 landing.

Comment 6

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Gecko patch

Comment 7

[Kevin Grandon :kgrandon]

Comment on attachment 801604 [details] [diff] [review]
Gaia patch

Without too much context, this makes sense to me. Thanks for the flag!

Comment 8

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 801606 [details] [diff] [review]
Gecko patch

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

::: b2g/chrome/content/shell.js
@@ +719,5 @@
>      let listener = this._listeners[uid];
>      if (!listener)
>       return;
>  
> +     dump('\n\n\n!!!!!\n\n\nALERTSHELPER HANDLEEVENT\n')

Remove the log for debugging only.

::: dom/inputmethod/MozKeyboard.js
@@ +286,5 @@
>      Services.obs.addObserver(this, "inner-window-destroyed", false);
>      cpmm.addMessageListener('Keyboard:FocusChange', this);
>      cpmm.addMessageListener('Keyboard:SelectionChange', this);
>      cpmm.addMessageListener('Keyboard:GetContext:Result:OK', this);
> +    cpmm.addMessageListener('Keyboard:Layouts', this);

I think a message name of `Keyboard:LayoutsUpdate` or `Keyboard:LayoutsChange' is more consistent and accurate.

@@ +361,5 @@
>  
>      if (data) {
> +      this._mgmt._supportsSwitching = this._layouts[data.type] ?
> +        this._layouts[data.type] > 1 :
> +        false;

If inputcontext is null, switching keyboard is not allowed. So we need to set _supportsSwiching to false.

Also you need to check if |this._layouts| isn't null before using it.

Comment 9

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 801611 [details] [diff] [review]
Gecko patch

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

Please addressed the above comments.

::: dom/inputmethod/MozKeyboard.js
@@ +359,5 @@
>        this._inputcontext = null;
>      }
>  
>      if (data) {
> +      this._mgmt._supportsSwitching = this._layouts[data.type] ?

I made a mistake you don't need to check if this._layouts is null here, as it won't be null.

Comment 10

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Gecko patch v2

Comment 11

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to Yuan Xulei [:yxl] from comment #8)
> Remove the log for debugging only.
Yeah already was removed, you checked the obsoleted patch :-)

> I think a message name of `Keyboard:LayoutsUpdate` or
> `Keyboard:LayoutsChange' is more consistent and accurate.
Changed to LayoutsChange
 
> If inputcontext is null, switching keyboard is not allowed. So we need to
> set _supportsSwiching to false.
Done

Comment 12

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 802190 [details] [diff] [review]
Gecko patch v2

r=me.

Comment 13

[Jan Jongboom [:janjongboom] (inactive)]

Thanks Yuan. And another patch in queue waiting for bug 906096 :p

Comment 14

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 801604 [details] [diff] [review]
Gaia patch

Looks good to me.

Jan,

Thanks for working on this.

Comment 15

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Jan Jongboom [:janjongboom] from comment #13)
> Thanks Yuan. And another patch in queue waiting for bug 906096 :p

I think it is possible to remove the dependence from bug 906096. Don't let many bugs locked. Could you do so? I'll help you with bug 906096.

Comment 16

[Jan Jongboom [:janjongboom] (inactive)]

Yeah, I'll re-apply against mc if you can help me with 906096 :-) anyway, it won't work in FF nightly tho until that one is landed.

Comment 17

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Gecko patch against mc (v4)

Here is the patch rebased against mc.

Comment 18

[Jan Jongboom [:janjongboom] (inactive)]

Try is running @ https://tbpl.mozilla.org/?tree=Try&rev=d586c7611c44

Comment 19

[Jan Jongboom [:janjongboom] (inactive)]

I've updated the gaia patch so it works against current FF Nightly \o/ (copy-paste FTW)

Comment 20

[Jan Jongboom [:janjongboom] (inactive)]

Try is green

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Gecko: https://hg.mozilla.org/integration/b2g-inbound/rev/9a70b1360b8f
Gaia: https://github.com/mozilla-b2g/gaia/commit/49cdb9bd52c98aa256f31c2acdfd5f41d3ab175e

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9a70b1360b8f

Comment 23

[Jan Jongboom [:janjongboom] (inactive)]

Thanks Ryan!

Comment 24

[Jan Jongboom [:janjongboom] (inactive)]

Backed out gaia commit because of bug 916028 in https://github.com/mozilla-b2g/gaia/commit/4ad9fb3beafb075367017de4fdc4d85b1b52f8e0

Comment 25

[Jan Jongboom [:janjongboom] (inactive)]

Relanded in https://github.com/mozilla-b2g/gaia/commit/8d4bb1dc63a66908ab6df2a74e6b6df591605700

Comment 26

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

When testing this patch with 910713, I found I could not get it to work on device.
But using FF Nightly is ok.

Error message on Unagi
======================
09-16 17:47:10.761: E/GeckoConsole(1157): [JavaScript Error: "Keyboard is undefined" {file: "chrome://browser/content/shell.js" line: 1075}]

Jan,

Would you see this when testing on device?

Comment 27

[Jan Jongboom [:janjongboom] (inactive)]

Hmm, I think that's because I build with 906096 included... We fixed that there... Shall I create a follow up?

Comment 28

[Jan Jongboom [:janjongboom] (inactive)]

(/me backporting change and building gecko, let's see if it fixes it)

Comment 29

[Jan Jongboom [:janjongboom] (inactive)]

Created bug 916740 as follow up. Sorry, my bad...

Comment 30

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Jan,

Thanks for your quick response on this.
I'll give it a try with your patch if I got some free cycles today.

Comment 31

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Merged before branching.