@b4n commented on this pull request.
Looks mostly OK but for the comments. Good job, now it works as expected with
all layouts I could try :+1:
> + /*
+ fprintf(stderr, "(GdkEventKey*) {\n"
+ " state=%x (filtered=%x)\n"
+ " keyval=%x (%u)\n"
+ " length=%d\n"
+ " string=%s\n"
+ " hardware_keycode=%x (%u)\n"
+ " group=%u\n"
+ " is_modifier=%u\n"
+ "}\n",
+ ev->state, state,
+ ev->keyval, ev->keyval,
+ ev->length,
+ ev->string,
+ ev->hardware_keycode, ev->hardware_keycode,
+ ev->group,
+ ev->is_modifier
+ );
+ */
this should probably go away in production
> + " is_modifier=%u\n"
+ "}\n",
+ ev->state, state,
+ ev->keyval, ev->keyval,
+ ev->length,
+ ev->string,
+ ev->hardware_keycode, ev->hardware_keycode,
+ ev->group,
+ ev->is_modifier
+ );
+ */
+
+ /* control or control + shift pressed */
+ if(state == GDK_CONTROL_MASK || state == (GDK_CONTROL_MASK |
GDK_SHIFT_MASK))
+ {
+ CalculateNumKeys(display);
As said, I'd rather not re-compute this on so many keys. Not that's it's
overly expensive (although it performs 3 lookups for which I don't know the
complexity), but I feel like those events are gonna be pretty common and this
could be minimized. See
https://github.com/b4n/geany-plugins/commit/fa8acdbe536d0f063b2b12de77370fc9005f915d
for a suggested implementation.
> + /* could use hardware keycode instead of keyvals but if unable to get
> keyode then don't
+ * have logical default to fall back on
Actually we couldn't, at least not without manually handling the `group`. So
it's probably better to stick to keycodes, as at least for them we know what
we're looking for (<kbd>Shift</kbd> or no <kbd>Shift</kbd>).
> @@ -74,6 +74,7 @@ static gboolean bAlwaysSaveMarkers=FALSE; /* Always save
> markers, even if file h
/* internal variables */
static gint iShiftNumbers[]={41,33,34,163,36,37,94,38,42,40};
+static gint iNoShiftNumbers[]={49,50,51,52,53,54,55,56,57,58};
Hum, what are those? Shouldn't this be:
```suggestion
static gint iNoShiftNumbers[]={48,49,50,51,52,53,54,55,56,57};
```
This at least are the 0-9 from
[ASCII](https://fr.wikipedia.org/wiki/American_Standard_Code_for_Information_Interchange),
and what `xev` gives me as the keysyms for <kbd>0</kbd>-<kbd>9</kbd>.
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany-plugins/pull/1458#pullrequestreview-2972970694
You are receiving this because you are subscribed to this thread.
Message ID: <geany/geany-plugins/pull/1458/review/[email protected]>
