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

[sil_euro_latin] Update to keyboard #3235

Merged
merged 3 commits into from
Dec 4, 2024

Conversation

LornaSIL
Copy link
Contributor

@LornaSIL LornaSIL commented Dec 3, 2024

  • Update language tags
  • Update documentation for punctuation

Finished fix for #3221
Fixes #2957

Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

Sorry to be a downer on this but I think we need some very careful analysis on nextlayer changes. I agree more nextlayer nuance would be good to have but currently this change is a step backwards (less nuanced!)

@@ -2377,6 +2377,7 @@
"id": "K_1",
"text": "1",
"sp": 0,
"nextlayer": "numeric",
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer that this be implemented in the .kmn rather than in the touch layout -- in the begin postkeystroke code path. The logic is there for digits already:

    c Okay, let's stay on the numeric layer if we are there already
    platform('touch') if(&newLayer = "") if(&layer = 'numeric') any(digit) > context

So we just need to create a new store digits-and-punct that includes the other outputs that should not switch layers, and use that there, I think.

Also, I don't think all the punctuation keys should be keeping us on the numeric layer, e.g. '"' should definitely go back to default. We need to consider each punctuation character carefully; the following are my starting thoughts:

0-9 -- stay numeric
. and , -- these should go back to the default layer unless preceded by a digit.
@ -- IMO go back to default layer, used most commonly probably in email addresses
# -- Debatable, the online elite use hashtags 😆
<, > -- Debatable again... can be used for maths but also many other things. perhaps if preceded by digits plus optional whitespace?
«, » -- these are quotation marks, so go back to default
+, -, *, /, ^, %, =, -- all of these have multiple uses (particularly - and /!). I don't think treating them as maths symbols only makes sense. If preceded by digits and optional whitespace?
( and friends -- these are much more commonly used in text than for numbers, I think. Should not stay in numeric layer
", !, `, ~, ¡, ?, ¿, ;, :, and & -- these are not numeric related. Go back to default layer.

So, in essence, I don't really agree with @ermshiperete's original suggestion, and I missed the issue when it was opened. We can add more nuance to the layer switch by sticking in the numeric layer when in the middle of a digit sequence and mathematical operators are used, but apart from that I think we need to drop back to default layer.

The rules would be something like (needs careful crafting):

    store(math-operator) ',.=+-*/' c ... and others
    store(spacing-math-operator) '=+-*/' c ... and others, but not ./,
    platform('touch') if(&newLayer = "") if(&layer = 'numeric') any(digit) any(math-operator) > context
    platform('touch') if(&newLayer = "") if(&layer = 'numeric') any(digit) ' ' any(spacing-math-operator) > context

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with fine-tuning the rules when we do a layer switch. However, I think : should stay in numeric layer if preceded by a digit since that's used for time.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I will pull my comment into #3013, and let's try and spec it in detail.

@@ -2767,11 +2800,13 @@
{
"id": "U_0027",
"text": "'",
"nextlayer": "numeric",
"hint": "`",
"sk": [
{
"text": "",
Copy link
Member

Choose a reason for hiding this comment

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

this seems wrong

@LornaSIL
Copy link
Contributor Author

LornaSIL commented Dec 4, 2024

I'm reverting the changes to the mobile layout so this PR will not address #2957 as I do not feel confident in how to do it. It only address langtags and documentation.

@LornaSIL
Copy link
Contributor Author

LornaSIL commented Dec 4, 2024

@mcdurdin I won't be making the changes to the numeric layer in this PR so if you can check it just for the tagging and also documentation of '' (I just updated the prefix image which only displayed a single quote) I'd appreciate it.

@mcdurdin
Copy link
Member

mcdurdin commented Dec 4, 2024

I removed fixes #2957 from the original comment, hope that is okay. The documentation update looks fine.

@DavidLRowe DavidLRowe merged commit 932acde into keymanapp:master Dec 4, 2024
2 checks passed
@LornaSIL LornaSIL deleted the sil_euro_latin branch December 4, 2024 23:05
@mcdurdin
Copy link
Member

mcdurdin commented Dec 4, 2024

@LornaSIL wrote:

I'm reverting the changes to the mobile layout so this PR will not address #2957 as I do not feel confident in how to do it.

Then I wrote:

I removed fixes #2957 from the original comment, hope that is okay. The documentation update looks fine.

Ugh. We both meant #3013. I'll fix this up. #2957 is fixed by this, #3013 isn't.

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.

[sil_euro_latin] Update help for unmodified apostrophe '
4 participants