qmk / qmk_configurator

The QMK Configurator
http://config.qmk.fm
702 stars 350 forks source link

[Bug] "Menu" key should probably use KC_APP instead of KC_MENU (or, have both as options) #1143

Closed mgsloan closed 2 years ago

mgsloan commented 2 years ago

Describe the Bug

When I use the "Menu" button from the media keys section, it does not work as expected. Instead xev shows it is keycode 138 "SunProps" instead of keycode 135 "Menu". Manually modifing the json file to use KC_MENU fixes the issue.

From https://www.reddit.com/r/olkb/comments/5uzhiq/whats_the_difference_between_kc_app_and_kc_menu/

All the search results say it does unexpected stuff and that kc_app is the code for the menu key.

I am curious as well though because I haven't seen a good explanation of what KC_MENU should be for.

See also KC_MENU produces Sun Props key · Issue #306 · qmk/qmk_firmware

I propose that QMK configurator instead associate KC_APP with the media key called "Menu"

Additional Context?

I like to have a key on my keyboard to open right-click context menus. I discovered this was possible while reading the documentation for the text format used for configuring kinesis advantage 2 keyboard vanilla firmware. One of my kinesii controllers broke, so I replaced it with KinT + QMK.

In the Kinesis text configuration language the key I want is called [menu] (also yielded by KC_APP) and xev shows it to be

KeyPress event, serial 33, synthetic NO, window 0x3e00001,
    root 0x7a4, subw 0x0, time 39243766, (367,300), root:(1553,300),
    state 0x10, keycode 135 (keysym 0xff67, Menu), same_screen YES,

Whereas the menu media key in the configurator / KC_MENU in configurator json results in

KeyPress event, serial 33, synthetic NO, window 0x3e00001,
    root 0x7a4, subw 0x0, time 39863037, (367,200), root:(1553,600),
    state 0x10, keycode 138 (keysym 0x1005ff70, SunProps), same_screen YES,
mgsloan commented 2 years ago

Ahah! https://github.com/qmk/qmk_configurator/pull/770

KC_APP now uses a Hamburger Menu icon to differentiate it visually from KC_MENU. The latter is now marked as "(Legacy)" in its help text.

Uhhh that seems like a strange way to handle this problem. I would never guess that menu == hamburger. The hamburger icon also does not appear in the configurator.

I tried to fix this, but I'm not sure how to move forward nicely with this hamburger convention, so it seems there are decisions to be made. Personally I think the following is good:

From perusing the code I also now see that the proper "Menu" key is part of the ansi key selector layout, between right ctrl and right OS.

yanfali commented 2 years ago

As a work around try ANY(KC_APP)

mgsloan commented 2 years ago

It turns out it is possible in the configurator UI. For key selection there are two keys with the text "Menu", one on the ansi selector pane (which uses KC_APP, which I think is correct), but it renders differently when deployed - instead using the hamburger icon. On the media key selection pane there is also a key with the text "Menu", but it does not work as expected.

noroadsleft commented 2 years ago

@mgsloan How do you feel about the following change?

KC_APP (unchanged): image

KC_MENU (marked as (Legacy)): image

mgsloan commented 2 years ago

I think that would do the trick! Thanks James!!

For consistency it probably also makes sense to update their display to also look like this (when the key is used in the layout). IIRC currently the non legacy menu is displayed using a hamburger symbol

On Mon, Jun 27, 2022 at 11:15 AM James Young @.***> wrote:

@mgsloan https://github.com/mgsloan How do you feel about the following change?

KC_APP (unchanged): [image: image] https://user-images.githubusercontent.com/18669334/175991573-695122bc-c5cc-425f-bd25-20a6e24b1467.png

KC_MENU (marked as (Legacy)): [image: image] https://user-images.githubusercontent.com/18669334/175991731-0423e6c7-c3e5-4159-b55c-2ce5a1ee544c.png

— Reply to this email directly, view it on GitHub https://github.com/qmk/qmk_configurator/issues/1143#issuecomment-1167588836, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAFQKUWZCJGM7WR4KI3T2LVRHOTHANCNFSM5ZKGJ5DA . You are receiving this because you were mentioned.Message ID: @.***>

noroadsleft commented 2 years ago

I'd actually be more likely to revert the hamburger icon. I added that originally because users weren't able at the time to differentiate between the modern Menu key (KC_APP) and the legacy KC_MENU in the keymap display. I'm hesitant to use icons in the keycode select area because for the most part, the icons aren't standardized, and the ones that are are not widely used/known.

mgsloan commented 2 years ago

Yes, exactly what I was thinking too!

Thanks so much for addressing this, I hope it'll reduce confusion for others in the future (my keyboard lacked a functioning menu button for quite a while, which I'm accustomed to having on my unmodified Kinesis)

I highly recommend trying out including this key in layouts, btw, it is quite useful.

On Tue, Jun 28, 2022 at 1:50 AM James Young @.***> wrote:

Closed #1143 https://github.com/qmk/qmk_configurator/issues/1143 as completed via #1145 https://github.com/qmk/qmk_configurator/pull/1145.

— Reply to this email directly, view it on GitHub https://github.com/qmk/qmk_configurator/issues/1143#event-6891553133, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAFQKWAXWYZGILL6XUZV43VRKVF5ANCNFSM5ZKGJ5DA . You are receiving this because you were mentioned.Message ID: @.***>