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.

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

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

Conversation

octastylos-pseudodipteros
Copy link
Contributor

Summary of the Pull Request

Adds a new language for math and science, manually tested.

PR Checklist

  • Closes: Add new language for math and science to Quick Accent #26270
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • Tests: Added/updated and all pass
  • Localization: All end user facing strings can be localized
  • Dev docs: No need
  • New binaries: None
  • Documentation updated: No need

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

Comment on lines 209 to 230
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[] { "ζ", "ℤ" },
Copy link
Contributor

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?

Copy link
Contributor Author

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).

Copy link
Contributor

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 -

  1. 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.
  2. 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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@htcfreek thoughts?

Copy link
Collaborator

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.

Copy link
Contributor

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();
Copy link
Collaborator

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.

Copy link
Contributor Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants