Closed Bug 912951 Opened 6 years ago Closed 6 years ago

supportsSwitching API should return the real value

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:koi+, b2g-v1.2 fixed)

RESOLVED FIXED
1.2 FC (16sep)
blocking-b2g koi+
Tracking Status
b2g-v1.2 --- fixed

People

(Reporter: rudyl, Assigned: janjongboom)

References

Details

(Whiteboard: [ft:system-platform])

Attachments

(2 files, 3 obsolete files)

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.
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.
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.
(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.
Assignee: nobody → janjongboom
Whiteboard: [ft:system-platform]
Attached patch Gaia patchSplinter Review
f? for kgrandon for the desktop helper part.
Attachment #801604 - Flags: review?(rlu)
Attachment #801604 - Flags: feedback?(kgrandon)
Attached patch Gecko patch (obsolete) — Splinter Review
Please note that this bug depends on bug 906096 landing.
Attachment #801606 - Flags: review?(xyuan)
Depends on: 906096
Attached patch Gecko patch (obsolete) — Splinter Review
Attachment #801606 - Attachment is obsolete: true
Attachment #801606 - Flags: review?(xyuan)
Attachment #801611 - Flags: review?(xyuan)
Comment on attachment 801604 [details] [diff] [review]
Gaia patch

Without too much context, this makes sense to me. Thanks for the flag!
Attachment #801604 - Flags: feedback?(kgrandon) → feedback+
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.
Attachment #801606 - Flags: review-
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.
Attachment #801611 - Flags: review?(xyuan) → review-
Attached patch Gecko patch v2 (obsolete) — Splinter Review
Attachment #801611 - Attachment is obsolete: true
Attachment #802190 - Flags: review?(xyuan)
(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 on attachment 802190 [details] [diff] [review]
Gecko patch v2

r=me.
Attachment #802190 - Flags: review?(xyuan) → review+
Thanks Yuan. And another patch in queue waiting for bug 906096 :p
Comment on attachment 801604 [details] [diff] [review]
Gaia patch

Looks good to me.

Jan,

Thanks for working on this.
Attachment #801604 - Flags: review?(rlu) → review+
(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.
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.
Here is the patch rebased against mc.
Attachment #802190 - Attachment is obsolete: true
Attachment #802957 - Flags: review+
I've updated the gaia patch so it works against current FF Nightly \o/ (copy-paste FTW)
Try is green
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9a70b1360b8f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2 FC (16sep)
Thanks Ryan!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Relanded in https://github.com/mozilla-b2g/gaia/commit/8d4bb1dc63a66908ab6df2a74e6b6df591605700
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
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?
Flags: needinfo?(janjongboom)
Hmm, I think that's because I build with 906096 included... We fixed that there... Shall I create a follow up?
Flags: needinfo?(janjongboom)
(/me backporting change and building gecko, let's see if it fixes it)
Depends on: 916740
No longer depends on: 906096
Created bug 916740 as follow up. Sorry, my bad...
Jan,

Thanks for your quick response on this.
I'll give it a try with your patch if I got some free cycles today.
Merged before branching.
blocking-b2g: koi? → koi+
You need to log in before you can comment on or make changes to this bug.