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

KC_TRANS #55

Open
hasumikin opened this issue Jan 30, 2022 · 4 comments
Open

KC_TRANS #55

hasumikin opened this issue Jan 30, 2022 · 4 comments
Labels
good first issue Good for newcomers

Comments

@hasumikin
Copy link
Member

hasumikin commented Jan 30, 2022

#51

Aliases: KC_TRNS and _______ (seven "_"s)

@hasumikin hasumikin added the good first issue Good for newcomers label Jan 30, 2022
@hasumikin hasumikin changed the title KC_TRANS and _______ KC_TRANS Jan 30, 2022
@rschenk
Copy link

rschenk commented Jul 1, 2022

I looked into this issue, but I think implementing transparent keys will require refactoring the way layers are implemented to introduce the concept of whether a layer is active or not.

Imagine the following hypothetical keyboard:

#                          key1     key2     key3 
kbd.add_layer :default, %i(KC_A     KC_B     KC_C     SPC_NUM ENT_SYM ESC_NAV)
kbd.add_layer :numbers, %i(_______  KC_2     KC_3     SPC_NUM ENT_SYM ESC_NAV)
kbd.add_layer :symbols, %i(KC_DOT   _______  KC_COMMA SPC_NUM ENT_SYM ESC_NAV)
kbd.add_layer :nav,     %i(KC_UP    KC_DOWN  _______  SPC_NUM ENT_SYM ESC_NAV)

kbd.define_mode_key :SPC_NUM, [ :KC_SPACE, :numbers, 200, 200 ]
kbd.define_mode_key :ENT_SYM, [ :KC_ENTER, :symbols, 200, 200 ]
kbd.define_mode_key :ESC_NAV, [ :KC_ESC,   :nav,     200, 200 ]

Case 1 Hold down SPC_NUM and tap key1
Expected behavior: send "A" to host
Actual behavior if _______ was implemented with current layer model: ✅ "A" sent to host

Case 2 Hold down ENT_SYM and tap key2
Expected behavior: send "B" to host
Actual behavior if _______ was implemented with current layer model:

  • if we defer to the next layer in the stack, then ❌ "2" sent to host
  • if we defer always to the default layer, then ✅ "B" sent to host

Case 3 Hold down ESC_NAV and hold down SPC_NUM and tap key3
Expected behavior: send "3" to host
Actual behavior if _______ was implemented with current layer model:

  • if we defer to the next layer in the stack, then ❌ "," sent to host
  • if we defer always to the default layer, then ❌ "C" sent to host

The problem is that a transparent keycode needs to consider the layers that are currently active underneath it, not just whatever layer is underneath it, and there is no concept in PRK at the moment that multiple layers could be active.

Not sure if refactoring the layer model is something that should be undertaken right now, or if it should wait?

Edit: having thought about this some more, perhaps having _______ always defer to the default layer is good enough? At least for now?

@hasumikin
Copy link
Member Author

hasumikin commented Jul 3, 2022

@rschenk Good point, thanks!
We need to investigate the spec of QMK Firmware as it's the defacto standard, I guess

@rschenk
Copy link

rschenk commented Jul 4, 2022

It does look like QMK has the notion of multiple active layers:

you can change layer_state to overlay the base layer with other layers

-- Multiple active layers.

The firmware works its way down from the highest active layers to look up keycodes. Once the firmware locates a keycode other than KC_TRNS (transparent) on an active layer, it stops searching, and lower layers aren't referenced.

-- Layer precedence

So it seems like PRK needs to get the concept of active layers.

I guess the question then becomes, how "Ruby-like" should/can this be implemented? Right now the layers are modeled as a hash of arrays. Now it seems that we need to also store a active? boolean for each layer. The C-like procedural way would be to make another array of booleans to store the active states for each layer. I feel like the Ruby way would be to make a Layer class that would store both the keymap and the active state for each layer. Likewise it might also make sense to have a LayerStack class that would keep track of all the layers, the default layer, and would be responsible for handling KC_TRANS. That would be quite a refactoring.

@hasumikin
Copy link
Member Author

@rschenk Thanks again.

Now that I want the PRK's layer system to become similar to the QMK's one.
It means we have to introduce a thing like layer_state.

But I, for now, don't know whether the state should be an independent variable or a member of the existing keymap object.

At the same time, I'm reluctant to introduce a specified class such as LayerStack because I am a believer in PORO, Plain Old Ruby Objects, like Array and Hash.
PORO would have enough functionality to handle this issue, I guess.

So, two major improvements we will have:

  1. Fixing the behavior of changing the active layer in this somehow inelegant block:
    https://github.com/picoruby/prk_firmware/blob/master/src/ruby/app/models/keyboard.rb#L1040-L1097
    In order to do this, I'm OK that a new method is produced in the Keyboard class.
    We will also add the layer_state concept in some way.

  2. Adding a new logic to search the desired keycode by taking advantage of the layer_state and going back to retroactive layers in this somewhat better block:
    https://github.com/picoruby/prk_firmware/blob/master/src/ruby/app/models/keyboard.rb#L1106-L1120
    As in, a new method can be added to our big Keyboard class.

Are we now seeing a common perspective?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants