Define 2 different symbol panels that includes ',' or not.

RESOLVED FIXED in Firefox OS v2.1

Status

Firefox OS
Gaia::Keyboard
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: rudyl, Assigned: rudyl)

Tracking

(Depends on: 1 bug)

unspecified
2.1 S2 (15aug)
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(b2g-v2.1 fixed)

Details

(Whiteboard: [p=2])

Attachments

(3 attachments)

According to the latest UX spec in Bug 983043, we need to show 
  1. When there are multiple keyboard layouts - 
     [IME switching key] in the bottom row of the symbol panel.

  2. When there is only one layout available, show ',' in the bottom row.

See p.10 - p.11 of the spec for the details.

And this implies we need to define 2 different types of symbol, one with ',' and the other w/o ',' or think of a better way to describe this in layout definition file.
Blocks: 1015309

Comment 1

3 years ago
> or think of a better way to describe this in layout definition file.

Maybe something like:

{ value: ',', visible: ['single'] }, { value: '+', visible: ['multi'] }

Comment 2

3 years ago
Created attachment 8446806 [details] [review]
patch to add multiIME field at keys

Hi Rudy,

this patch add support to multiIME field at key so that when using Multi IME the value of this field will replace the field value. Do you like this idea?
Attachment #8446806 - Flags: feedback?(rlu)
Duplicate of this bug: 1030933
Depends on: 1031183
Raniere,

Thanks for submitting the patch, I will find some time to take a look.
BTW, I have an idea about how to auto test this kind of changes, if you're interested, take a look at bug 1031183.
Assignee: nobody → ra092767
Status: NEW → ASSIGNED
Comment on attachment 8446806 [details] [review]
patch to add multiIME field at keys

Hi Raniere,

First of all, sorry for the delay to give feedback on this patch.
I think the approach to add 'multiIME" field should be working.
However, I also noticed an issue that the keycode sent might be incorrect when there are multiple IME present, for example, when you press ',' on the 3rd row, it would still output '+'.
Please help take a look at the above issue and my comments on the github.

Thanks again.
Attachment #8446806 - Flags: feedback?(rlu) → feedback+

Comment 6

3 years ago
> Please help take a look at the above issue and my comments on the github.

