-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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.
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
[Quick Accent] Add new language - Math #34428
base: main
Are you sure you want to change the base?
[Quick Accent] Add new language - Math #34428
Conversation
LetterKey.VK_A => new[] { "α", "∀" }, | ||
LetterKey.VK_B => new[] { "β" }, | ||
LetterKey.VK_C => new[] { "χ", "ℂ", "∁" }, | ||
LetterKey.VK_D => new[] { "δ", "∂", "◇" }, | ||
LetterKey.VK_E => new[] { "ε", "η", "∃", "∄", "∈", "∉" }, | ||
LetterKey.VK_F => new[] { "φ", "ƒ" }, | ||
LetterKey.VK_G => new[] { "γ" }, | ||
LetterKey.VK_I => new[] { "ι" }, | ||
LetterKey.VK_K => new[] { "κ" }, | ||
LetterKey.VK_L => new[] { "λ" }, | ||
LetterKey.VK_M => new[] { "μ" }, | ||
LetterKey.VK_N => new[] { "ν", "ⁿ", "ℕ" }, | ||
LetterKey.VK_O => new[] { "ο", "ω" }, | ||
LetterKey.VK_P => new[] { "π", "ψ", "φ", "∏", "⊥", "⊢", "⊬" }, | ||
LetterKey.VK_Q => new[] { "ℚ" }, | ||
LetterKey.VK_R => new[] { "ρ", "ℝ" }, | ||
LetterKey.VK_S => new[] { "σ", "∑", "∫", "∮" }, | ||
LetterKey.VK_T => new[] { "θ", "τ", "⊨", "⊭" }, | ||
LetterKey.VK_U => new[] { "υ" }, | ||
LetterKey.VK_X => new[] { "ξ", "×", "⋅" }, | ||
LetterKey.VK_Y => new[] { "υ" }, | ||
LetterKey.VK_Z => new[] { "ζ", "ℤ" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @octastylos-pseudodipteros, in the issue's comments, there were suggestions of not including the Greek alphabets as they are already included. Any reason for specifically including them here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greek letters are often used together with math symbols and even though there is a separate language for them, it is not possible to use both math and Greek letters language at the same time (if the user doesn't want to have symbols from all languages).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of issues with this understanding -
- The issue you're stating is open [Quick Accent] Select multiple languages #21261. It is only a matter of time before it gets implemented and this issue is resolved.
- If in the future, we have to change some symbols / add some (symbols/accents) in the Greek language, we would have to deal with it in two places (redundancy).
My suggestion would be to remove the Greek references from your function and pass the letter to the GetDefaultLetterKeyEL
function to get the Greek references for that particular letter. You could then concatenate these in your string array and return the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a good idea, I will do it. Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @GhostVaibhav, in the new commit Math language uses Greek letters from the Greek letter alphabet, but because of this there are also letters with tonos (eg. ά, έ...), which aren't commonly used in math.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @octastylos-pseudodipteros, that should not be a major blocking issue since all the other characters would be legally used. I have other concerns with removing "π" from Portuguese.
@htcfreek the "π" symbol doesn't have any special meaning in Portuguese and this PR removes it from there. Is it allowed to change the base definitions of already defined languages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@htcfreek thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sory for the late reply. I missed your question.
I think removing is okay. If the character doesn't belongs to a language it makes no sense to have it in the set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @htcfreek, the PR looks good to me. Can you also review it (just in case)?
_ => Array.Empty<string>(), | ||
}; | ||
|
||
return characters.Union(GetDefaultLetterKeyEL(letter)).ToArray(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the EL list contains only math related characters? If not we should split the EL list into EL math and EL text or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @htcfreek, GetDefaultLetterKeyEL
contains 7 characters with tonos (ά έ ή ί ό ύ ώ), which are not commonly used in math. In the original commit, this function was not used and these characters were excluded from the math language. Should I revert to the original commit?
Summary of the Pull Request
Adds a new language for math and science, manually tested.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Adds a new language for math and science with basic symbols, some of them were taken from All Languages
Validation Steps Performed
Manually tested