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

[Proposal] Use IntFlag types for handling Strokes + cleanup #1688

Open
leogott opened this issue Sep 5, 2024 · 3 comments
Open

[Proposal] Use IntFlag types for handling Strokes + cleanup #1688

leogott opened this issue Sep 5, 2024 · 3 comments

Comments

@leogott
Copy link

leogott commented Sep 5, 2024

Currently the Stenotype class converts binary to lists of strings, but not just that -- it also stores keymap
(which is affected by the active steno system and the user configuration).
Using the Keymap (which is in essence a mutable Mapping[list[str], list[str]] without saying so) it then converts those "machine strokes" to a different list of strings, using a function called keys_to_actions.
(It also takes care of filtering out empty strokes)

This gets then passed on to an engine method (using a slightly confusing but overall sound signal/slot construct).
This method, _on_stroked, immediately converts the list[str] to plover.steno.Stroke, the class/type of which is used as a Singleton of sorts, possibly for historical reasons.
Stroke is a confusing one. It is a subclass of int (good!), has like three methods of its own, but also a helper class, written in C, that it defers to for almost everything (30 methods or so).
(Idk why it doesn't inherit from it using multiple inheritance, but maybe there's a quirk with the mro that I'm not seeing)

This stroke class it what eventually arrives at the Translator class' translate_stroke method, just to be immediately packed into a more opaque Translation object, where it's usually in the presence of several friends as an outline.

Then there is plover.system, module acting as a Singleton storing the active steno system. Except -- there appears to be no reason for it to be one. It is barely ever accessed, and only has a method to replace all of its data.

I propose the following changes:

Instances of plover.machine.base.StenotypeBase (eg. plover.machine.geminipr.GeminiPr)
should emit Instances of IntFlag instead of list[str].

The StenotypeBase instance should not be keeping track of the keymap. Instead the Keymap should either be handled by the Engine Instance entirely, or handled in the Stroke.from_keys method and stored in a class attribute there.

The steno.Stroke class should subclass enum.IntFlag. Whenever Stroke.setup was called before, instead the reference to Stroke should be dropped, and a new Type created from Stroke (happens all tidy and behind the scenes with enum).

Accordingly, the Keymap should be a Mapping from a machine-specific IntFlag type to a system-specific IntFlag type.

plover.system.setup should define a method for loading a system from the registry into a dataclass, probably store a reference to it for reuse. Engine will store a reference to this class representing the active system.
plover.system should also define a method for getting the Stroke type specific to a system (maybe don't call it get_stroke). The stroke type could come with a reference to the system dataclass.
Or this method could belong to a system class.

Additionally to the Stroke class, there should be an Outline class, a thin wrapper around Tuple[Stroke] and this should be used whenever strokes are joined in a tuple now. (The Translation class should then store Stroke instances, mostly.)

Why?

Storing and operating on lists of strings, devoid of context, is inefficient and an inaccurate data model. It's also confusing from a new plugin developer's perspective.

Why not?

  • It might be more work than I do on my own right now. I started on it already so I'm hoping for your feedback.
  • Maintaining backwards compatibility might reduce the positive impact.
  • Apart from that, I'm asking you.
@greenwyrt
Copy link
Contributor

Your explanation of the process all the way to translator should be included as part of the dev docs.

@leogott
Copy link
Author

leogott commented Feb 3, 2025

Sorry for letting this sit forever. I hope to get back to this with a somewhat reduced scope (later this year).

While I stand by this in principle:

Instances of plover.machine.base.StenotypeBase (eg. plover.machine.geminipr.GeminiPr)
should emit Instances of IntFlag instead of list[str].

It would break all the backwards compatibility, and also I ran into an issue with my idea: the default python implementation of the int-flag type violates certain assumptions I had/have about enums. (Need to run some more general tests to be sure, and possibly work on enhancing python itself 👀 )

Knowing what I know now, the use of a helper function written in C (instead of an IntFlag) makes plenty sense.
But it would be "better" (here: more readable, maintainable and neat),

  • if plover stroke returned proper PyObjects instead of uints,
  • the implementation was moved into the plover repo, and
  • if it built with stub-files to be understood by the IDE.

That would allow for it to be instanced and called by the Engine directly, with no need for the thin-wrapper that is the current stroke.py

This is probably an achievable goal that could be achieved in a single PR.


Other stuff that is relevant:

Having the Keymap as an attribute of the plover machine (type or instance) also makes some sense to me now.
Think: "What labels/stickers would you put on the keys. Or switching Fn and Ctrl."

Following that, it would make sense to apply typing to both machines and dictionaries. Think: "What is the set of strokes that Machine+Keymap can emit; what is the set of strokes that can be interpreted by Dictionary".

While this doesn't really solve any problem — that can't be worked around easily — it would be pretty cool to have a list of dictionaries for several related systems (like plover-system and regenpfeiffer) and interpret a key as Z- for the regenpfeiffer dicts, and as S- for the plover-system dicts.

But there are solid workarounds for that, so it's probably not worth the hassle.

@greenwyrt
Copy link
Contributor

I'm not quite sure about the history and motivation but I think plover_stroke was split away from the main code as a plugin. Others like @TheaMorin probably know more about it than I do.

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

No branches or pull requests

2 participants