Release bindings for modifier keys are ignored in some cases

[fbbdev]

Under some conditions (apparently only when entering command line mode) release bindings for modifier keys (ctrl, shift) are not run. This results in the sampler tool remaining active or the brush remaining stuck in multi-mode (which could lead to damage or wasted work when the view is zoomed in on a single frame).

To reproduce (combinations given for my qwerty it layout):

• Slice image or add some frames;
• Type a colon (<shift>+.); exit the command line, multibrush will be active. Press and release <shift> to deactivate.
• Type a colon with <ctrl> pressed (<ctrl>+<shift>+.); exit the command line, sampler tool will be active. Press b to bring back the brush. Pressing and releasing <ctrl> won't help because sampler/off reverts to the previous tool which right now is the sampler itself.

[cloudhead]

The first issue should be fixed in de236db84f6f9cd96cbbfe1db65617b0200efa7a - the second issue I couldn't reproduce.

Do you mind trying this gain?

[fbbdev]

The second issue is still there for me. Also, I don't think the fix for the first issue is quite right, and this is why the second one is not resolved. Let me explain what I think was happening (before the fix):

--- NORMAL MODE
- <shift> PRESSED  -> run binding [:brush/set multi]
- <shift> RELEASED -> run binding [:brush/unset multi]

- <shift> PRESSED  -> run binding [:brush/set multi]
- '.' (or equivalent key modulo layout)
          PRESSED  -> character input ':' -> MODE SWITCH

--- COMMAND MODE
- '.'     RELEASED -> ignored
- <shift> RELEASED -> ignored
- <esc>   PRESSED  -> MODE SWITCH

--- NORMAL MODE
# <shift> release binding [:brush/unset multi] never run, so brush is still in multi mode

So,

Why is the fix incorrect?

1. The problem is still present for key mappings like <ctrl> or alternative mappings for <shift>: suppose I change my init.rx to include this line:

   map <shift> :zoom + {:zoom -}

   so that I can quickly zoom in and out by pressing shift. Now if I enter command mode (<shift>+. on my layout) the canvas zooms in and remains zoomed (just replace brush commands with zoom commands in the timeline above and you'll see this clearly).

2. Brush reset on mode switch is unexpected by users and could be confusing. Suppose I give the following command:

   :brush/set erase

   and I go on happily erasing without having to hold 'e'. Now I enter command mode and give some other command. When I return to normal mode, my eraser is unexpectedly gone.

What do I propose then?

IMO there are two ways to fix this:

1. In normal mode, keep track of the set of pressed keys (only those with release bindings); on mode switch, run all release bindings and empty the set. Upon coming back to normal mode, release events for keys that are not in the set should be ignored: this is to avoid a corner case where I could leave command mode with shift pressed, release it, and thus run the release binding a second time (the first one being upon entering command mode).

2. Always record the full set of pressed keys, in every mode. When leaving normal mode, store a snapshot. When entering normal mode again, compare the current set to the snapshot and run press handlers for newly pressed keys, release handlers for newly released keys. Then throw away the snapshot.

The first proposal is like your fix, but generalized to every possibile key mapping, not just brushes. Plus, it won't change brush state unexpectedly.

The second proposal is a bit more involved (actually, I'm not sure which of the two may be less complicated), a bit more general (future-proof for other modes etc.), but would still have a slightly confusing behavior: upon entering command mode, normal mode state is freezed as it is; then it would change suddenly when leaving command mode.

I'm not sure which of the two is better. Also, I'm sorry for just spitting out comments, I'd like to contribute some code (if you'd like it of course) but I don't feel I understand your code enough yet. Hope I'm not annoying you! Nice work anyway & thank you.

[cloudhead]

Thanks for your thorough comment, you're right - I didn't think it through!

How about this as a simpler solution: on mode switch, just run the release handlers for all keys pressed before entering that mode. It's a bit like your solution 2., but simpler to implement.

[cloudhead]

Here it is: 85014d0d69ccdcf818e006bf65b307fc38690785

I think this is all we need for the correct behavior.

[cloudhead]

map <shift> :zoom + {:zoom -}

This is a cool idea - it would be nice to be able to do this with any key, but at the moment you'll get key-repeat and it'll zoom in over and over, if you map to m for example. (Fixed)

[fbbdev]

Great idea with fixing the repeat issue based on the presence of a release binding! I didn't think of that.

As regards the original issue, 85014d0 works almost perfectly, thank you!

Your fix is mostly like my solution 1; still I'd like to point out that now we get the dual problem: leave the command line with <shift> or <ctrl> pressed (e.g. <shift>+<enter>, <shift>+<esc>); then, upon releasing the key, the release binding will run: but the corresponding press binding has not been run before! Of course this is less probable and less annoying but still a relevant corner case IMO, especially when one starts typing fast. So I suggest a couple simple changes that should make this fully equivalent to solution 1:

• At src/session.rs:1151 change drain with iter().cloned and let handle_keyboard_input remove entries from the set:

  let pressed: Vec<platform::Key> = self.keys_pressed.iter().cloned().collect();

• At src/session.rs:1859 change the statement to:

  if !self.keys_pressed.remove(&key) {
      return;
  }

That should do. Let me know what you think of it!

[cloudhead]

Yup, thanks for the fix!

[cloudhead]

Closing the issue - if you encounter more corner cases, just re-open :+1: