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

Check for uppercase at each position when adding keyboard edits #38

Closed
wants to merge 1 commit into from

Conversation

tvquizphd
Copy link
Contributor

@tvquizphd tvquizphd commented Jan 3, 2021

This addresses a retext-spell issue I've discovered that is fundamentally an nspell issue.

I'm including a concrete example of the problem for reference.

Differences from main are bolded:

One Example I've tested

  • assume the input word is "Twinz"
  • assume character ais "t" at index 0
  1. We ignore that "T" is uppercase in "Twinz".
  2. The first group is "qwertzuop".
    1. We know the position of "t" in the group is 4.
    2. We split the word "Twinz" at position 4 into before="Twin" and after = ""
    3. We iterate thrice to group character b="e"
      • We select lowercase "e" for index 4 because "z" was lowercase at index 4

This is how we (confusingly) arrive at the correct suggestion of "Twine".

Ignoring a bigger problem

In main, the notion of "Keyboard groups" doesn't really make sense as it replaces at a position (of a in group) that is unrelated to the original position (of a in word). This solution ignores that "root problem."

For another solution, see my related PR.

Following the approach of main, this solution splits the word at the unrelated position of character a in the keyboard group. Working within that constraint, this solution uses the case of character that will be replaced to decide the case of the replacement character. See my review comments.

Copy link
Contributor Author

@tvquizphd tvquizphd left a comment

Choose a reason for hiding this comment

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

This change has the effect that only upper-case replacements are suggested to replace upper-case characters, and only lower-case replacements are suggested to replace lower-case characters. This solution keeps the current (confusing) behavior of main by making replacements at the in-group position instead of the in-word index. See my other PR for a different solution.

@@ -78,7 +79,6 @@ function suggest(value) {
while (++index < length) {
character = value.charAt(index)
insensitive = character.toLowerCase()
upper = insensitive !== character
Copy link
Contributor Author

@tvquizphd tvquizphd Jan 3, 2021

Choose a reason for hiding this comment

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

We don't need to know the case of character, as we will (confusingly) make our replacements at the in-group position instead of the in-word index. My other PR handles this differently.

@@ -91,6 +91,8 @@ function suggest(value) {
}

before = value.slice(0, position)
between = value[position] || value.slice(-1)
upper = between !== between.toLowerCase()
Copy link
Contributor Author

@tvquizphd tvquizphd Jan 3, 2021

Choose a reason for hiding this comment

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

Here, we observe the upper-case status of between, which is the character that will be replaced, or the last character in the word if position happens to occur after the end of the word. This would occur when replacing the "z" in "Twinz" for keyboard group "qwertzuop", at position = 5, which is beyond the last index in "Twinz".

The current approach of main would erroneously make these replacements as TwinzQ ... TwinzP. This solution would make the slightly more sensible replacements of Twinzq... Twinzp. See my other PR for a solution that would make the replacements of Twinq... Twinp.

@tvquizphd
Copy link
Contributor Author

This PR only fails #46, and it matches the expected value more closely for this test than the does the other PR.

@tvquizphd tvquizphd marked this pull request as draft January 3, 2021 17:21
@tvquizphd tvquizphd marked this pull request as ready for review January 6, 2021 06:17
@tvquizphd tvquizphd marked this pull request as draft January 6, 2021 06:21
@tvquizphd
Copy link
Contributor Author

I'm closing this PR in favor of PR #39, which now passes the tests just as well as this PR does.

@tvquizphd tvquizphd closed this Jan 9, 2021
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.

1 participant