Skip to content

Indirection on new_pressed_key - #389

Merged
rgoulter merged 6 commits into
masterfrom
refactor-npk-usage
Sep 2, 2025
Merged

Indirection on new_pressed_key#389
rgoulter merged 6 commits into
masterfrom
refactor-npk-usage

Conversation

@rgoulter

@rgoulter rgoulter commented Sep 2, 2025

Copy link
Copy Markdown
Owner

Towards #381, one blocker is that various key::Key implementations themselves invoke key::Key::new_pressed_key.

This PR is to rearrange the types so that these invocations instead return some kind of value.

@rgoulter

rgoulter commented Sep 2, 2025

Copy link
Copy Markdown
Owner Author

Hmm.

Previously, when the implementation returns KeyState directly, it's able to return NoOpKeyState.

It's not obvious to me whether it's better to have a NoOpKeyState, or to have a NewPressedKey::NoOp variant. But, seems untidy to have both.

@rgoulter
rgoulter force-pushed the refactor-npk-usage branch 3 times, most recently from 017ba6e to c304e51 Compare September 2, 2025 13:37
@rgoulter

rgoulter commented Sep 2, 2025

Copy link
Copy Markdown
Owner Author

Good news is, the test suite is quite comprehensive if it's catching issues.

@rgoulter

rgoulter commented Sep 2, 2025

Copy link
Copy Markdown
Owner Author

Okay, tests pass again.

But:

  1. the code (especially relating to match pkr { ... } in Keymap process_input & handle_event is largely (entirely?) duplicated. Likely will be cleaner if this common code is in a common method. (Recall, process_input deals with input events, handle_event deals with input+key events).
  2. KeyPath add_path_item ... it's too complex to keep the same name for different action. -- Should rename it in separate PR to "prepend_path_item", and in this PR to "append_path_item".
  3. Similarly, can't the NPK loops be factored?

@rgoulter

rgoulter commented Sep 2, 2025

Copy link
Copy Markdown
Owner Author

Similarly, for the same reason we want factor out new_pressed_key invocations, it'd be a good idea to factor out use of NoOpKeyState.

@rgoulter

rgoulter commented Sep 2, 2025

Copy link
Copy Markdown
Owner Author

Regarding common code: it's true that lots of the code is common, especially in how the pending key state is handled in process_input and handle_event.. but, since both these methods mutably borrow self (&mut self), & since the code which processes the events with the pending state calls other methods on self which also mutably borrow self, the common code can't really be expressed as a method (since a method borrows at least &self).

There might be some way, but I can't think of how to do it nicely.

@rgoulter
rgoulter marked this pull request as ready for review September 2, 2025 15:11
@rgoulter
rgoulter merged commit 10c370c into master Sep 2, 2025
6 checks passed
@rgoulter
rgoulter deleted the refactor-npk-usage branch September 2, 2025 15:13
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