eonpatapon / gnome-shell-extension-caffeine

Disable screensaver and auto suspend
GNU General Public License v2.0
578 stars 107 forks source link

FYI: Are you sure to use "Gdk.SHIFT_MASK" in generalPage.js #279

Closed osamuaoki closed 1 year ago

osamuaoki commented 1 year ago

I have similar code and I found "Gdk.SHIFT_MASK" was undefined while debugging it under my environment of GNOME43.

In this code generalPage.js file uses this. I think desired constant is "Gdk.ModifierType.SHIFT_MASK", instead.

You can test situation for your build environment using my test code under test-keys/* of https://github.com/osamuaoki/inputmethod-shortcuts

(I don't know if this issue is version dependent. I see similar code as yours every where. E.g., https://github.com/jqno/gnome-happy-appy-hotkey/)

stuarthayhurst commented 1 year ago

Thanks for the tip :)

@pakaoraki I was looking through the code, is this much validation required for the bindings? If they've managed to press the keys it's probably valid, but I also haven't played around with that part of GNOME much to be sure.

    isValidBinding(mask, keycode, keyval) {
        return !(mask === 0 || mask === Gdk.SHIFT_MASK && keycode !== 0 &&
                 ((keyval >= Gdk.KEY_a && keyval <= Gdk.KEY_z) ||
                     (keyval >= Gdk.KEY_A && keyval <= Gdk.KEY_Z) ||
                     (keyval >= Gdk.KEY_0 && keyval <= Gdk.KEY_9) ||
                     (keyval >= Gdk.KEY_kana_fullstop && keyval <= Gdk.KEY_semivoicedsound) ||
                     (keyval >= Gdk.KEY_Arabic_comma && keyval <= Gdk.KEY_Arabic_sukun) ||
                     (keyval >= Gdk.KEY_Serbian_dje && keyval <= Gdk.KEY_Cyrillic_HARDSIGN) ||
                     (keyval >= Gdk.KEY_Greek_ALPHAaccent && keyval <= Gdk.KEY_Greek_omega) ||
                     (keyval >= Gdk.KEY_hebrew_doublelowline && keyval <= Gdk.KEY_hebrew_taf) ||
                     (keyval >= Gdk.KEY_Thai_kokai && keyval <= Gdk.KEY_Thai_lekkao) ||
                     (keyval >= Gdk.KEY_Hangul_Kiyeog && keyval <= Gdk.KEY_Hangul_J_YeorinHieuh) ||
                     (keyval === Gdk.KEY_space && mask === 0) || this.keyvalIsForbidden(keyval))
        );
    }
osamuaoki commented 1 year ago

YES. Users "xkb" setting may not be "us". These are safegurd for shortcut registration to prevent registering normal letter key "keyval". You don't want to make user register Shift-"A" etc.

The impact of the bug of using "Gdk.SHIFT_MASK" is minimal. Since it is undefined, this part of logic returns always "false". Registering "a" (without shift) as shortcut for caffeine is blocked but registering Shift-"A" is allowed. I think it is good idea to place safeguard.

If you read gnome-control-center code in C, they do this. I think everyone are fallowing. Since C doesn't care this is enum-type or not, code can skip type. But for gjs to pick up its value through GNOME introspection, we must specify enum name as "Gdk.ModifierType.SHIFT_MASK".

In the GTK4 gdk, these modifier constants are defined as enums in /usr/include/gtk-4.0/gdk/gdkenums.h

(FYI: Somehow, gjs API document https://gjs-docs.gnome.org/gdk40~4.0/ lacks these but GTK3 version gjs API document has https://gjs-docs.gnome.org/gdk30~3.0/gdk.modifiertype#default-modifier_mask .)

stuarthayhurst commented 1 year ago

Ah gotcha, I figured the shift mask was important but wasn't sure how it extended to other languages.

The impact of the bug of using "Gdk.SHIFT_MASK" is minimal. Since it is undefined, this part of logic returns always "false". Registering "a" (without shift) as shortcut for caffeine is blocked but registering Shift-"A" is allowed. I think it is good idea to place safeguard.

I'm not sure I follow? I did a quick test on my system, and raw values like "A" are blocked, and "SHIFT" + "A" is allowed. As that was what I expected to happen, I'm not sure I understand how the code is broken.

Either way, I can agree that the value is undefined, I'm just not sure how it impacts it, in order to test it :)

osamuaoki commented 1 year ago

Hi,

I'm not sure I follow? I did a quick test on my system, and raw values like "A" are blocked, and "SHIFT" + "A" is allowed. As that was what I expected to happen, I'm not sure I understand how the code is broken.

gnome-control-panel's keyboard shortcut dialog is reached from "Settings" -> "Keyboard" -> "Keyboard Shortcuts"

stuarthayhurst commented 1 year ago

Ohhh that makes more sense, I thought "SHIFT" + "A" was supposed to be enabled, but that does make sense.

pakaoraki commented 1 year ago

Thanks for the fix :)