[Bf-blender-cvs] [ea79dab0621] master: Docs: add notes about wmWindow.eventstate & modifier key checks

[Campbell Barton]

Commit: ea79dab0621a0ef992d6289a95e72329279f6383
Author: Campbell Barton
Date:   Wed Sep 21 16:41:49 2022 +1000
Branches: master
https://developer.blender.org/rBea79dab0621a0ef992d6289a95e72329279f6383

Docs: add notes about wmWindow.eventstate & modifier key checks

There were undocumented limitations in the current modifier handling
that came to my attention while investigating related issues.

===================================================================

M	source/blender/makesdna/DNA_windowmanager_types.h
M	source/blender/windowmanager/intern/wm_event_system.cc

===================================================================

diff --git a/source/blender/makesdna/DNA_windowmanager_types.h b/source/blender/makesdna/DNA_windowmanager_types.h
index 1c71129a3c7..d57cca0b055 100644
--- a/source/blender/makesdna/DNA_windowmanager_types.h
+++ b/source/blender/makesdna/DNA_windowmanager_types.h
@@ -307,7 +307,22 @@ typedef struct wmWindow {
    */
   short pie_event_type_last;
 
-  /** Storage for event system. */
+  /**
+   * Storage for event system.
+   *
+   * For the most part this is storage for `wmEvent.xy` & `wmEvent.modifiers`.
+   * newly added key/button events copy the cursor location and modifier state stored here.
+   *
+   * It's also convenient at times to be able to pass this as if it's a regular event.
+   *
+   * - This is not simply the current event being handled.
+   *   The type and value is always set to the last press/release events
+   *   otherwise cursor motion would always clear these values.
+   *
+   * - The value of `eventstate->modifiers` is set from the last pressed/released modifier key.
+   *   This has the down side that the modifier value will be incorrect if users hold both
+   *   left/right modifiers then release one. See note in #wm_event_add_ghostevent for details.
+   */
   struct wmEvent *eventstate;
   /** Keep the last handled event in `event_queue` here (owned and must be freed). */
   struct wmEvent *event_last_handled;
diff --git a/source/blender/windowmanager/intern/wm_event_system.cc b/source/blender/windowmanager/intern/wm_event_system.cc
index 841df253dab..c26696704dd 100644
--- a/source/blender/windowmanager/intern/wm_event_system.cc
+++ b/source/blender/windowmanager/intern/wm_event_system.cc
@@ -5481,6 +5481,25 @@ void wm_event_add_ghostevent(wmWindowManager *wm, wmWindow *win, int type, void
         }
       }
 
+      /* NOTE(@campbellbarton): Setting the modifier state based on press/release
+       * is technically incorrect.
+       *
+       * - The user might hold both left/right modifier keys, then only release one.
+       *
+       *   This could be solved by storing a separate flag for the left/right modifiers,
+       *   and combine them into `event.modifiers`.
+       *
+       * - The user might have multiple keyboards (or keyboard + NDOF device)
+       *   where it's possible to press the same modifier key multiple times.
+       *
+       *   This could be solved by tracking the number of held modifier keys,
+       *   (this is in fact what LIBXKB does), however doing this relies on all GHOST
+       *   back-ends properly reporting every press/release as any mismatch could result
+       *   in modifier keys being stuck (which is very bad!).
+       *
+       * To my knowledge users never reported a bug relating to these limitations so
+       * it seems reasonable to keep the current logic. */
+
       switch (event.type) {
         case EVT_LEFTSHIFTKEY:
         case EVT_RIGHTSHIFTKEY: {