Closed Bug 914459 Opened 11 years ago Closed 11 years ago

basicLayoutKey is not displayed correctly when it is defined by keyboard

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: seinlin, Assigned: seinlin)

Details

Attachments

(2 files, 3 obsolete files)

The design of keyboard gives the author the ability to change the basic layout key contents, but it is not displayed correctly when it is defined by keyboard.

Attached file:

Arabic keyboard defines basicLayoutKey,

Expect result: In alternateLayout the basicLayoutKey should displays the content defined by Arabic keyboard 

Actual result: In alternateLayout the basicLayoutKey displays "ABC".
Attached patch bug-914459-pull-request.html (obsolete) — Splinter Review
When keyboard defined basicLayoutKey, it should be used in alternateLayout and symbolLayout.
Attachment #801999 - Flags: review?(lchang)
Attached file bug-914459-pull-request.html (obsolete) —
Re-attach the file in correct format.
Attachment #801999 - Attachment is obsolete: true
Attachment #801999 - Flags: review?(lchang)
Attachment #802025 - Flags: review?(lchang)
Comment on attachment 802025 [details]
bug-914459-pull-request.html

It looks good to me. :)
However, I think it should be reviewed by the keyboard owner.

@Rudy, could you please review this patch, thanks.
Attachment #802025 - Flags: review?(rlu)
Attachment #802025 - Flags: review?(lchang)
Attachment #802025 - Flags: feedback+
Attached file bug-914459-pull-request-2.html (obsolete) —
Correct the format and send pull request again.
Attachment #802025 - Attachment is obsolete: true
Attachment #802025 - Flags: review?(rlu)
Attachment #802143 - Flags: review?(rlu)
When a keyboard defines basicLayoutKey but no alternateLayout, it will use the base alternateLayout + basicLayoutKey. Need to clean the basicLayoutKey after it is assigned, otherwise the basicLayoutKey will be used by other keyboard which do not define an alternateLayout.
Attachment #802143 - Attachment is obsolete: true
Attachment #802143 - Flags: review?(rlu)
Attachment #802817 - Flags: review?(rlu)
Comment on attachment 802817 [details]
bug-914459-pull-request-3.html

Hi Seinlin, 

I leave some comment on the pull reqeust, we might need to put the logic to replace basicLayoutKey in one place if possible.

Please help take a look and set the review flag back when you have an update.
Thanks.
Attachment #802817 - Flags: review?(rlu)
Change the assignee, since Seinlin has a WIP patch.

Hi Seinlin,

BTW, you may update the code in the same pull request if you just pushed to the same branch.
You don't have to create another pull request. :)
Assignee: nobody → kli
(In reply to Rudy Lu [:rudyl] from comment #7)
> Change the assignee, since Seinlin has a WIP patch.
> 
> Hi Seinlin,
> 
> BTW, you may update the code in the same pull request if you just pushed to
> the same branch.
> You don't have to create another pull request. :)

Hi Rudy,

Thanks! I update change to the same pull request. Please have a look.
Comment on attachment 801986 [details]
Arabic keyboard defines a basicLayoutKey, but alternateLayout displays it as "ABC".

test
Attachment #801986 - Flags: feedback?(seinlin.maung)
feedback
Attachment #801986 - Flags: feedback?(seinlin.maung)
Comment on attachment 802817 [details]
bug-914459-pull-request-3.html

Hi Jan,

Not sure if you are interested in reviewing this patch first?
Attachment #802817 - Flags: review?(janjongboom)
Comment on attachment 802817 [details]
bug-914459-pull-request-3.html

r=me. The code works, but looks very strange if you see it in the codebase. Please add a comment that it's not possible to use |layout| variable here because it holds the alternative layout that doesn't have information about the alt key to be used; and that we need this info from the main layout object.

Please needinfo me if you added the comment and I'll land the patch :-) Thanks for your help!
Attachment #802817 - Flags: review?(janjongboom) → review+
Thanks, I added a comment and pull request is updated!
Flags: needinfo?(janjongboom)
Hi, thanks a lot for a working on this. The patch has been landed in https://github.com/mozilla-b2g/gaia/commit/042b3f3bf882f2b3cb91207d54927b2490d65b9e.
Flags: needinfo?(janjongboom)
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: