[Core] Add getreuer's Autocorrect feature to core #15699

drashna · 2022-01-02T01:27:01Z

Description

As title.
It's a super cool feature, works well, and would be a great addition to QMK.

Types of Changes

Core
New feature
Enhancement/optimization
Documentation

Checklist

My code follows the code style of this project: C, Python
I have read the PR Checklist document and have made the appropriate changes.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

getreuer

Thanks, Drashna! This is awesome. Here are a few nits and suggestions.

quantum/process_keycode/process_autocorrect.c

lib/python/qmk/cli/generate/autocorrect_data.py

quantum/quantum_keycodes.h

quantum/process_keycode/process_autocorrect.c

lib/python/qmk/cli/generate/autocorrect_data.py

drashna · 2022-01-08T22:38:46Z

lib/python/qmk/cli/generate/autocorrect_data.py:55:1: C901 'parse_file' is too complex (20)

I'm not sure how to fix this.

Nor these:

lib/python/qmk/cli/generate/autocorrect_data.py:193:9: E731 do not assign a lambda expression, use a def
lib/python/qmk/cli/generate/autocorrect_data.py:222:5: E731 do not assign a lambda expression, use a def

Also, if "english_words" is added to requirements.txt, the unit tests fail (presumably because it's asking to install dependencies), and if it's not, cli ci fails

Some style changes to hopefully address lint errors that came up in qmk/qmk_firmware#15699. * Break up `parse_file()` into several functions. * Avoid using `lambda`. * Better error message if node link exceeds 64KB limit.

getreuer · 2022-01-09T04:25:16Z

Drashna, thank you for all your work on getting this together! Here are some suggestions that hopefully fix these lint errors. I also just made these style fixes in my repo in getreuer/qmk-keymap@c28e526 to show concretely what I suggest below.

lib/python/qmk/cli/generate/autocorrect_data.py:55:1: C901 'parse_file' is too complex (20)

I'm not sure how to fix this.

This is saying there are too many branches and loops in parse_file. We should pull some bits out as separate functions, which parse_file calls. For instance define

def check_typo_against_dictionary(typo: str) -> None:
  """Checks `typo` against English dictionary words."""

  if typo.startswith(':') and typo.endswith(':'):
    if typo[1:-1] in CORRECT_WORDS:
      print(f'Warning:{line_number}: Typo "{typo}" is a correctly spelled '
            'dictionary word.')
  elif typo.startswith(':') and not typo.endswith(':'):
    for word in CORRECT_WORDS:
      if word.startswith(typo[1:]):
        print(f'Warning:{line_number}: Typo "{typo}" would falsely trigger '
              f'on correctly spelled word "{word}".')
  elif not typo.startswith(':') and typo.endswith(':'):
    for word in CORRECT_WORDS:
      if word.endswith(typo[:-1]):
        print(f'Warning:{line_number}: Typo "{typo}" would falsely trigger '
              f'on correctly spelled word "{word}".')
  elif not typo.startswith(':') and not typo.endswith(':'):
    for word in CORRECT_WORDS:
      if typo in word:
        print(f'Warning:{line_number}: Typo "{typo}" would falsely trigger '
              f'on correctly spelled word "{word}".')

and then call this function in the loop in parse_file().

Nor these:

lib/python/qmk/cli/generate/autocorrect_data.py:193:9: E731 do not assign a lambda expression, use a def
lib/python/qmk/cli/generate/autocorrect_data.py:222:5: E731 do not assign a lambda expression, use a def

The lint message is (tersely) saying that we should rewrite an assigned lambda like

typo_len = lambda e: len(e[0])

as

def typo_len(e: Tuple[str, str]) -> int:
    return len(e[0])

Also, if "english_words" is added to requirements.txt, the unit tests fail (presumably because it's asking to install dependencies), and if it's not, cli ci fails

english_words is an optional dependency: if it is present, it is used to warn if the typos would create false triggers on correctly-spelled English words, but the program still produces the trie data either way. On the one hand, we could simply remove the english_words import if adding new dependencies is a big obstacle. On the other hand, it is useful to have this checking, so it would make sense if qmk could take on english_words as a dependency. Or, if there is an easier way to get an English dictionary, we could change the code to use that instead. I'm open to moving forward with whatever path you prefer.

Thanks again! =)

drashna · 2022-01-09T06:41:08Z

Thanks! I think i got most of it working. Though, the try part is causing the pytest stuff to error out, because it's being called in every python file, I think (due to how the cli stuff is configured).

I think it would work if the try import was moved into parse_file function instead. But I'm not exactly sure how to handle that, and properly set the correct_words stuff.

(i'm definitely not a python person)

drashna · 2022-01-09T07:55:44Z

got it mostly working, but still got pytest errors.

Mostly, just running into this:

lib/python/qmk/cli/generate/autocorrect_data.py:68:9: F841 local variable 'correct_words' is assigned to but never used

And I'm not sure how to correct it.

drashna · 2022-01-09T17:33:57Z

Well, got it passing all the tests, and it does appear to be working correctly. So looks like it's good to go.

getreuer · 2022-01-09T18:17:43Z

Awesome! Great work, Drashna! 🥳

drashna · 2022-01-10T01:22:09Z

Awesome! Great work, Drashna! 🥳

Thanks!

And documents added.

ZenghaoWang · 2022-01-13T02:19:11Z

Hi Drashna,

I wanted to try this out, but after locally checking out your PR and enabling the feature in my rules.mk, nothing is being autocorrected. I'm a bit lost on how to debug this, any ideas on where to start?

Apologies if this is the wrong place to ask.

drashna · 2022-01-13T02:42:25Z

keymap config is zero'ed by default, so that would cause this feature to be disabled by default. You'd need to hit the AUTOCRT/AUTOCORRECT_TOGGLE keycode once to enable it, initially.

ZenghaoWang · 2022-01-13T02:58:12Z

No change, even after I add AUTOCRT to my keymap and tap it

drashna · 2022-01-13T03:09:24Z

No change, even after I add AUTOCRT to my keymap and tap it

Odd. I can confirm that it does work, even from this branch directly.

quantum/process_keycode/process_autocorrect.c

drashna · 2022-04-18T16:11:52Z

and json support added.

zvecr

info.json having context of headers bleeds implementation details into something that should be fairly generic. Its also questionable if the feature should be enabled at the keyboard level, which makes the schema/mappings changes not required. My view would be that it should never be.

Providing a default header to fall back on, and then warning seems a tad strange. I'm not really sure what use case that is trying to cover (especially if you take the viewpoint above).

data/mappings/info_config.json

data/schemas/keyboard.jsonschema

data/mappings/info_rules.json

Fix compilation issues Clean up names Reformat python cleanup add support for swap hands Add support for -km and -kb Ugly hack to get working Make output pretty Add support for --output Fix lint issue hopefully address cli ci errors Fix double space additional tweaks Apply suggestions from code review Update lib/python/qmk/cli/generate/autocorrect_data.py Apply suggestions from code review Co-authored-by: Albert Y <[email protected]> adddep Apply cli ci suggestion' Attempt to fix linter remove english_words dep Brute force hack to pass CI Fix issue with autocorrect leave english words enabled Fix pytest issues? Get cli python code working pass on line_number get tests passed Add documentation minor tweaks Fixes based on feedback Add fixes based on feedback Fix rebase conflicts add one more backspace if it's needed Add fixes move pressed processing improvements based on feedback "Fix" formatting fix error output fix linting fix linting, more brute force python formatting fix test this fix compilation issues with mods Add user callback for which keycodes to handle Allow configurable data file fix pr linting better handle keycodes fix some compiler issues Add doxygen comment Fix some edge cases Remove special case - not sure why I added it Make buffer check a switch Add comments Additional improvements (mods+) Update quantum/process_keycode/process_autocorrect.c Co-authored-by: Albert Y <[email protected]> fix oneshot check Co-authored-by: Albert Y <[email protected]> Add fixes and commenting from filterpaper Co-authored-by: Albert Y <[email protected]> Fix up switch case Ignore "dead" keys when features are disabled fix check Co-authored-by: Albert Y <[email protected]> Apply suggestions from code review Co-authored-by: Albert Y <[email protected]> apply documentation suggetion Co-authored-by: Albert Y <[email protected]> Expand handling for autocorrect triggering Fix functions and add more docs Apply suggestions from code review Co-authored-by: Albert Y <[email protected]> Rename apply function clarify pointier parameter Apply suggestions from code review Co-authored-by: Albert Y <[email protected]> fix docs Co-authored-by: Albert Y <[email protected]> Enable autocorrect by default Add on/off keycodes for autocorrect Move autocorrect to be after existing keycodes Update generate-autocorrect-data file Remove empty line Move svg to be local It\'s not imgur compatible so host it locally Switch to imgur hosted png Fix overflow issue on AVR Co-authored-by: Albert Y <[email protected]> Attempt to add tests? Add config.h for test fix formatting Prevent autocorrect test warning remove tests Add tests for autocorrect (wooooo!!!) Thanks karlk90!! Re-add autocorrect define to hide no default warning Add functions for state behavior Add enable/disable checks Fix lint issues Add additional test cases Clarify names Revert changes to drashna userspace for a cleaner commit history Add info.json support Add state check Don't ignore autocorrect h file Update docs Changes based on feedback Remove changes to json stuff

tzarc · 2022-09-17T05:57:43Z

tests/autocorrect/config.h

+
+#define AUTOCORRECT_DATA_H "quantum/process_keycode/autocorrect_data_default.h"


Suggested change

#define AUTOCORRECT_DATA_H "quantum/process_keycode/autocorrect_data_default.h"

tzarc · 2022-09-17T05:58:03Z

tests/autocorrect/config.h

@@ -0,0 +1,21 @@
+/* Copyright 2021 Stefan Kerkmann


Should probably be yourself.

tzarc · 2022-09-17T05:58:11Z

tests/autocorrect/test.mk

@@ -0,0 +1,20 @@
+# Copyright 2021 Stefan Kerkmann


Should probably be yourself.

tzarc · 2022-09-17T05:58:17Z

tests/autocorrect/test_autocorrect.cpp

@@ -0,0 +1,217 @@
+/* Copyright 2017 Fred Sundvik


Should probably be yourself.

tzarc · 2022-09-17T05:58:51Z

quantum/process_keycode/process_autocorrect.h

@@ -0,0 +1,17 @@
+// Copyright 2021 Google LLC


Is the Google thing because it matches their OSS policy?

It's because that's what the original code is.
https://github.com/getreuer/qmk-keymap/blob/a1f62991a99b205b7d493dcc1fdea3946cbb575c/features/autocorrection.c#L1-L13

And yeah, because of their OSS policies, IIRC>

tzarc · 2022-09-17T05:59:00Z

quantum/process_keycode/process_autocorrect.c

@@ -0,0 +1,287 @@
+// Copyright 2021 Google LLC


Is the Google thing because it matches their OSS policy?

tzarc · 2022-09-17T05:59:09Z

lib/python/qmk/cli/generate/autocorrect_data.py

@@ -0,0 +1,289 @@
+# Copyright 2021 Google LLC


Is the Google thing because it matches their OSS policy?

Co-authored-by: Albert Y <[email protected]>

github-actions bot added cli qmk cli command core keymap python labels Jan 2, 2022

drashna requested a review from a team January 2, 2022 01:28

drashna mentioned this pull request Jan 2, 2022

Migrate autocorrect to QMK Core? getreuer/qmk-keymap#4

Closed

getreuer reviewed Jan 2, 2022

View reviewed changes

filterpaper suggested changes Jan 2, 2022

View reviewed changes

lib/python/qmk/cli/generate/autocorrect_data.py Outdated Show resolved Hide resolved

lib/python/qmk/cli/generate/autocorrect_data.py Outdated Show resolved Hide resolved

lib/python/qmk/cli/generate/autocorrect_data.py Outdated Show resolved Hide resolved

drashna force-pushed the core/autocorrect branch from 74c0033 to bbddc69 Compare January 2, 2022 17:28

github-actions bot added the documentation label Jan 9, 2022

drashna force-pushed the core/autocorrect branch from 6a51e1e to cbff3bf Compare January 13, 2022 03:05

getreuer suggested changes Jan 13, 2022

View reviewed changes

drashna force-pushed the core/autocorrect branch from d805d20 to e59d902 Compare January 13, 2022 21:17

getreuer suggested changes Jan 16, 2022

View reviewed changes

quantum/process_keycode/process_autocorrect.c Show resolved Hide resolved

drashna force-pushed the core/autocorrect branch 2 times, most recently from 3de96bb to 8a572f5 Compare January 29, 2022 02:22

drashna force-pushed the core/autocorrect branch from 8a572f5 to c6d1488 Compare February 4, 2022 06:44

drashna force-pushed the core/autocorrect branch from 13f7756 to 695779a Compare April 18, 2022 16:03

zvecr reviewed May 20, 2022

View reviewed changes

data/mappings/info_config.json Outdated Show resolved Hide resolved

data/schemas/keyboard.jsonschema Outdated Show resolved Hide resolved

drashna force-pushed the core/autocorrect branch 2 times, most recently from fca16b1 to c65ef0a Compare May 20, 2022 16:53

drashna added breaking_change_2022q3 and removed breaking_change_2022q2 labels May 20, 2022

drashna force-pushed the core/autocorrect branch 2 times, most recently from e717f19 to e22731e Compare June 18, 2022 03:42

drashna force-pushed the core/autocorrect branch from e22731e to dca76c8 Compare July 3, 2022 17:05

drashna force-pushed the core/autocorrect branch from dca76c8 to 9c746c7 Compare July 19, 2022 16:18

drashna force-pushed the core/autocorrect branch from 9c746c7 to 849ea4a Compare August 7, 2022 21:16

drashna requested a review from a team August 7, 2022 21:20

tzarc requested changes Aug 14, 2022

View reviewed changes

data/schemas/keyboard.jsonschema Outdated Show resolved Hide resolved

data/mappings/info_rules.json Outdated Show resolved Hide resolved

drashna requested a review from a team August 14, 2022 20:24

drashna force-pushed the core/autocorrect branch from 63f1e1a to 19b9401 Compare August 15, 2022 18:14

drashna added the breaking_change_2022q4 label Aug 21, 2022

tzarc removed the breaking_change_2022q3 label Aug 25, 2022

drashna force-pushed the core/autocorrect branch from 19b9401 to 5dfc383 Compare August 26, 2022 19:20

drashna force-pushed the core/autocorrect branch from 5dfc383 to eac3795 Compare August 29, 2022 05:14

tzarc requested changes Sep 17, 2022

View reviewed changes

Changes from review

140315d

tzarc approved these changes Sep 17, 2022

View reviewed changes

tzarc merged commit fb29c0a into qmk:develop Sep 17, 2022

drashna deleted the core/autocorrect branch September 17, 2022 15:10

ramonimbao pushed a commit to ramonimbao/qmk_firmware that referenced this pull request Nov 28, 2022

[Core] Add getreuer's Autocorrect feature to core (qmk#15699)

9bcae00

Co-authored-by: Albert Y <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] Add getreuer's Autocorrect feature to core #15699

[Core] Add getreuer's Autocorrect feature to core #15699

drashna commented Jan 2, 2022 •

edited

Loading

getreuer left a comment

drashna commented Jan 8, 2022

getreuer commented Jan 9, 2022

drashna commented Jan 9, 2022 •

edited

Loading

drashna commented Jan 9, 2022 •

edited

Loading

drashna commented Jan 9, 2022

getreuer commented Jan 9, 2022

drashna commented Jan 10, 2022

ZenghaoWang commented Jan 13, 2022

drashna commented Jan 13, 2022

ZenghaoWang commented Jan 13, 2022

drashna commented Jan 13, 2022

drashna commented Apr 18, 2022

zvecr left a comment •

edited

Loading

tzarc Sep 17, 2022

tzarc Sep 17, 2022

tzarc Sep 17, 2022

tzarc Sep 17, 2022

tzarc Sep 17, 2022

drashna Sep 17, 2022

tzarc Sep 17, 2022

tzarc Sep 17, 2022


		#define AUTOCORRECT_DATA_H "quantum/process_keycode/autocorrect_data_default.h"

[Core] Add getreuer's Autocorrect feature to core #15699

[Core] Add getreuer's Autocorrect feature to core #15699

Conversation

drashna commented Jan 2, 2022 • edited Loading

Description

Types of Changes

Checklist

getreuer left a comment

Choose a reason for hiding this comment

drashna commented Jan 8, 2022

getreuer commented Jan 9, 2022

drashna commented Jan 9, 2022 • edited Loading

drashna commented Jan 9, 2022 • edited Loading

drashna commented Jan 9, 2022

getreuer commented Jan 9, 2022

drashna commented Jan 10, 2022

ZenghaoWang commented Jan 13, 2022

drashna commented Jan 13, 2022

ZenghaoWang commented Jan 13, 2022

drashna commented Jan 13, 2022

drashna commented Apr 18, 2022

zvecr left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drashna commented Jan 2, 2022 •

edited

Loading

drashna commented Jan 9, 2022 •

edited

Loading

drashna commented Jan 9, 2022 •

edited

Loading

zvecr left a comment •

edited

Loading