KMKfw / kmk_firmware

Clackety Keyboards Powered by Python
https://kmkfw.zulipchat.com
Other
1.38k stars 474 forks source link

Add Alt GUI Swap module #768

Closed dunk2k closed 1 year ago

dunk2k commented 1 year ago

Essentially cg_swap code repurposed, i.e. nothing clever.

Result: another piece of functionality that increases parity, of KMK, with QMK.

xs5871 commented 1 year ago

This doesn't add any functionality. The _cg_mapping of CgSwap is user-modifiable to allow for alt-gui swaps. That use case should be mentioned in the docs at least; _cg_mapping shouldn't have a leading underscore and the module (and internal variables) should be renamed to something akin ModRemap to reflect its general functionality.

dunk2k commented 1 year ago

the _cg_mapping of CgSwap is user-modifiable to allow for alt-gui swaps. That use case should be mentioned in the docs at least

It's not chief, hence me whipping this up.
More than happy to cancel this PR in favour of documentation updated to reflect the above.

xs5871 commented 1 year ago
cgswap = CgSwap()
cgswap._cg_mapping = {
    KC.LALT: KC.LGUI,
    KC.RALT: KC.RGUI,
    KC.LGUI: KC.LALT,
    KC.RGUI: KC.RALT,
}

Please confirm if this is working for you.

dunk2k commented 1 year ago
cgswap = CgSwap()
cgswap._cg_mapping = {
    KC.LALT: KC.LGUI,
    KC.RALT: KC.RGUI,
    KC.LGUI: KC.LALT,
    KC.RGUI: KC.RALT,
}

Please confirm if this is working for you.

Hi @xs5871 ,

Yes this works for me. The boards/hillside PR (#769) has both Ctrl GUI swap and Alt GUI swap keys in keyboard.keymap element, and the above method isn't sufficient, without customising code, for this use case.

Would be happy to rework the CG swap module to a Mod Swap module after this PR is merged, as what you've mentioned above holds merit/principle.

xs5871 commented 1 year ago

agswap = CgSwap()
agswap._cg_mapping = {
    KC.LALT: KC.LGUI,
    KC.RALT: KC.RGUI,
    KC.LGUI: KC.LALT,
    KC.RGUI: KC.RALT,
}
AG_SWAP = KC.CG_SWAP
AG_NORM = KC.CG_NORM
AG_TOGG = KC.CG_TOGG

keyboard.modules.append(ag_swap)

cgswap = CgSwap()
keyboard.modules.append(cg_swap)

keyboard.keymap = [[
..., KC.CG_SWAP, AG_SWAP,...
]]
> ```
That is also possible without code modifications.
I'm not against refactoring the `CgSwap` module, but I don't think we need to merge the `AgSwap` first.
dunk2k commented 1 year ago

I'm not against refactoring the CgSwap module, but I don't think we need to merge the AgSwap first.

On that note chief, would this PR be better utilised to append cg_swap with the code you've above?

dunk2k commented 1 year ago

as of commit f315b58 I've added ag_swap.py code into cg_swap.py, renamed cg_swap.py to mod_swap.py

Not elegant but does allow users to import CgSwap and/or AgSwap from one module.
@xs5871 If you give this the go ahead, I'll amend docs/en/cg_swap.md to match.

xs5871 commented 1 year ago

To be honest, I don't want to merge code that needs refactoring or, much worse, documentation changes, from the get-go. The CgSwap module wouldn't pass code QA today and duplicating that code makes it worse.

I completely agree that the funcionatility you're proposing is a worthwile addition, the implementation however has to be an improvement, not two times the same legacy baggage.

dunk2k commented 1 year ago

I completely agree that the functionality you're proposing is a worthwhile addition, the implementation however has to be an improvement, not two times the same legacy baggage.

There really isn't a good/strong counter argument to that statement. On that note I'll cancel this PR and amend hillside PR accordingly.