Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[hoisan] New submission #3389

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[hoisan] New submission #3389

wants to merge 3 commits into from
+12,059 −0

Conversation

Cathaylaboratory
Copy link
Contributor

ready for reviewing

@Cathaylaboratory
hoisan submission
b3ac610 
ready for reviewing
@keyman-server
Copy link
Collaborator

Thank you for your pull request. You'll see a "build failed" message until the Keyman team has reviewed the pull request and manually initiated the build process.

Every change committed to this branch will become part of this pull request. When you have finished submitting files and are ready for the Keyman team to review this pull request, please post a "Ready for review" comment.

@keyman-server
Copy link
Collaborator

Thank you for your pull request. You'll see a "build failed" message until the Keyman team has reviewed the pull request and manually initiated the build process.

Every change committed to this branch will become part of this pull request. When you have finished submitting files and are ready for the Keyman team to review this pull request, please post a "Ready for review" comment.

@LornaSIL LornaSIL changed the title hoisan submission [hoisan] New submission Mar 21, 2025
@LornaSIL
Copy link
Contributor

You are getting a lot of warnings that you should address. When you compile your keyboard, you will see the warning in the Messages folder. I recommend always trying to deal with them. The rules are:

'qú' + any(vowels) >'qu'index(vowels_sac, 3)
'qù' + any(vowels) >'qu'index(vowels_huyen, 3)
'qủ' + any(vowels) >'qu'index(vowels_hoi, 3)
'qũ' + any(vowels) >'qu'index(vowels_nga, 3)
'qụ' + any(vowels) >'qu'index(vowels_nang, 3)
'Qú' + any(vowels) >'Qu'index(vowels_sac, 3)
'Qù' + any(vowels) >'Qu'index(vowels_huyen, 3)
'Qủ' + any(vowels) >'Qu'index(vowels_hoi, 3)
'Qũ' + any(vowels) >'Qu'index(vowels_nga, 3)
'Qụ' + any(vowels) >'Qu'index(vowels_nang, 3)
'QÚ' + any(vowels) >'QU'index(vowels_sac, 3)
'QÙ' + any(vowels) >'QU'index(vowels_huyen, 3)
'QỦ' + any(vowels) >'QU'index(vowels_hoi, 3)
'QŨ' + any(vowels) >'QU'index(vowels_nga, 3)
'QỤ' + any(vowels) >'QU'index(vowels_nang, 3)

The warnings are like this one: The rule will never be matched for key 'Ă' because its key code is never fired.

The reason it's a problem is that the any(vowels) is in the keystroke part of the rule (on the right of >), but the store has these characters aeiouăâêôơưyAEIOUĂÂÊÔƠƯY and none of the ones with accents should ever be part of the keystroke part of the rule since the keyboard doesn't produce those except through rules you create.

In addition, it would be best to make sure there is a SPACE before index(. I think in the future that could cause problems.

It says there are more than 100 warnings, so I don't think the rules I've mentioned are the only ones in question.

After you fix those issues I think it will be okay.

@Cathaylaboratory
Copy link
Contributor Author

Cathaylaboratory commented Mar 21, 2025 via email

@LornaSIL
Copy link
Contributor

The warning are there to make the keyboards function better.

@Cathaylaboratory
Copy link
Contributor Author

Cathaylaboratory commented Mar 21, 2025 via email

@Cathaylaboratory
Copy link
Contributor Author

Cathaylaboratory commented Mar 21, 2025 via email

@mcdurdin
Copy link
Member

I think that a keyboard with this many warnings should probably be in experimental/, not release/.

Generally we don't like to accept keyboards with warnings for a couple of reasons:

  1. They may indicate design problems with the keyboard or usability issues.
  2. Ignored warnings fill up the compile log and make it hard to spot problems in other places. Even in this keyboard, there may be more serious warnings that are hidden because only the first 100 warnings are displayed.

If warnings can be fixed, they should be fixed. This is all part of preparing a keyboard to be accepted into the keyboard repository. Now, if we missed this with the vietnam keyboard, that is not a precedent: the warnings should be fixed there also.

Note that some warnings may be upgraded to errors by future versions of Keyman Developer.

Related: keymanapp/keyman#10397

In addition, it would be best to make sure there is a SPACE before index(. I think in the future that could cause problems.

It is good formatting practice to include a space (@markcsinclair FYI, are we testing this currently?).

@keyman-server
Copy link
Collaborator

Thank you for your pull request. You'll see a "build failed" message until the Keyman team has reviewed the pull request and manually initiated the build process.

Every change committed to this branch will become part of this pull request. When you have finished submitting files and are ready for the Keyman team to review this pull request, please post a "Ready for review" comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Cathaylaboratory @keyman-server @LornaSIL @mcdurdin