Issue with keycode fixed. About avoid changes at render.js, should I add a loop over all the keys looking for 'multiIME' or change the loop searching for the space key?
Flags: needinfo?(rlu)
(In reply to Raniere Silva from comment #6)
> > Please help take a look at the above issue and my comments on the github.
> 
> Issue with keycode fixed. About avoid changes at render.js, should I add a
> loop over all the keys looking for 'multiIME' or change the loop searching
> for the space key?

It would be better that render.js only handles how to generate the DOM structure for the keyboard, and we have another new module layout_manager to handle create the 'layout' data structure.
See my comment here,
https://github.com/mozilla-b2g/gaia/pull/21083/files#r14569174
Flags: needinfo?(rlu)
The link to my GH comment should be,
https://github.com/mozilla-b2g/gaia/pull/21083#discussion_r14569180
Comment on attachment 8446806 [details] [review]
patch to add multiIME field at keys

Hi Tim,

If possible, we would like to get your feedback on this, since this would potentially touch the code in layout related code, and not only render.js and layout definition.

Thanks.
Attachment #8446806 - Flags: feedback?(timdream)

Comment 10

3 years ago
Rudy,

I rebase and update the patch. It don't do any change at render.js and handle the case of "complex" multiIME key. Can you take a look on it?

Thanks.
Flags: needinfo?(rlu)
https://bug983043.bugzilla.mozilla.org/attachment.cgi?id=8448520#12
Comment on attachment 8446806 [details] [review]
patch to add multiIME field at keys

I don't see any render.js code change. I assume they are not needed.

Thanks for the patch, below is my feedback:

1. I don't like the idea keep piling up special properties on keys, but after spending some time thinking about it I don't think we have a choice (other than NOT implementing this feature), so doing this is probably a sane idea.
2. Please make the naming more descriptive. |multiIME| doesn't ring any bell without |git blame| and locating this bug. I recommend |supportsSwitchingValue|, and the parsing function need to support |supportsSwitchingKeyCode| too.
3. You are overwriting the object itself without creating a prototype -- please look at the above code to see what is needed to prevent the original row/key/keys array/object from being modified. Sadly we probably need to copy all arrays (not just the |spaceKeyRow|) just for this feature.
4. Please include unit tests. Since the layout definition parsing rule got more and more complicated, unit tests is needed to ensure future modifications will not break your feature here.

This patch is conflict with part 1 of bug 1029356. Depend on what Rudy want to do, I think I can land that patch in another bug with a quicker review pass for you to rebase your patch on top of it.
Attachment #8446806 - Flags: feedback?(timdream)
Attachment #8446806 - Flags: feedback-
Attachment #8446806 - Flags: feedback+
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #12)
> This patch is conflict with part 1 of bug 1029356. Depend on what Rudy want
> to do, I think I can land that patch in another bug with a quicker review
> pass for you to rebase your patch on top of it.

Sorry, forget this part. I was confused that with part 1 of bug 1023730 which is already landed.
Oh, I forget to mention I f- the patch because the patch didn't actually modify the layout according to the spec -- it looks like ± is replaced with + but the position is not correct.

Comment 15

3 years ago
> I don't see any render.js code change.

I update the patch before you look on it.

> 2. Please make the naming more descriptive. |multiIME| doesn't ring any bell without |git blame| and locating this bug. I recommend |supportsSwitchingValue|, and the parsing function need to support |supportsSwitchingKeyCode| too.

Rudy already raise the support for key code [1] using:

{
   value: '+',
   multiIME: {     // alternative for multIME
           // changed to some special, say [Backspace]
           value: '⌫', ratio: 1.5, keyCode: KeyEvent.DOM_VK_BACK_SPACE
    } 
}

And I had change the patch based on Rudy's example.

I couldn't think of a case where only the key code change so based on Rudy's example I still looking for a suitable name.

[1] https://github.com/mozilla-b2g/gaia/pull/21083#discussion_r14569174
(In reply to Raniere Silva from comment #15)
> > I don't see any render.js code change.
> 
> I update the patch before you look on it.
> 
> > 2. Please make the naming more descriptive. |multiIME| doesn't ring any bell without |git blame| and locating this bug. I recommend |supportsSwitchingValue|, and the parsing function need to support |supportsSwitchingKeyCode| too.
> 
> Rudy already raise the support for key code [1] using:
> 
> {
>    value: '+',
>    multiIME: {     // alternative for multIME
>            // changed to some special, say [Backspace]
>            value: '⌫', ratio: 1.5, keyCode: KeyEvent.DOM_VK_BACK_SPACE
>     } 
> }
> 
> And I had change the patch based on Rudy's example.
> 
> I couldn't think of a case where only the key code change so based on Rudy's
> example I still looking for a suitable name.
> 
> [1] https://github.com/mozilla-b2g/gaia/pull/21083#discussion_r14569174

Yes, basically, this means we need a way to define an alternative to some key in "some cases", so my proposal would still be,

supportsSwitching Case
==
key definition
{
  value: '+',
  supportsSwitchingValue: {
    value: ','
    // may include other properties of a normal key
  }
}

==
This would bring a nested structure to key def., but I think we won't have multi-level nested, so it should be fine.
For the record, we also have 'textLayoutOverwrite' property used in some Chinese layouts, but that is used to affect the "modifyLayout()" logic, so the use case is different.
Flags: needinfo?(rlu)
Priority: -- → P1

Comment 17

3 years ago
Created attachment 8465407 [details]
video description of issue with patch

Rudy,

do you have any suggestion of how to solve the issue presented in this video where I used https://github.com/r-gaia-cs/gaia/tree/bug-1022609 ?
Flags: needinfo?(rlu)
I guess you were hit by a similar situation like I did,
see https://github.com/mozilla-b2g/gaia/pull/22204#discussion_r15452364

That is to say, we shouldn't modify a key directly, and should do a copy first like what we did to the spaceKey and [Enter] key in my patch of bug 1020779.

So, looks like this bug should wait for bug 1020779 to land, or it may cause conflicts.
Flags: needinfo?(rlu)

Comment 19

3 years ago
Comment on attachment 8446806 [details] [review]
patch to add multiIME field at keys

Omega, could you review this patch?

Rudy, probably not the best way to solve the issue describe at the video but works. Could you review this patch?
Attachment #8446806 - Flags: ui-review?(ofeng)
Attachment #8446806 - Flags: feedback?(rlu)
Attachment #8446806 - Flags: feedback-
Comment on attachment 8446806 [details] [review]
patch to add multiIME field at keys

I don't check all layouts, but it works well on English one.
Attachment #8446806 - Flags: ui-review?(ofeng) → ui-review+
Comment on attachment 8446806 [details] [review]
patch to add multiIME field at keys

As I mentioned in my mail, this would conflict with bug 1020779, so need your help to rebase the work on that.
BTW, please help address the unit tests of layout manager, and you will need to add some test cases for this new feature.
https://tbpl.mozilla.org/?rev=d87e6fd764877624fced23f52cd10d92cbdc1593&tree=Gaia-Try
Attachment #8446806 - Flags: feedback?(rlu) → feedback-
Take this since this is one of the features for v2.1 and already talked to Raniere about this.
Assignee: ra092767 → rlu
Status: ASSIGNED → NEW
Target Milestone: --- → 2.1 S2 (15aug)
Whiteboard: [p=1]
Status: NEW → ASSIGNED
Created attachment 8470500 [details] [review]
Patch V1

WIP here.

For 'en' layout, we have |+ : ; " ' ...| defined as the bottom row of the symbol panel and we would replace + as ',' when we show IME switching key.

However, this issue would involve several layout definition files, and we need to figure out how to handle various layout files that don't have '+' included in the bottom row.
Comment on attachment 8470500 [details] [review]
Patch V1

Omega, as our offline discussion, it would help if you could help review the layout definition files in https://github.com/RudyLu/gaia/commit/5d455ce3a193bf5982e4716c8a5f6adc74a125d1

This is because, these layouts include self-defined alternateLayout (symbol panel 1), and it might not have '+' at the bottom row, so we could not replace it with ',' when we have IME switching shown.
BTW, this also indicate an issue I mentioned earlier that we introduce a dependency between symbol 1 and symbol 2, which makes reusing symbol 2 while customizing symbol 1 harder.

Let me know if you have any questions and we can talk about this face-to-face.
Thanks.
Attachment #8470500 - Flags: feedback?(ofeng)
Modify the story point because we may need to review several layouts, see comment 24.
Whiteboard: [p=1] → [p=2]
Please see bug 983043 for UX spec update.
I removed Symbol 2 dependence as offline discussion, so the "comma rule" can be applied to all layouts easier.
For layouts without customized symbol panel: just use the same rules as English. (e.g. Français)
For layouts with partially customized symbol panel 1: just use the same rules as English. (e.g. Português)
For layouts with fully customized symbol panel 1: use the similar rules as English. (e.g. Español)
For layouts with fully customized both symbol panels: always put comma in the bottom row of symbol panels. (e.g. English - Neo)
Attachment #8470500 - Flags: feedback?(ofeng)
Comment on attachment 8470500 [details] [review]
Patch V1

I think this is ready for review.

Tim, could you please take a look at this patch?
Please help look into only the 2nd commit,
https://github.com/RudyLu/gaia/commit/981335b606bf5e821086e6f4a8ac31d53a293eed

Omega, thanks for the spec update, and please help do ui review.
Attachment #8470500 - Attachment description: WIP → Patch V1
Attachment #8470500 - Flags: ui-review?(ofeng)
Attachment #8470500 - Flags: review?(timdream)
Comment on attachment 8470500 [details] [review]
Patch V1

Good job, thanks!
Attachment #8470500 - Flags: ui-review?(ofeng) → ui-review+
(In reply to Rudy Lu [:rudyl] from comment #27)
> Comment on attachment 8470500 [details] [review]
> Patch V1
> 
> I think this is ready for review.
> 
> Tim, could you please take a look at this patch?
> Please help look into only the 2nd commit,
> https://github.com/RudyLu/gaia/commit/
> 981335b606bf5e821086e6f4a8ac31d53a293eed

Tim,

I just rebase this patch onto master, so please review this pull request directly.
Thanks.

> 
> Omega, thanks for the spec update, and please help do ui review.
Comment on attachment 8470500 [details] [review]
Patch V1

Rabbi, 

This patch would also need your help to give feedback on this change,
https://github.com/mozilla-b2g/gaia/pull/22700/files#diff-1f88fa88f09871c1d55a2a9292f421b8R85

This is because we have '_' on the original symbol panel 2, but now '_' has been moved to symbol panel 1, so we need to have it in the customized symbol panel 1.

Thanks.
Attachment #8470500 - Flags: feedback?(me)

Comment 31

3 years ago
(In reply to Rudy Lu [:rudyl] from comment #30)

> This patch would also need your help to give feedback on this change,
> https://github.com/mozilla-b2g/gaia/pull/22700/files#diff-
> 1f88fa88f09871c1d55a2a9292f421b8R85
> 
> This is because we have '_' on the original symbol panel 2, but now '_' has
> been moved to symbol panel 1, so we need to have it in the customized symbol
> panel 1.

As you are talking about Dz-BT layout, It seems everything is OK for me. When building that layout, I keep in mind to have all the Dz-BT symbols in the keyboard. So, "_" doesn't matter for me. And I think it is OK. Any more information needed?
(In reply to Rabbi Hossain from comment #31)
> (In reply to Rudy Lu [:rudyl] from comment #30)
> 
> > This patch would also need your help to give feedback on this change,
> > https://github.com/mozilla-b2g/gaia/pull/22700/files#diff-
> > 1f88fa88f09871c1d55a2a9292f421b8R85
> > 
> > This is because we have '_' on the original symbol panel 2, but now '_' has
> > been moved to symbol panel 1, so we need to have it in the customized symbol
> > panel 1.
> 
> As you are talking about Dz-BT layout, It seems everything is OK for me.
> When building that layout, I keep in mind to have all the Dz-BT symbols in
> the keyboard. So, "_" doesn't matter for me. And I think it is OK. Any more
> information needed?

No, that's good to know. I would take your answer as as feedback+, right?
Attachment #8470500 - Flags: review?(timdream) → review+
Blocks: 1019472
Comment on attachment 8470500 [details] [review]
Patch V1

Took comment 31 as a feedback+.
Attachment #8470500 - Flags: feedback?(me) → feedback+
Landed to Gaia master,
https://github.com/mozilla-b2g/gaia/commit/517253f887a093d31a5ff35bd31df1ae483c89a2
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-b2g-v2.1: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.