sudara / melatonin_inspector

A JUCE module that gives you the ability to inspect and visually edit (non-destructively) components in your UI.
MIT License
145 stars 17 forks source link

Add Keyboard toggle/icon and make key/mouse focus mutually exclusive #102

Closed sudara closed 6 months ago

sudara commented 7 months ago

This PR

Sine Machine v14 - 2024-03-17 57@2x

According to @baconpaul's comment here and my own futzing about, I'm making the assumption that when keyboard focus is enabled, mouse listening should be disabled. Everything feels too messy otherwise.

Note: Even with the overlay's mouse listening disabled, clicking on elements in JUCE UIs still does set keyboard focus by default, see https://docs.juce.com/master/classComponent.html#a5e27530ab343f52b524c1c3f1a1d98eb — in other words, most of the time focus will still change by clicking around the UI!

Questions:

sudara commented 7 months ago

Ah, I didn't catch that the JUCE_BEGIN_IGNORE_WARNINGS_GCC_LIKE code was added to an autogenerated file in #100. I'll have to add that to the ruby file that generates assets for it to persist...

baconpaul commented 7 months ago

Ahh sorry didn’t realize that was auto generated!

this change looks great. Having this choice be sticky in props would be lovely but I can also have sure menus launch different configs.

sudara commented 7 months ago

A couple UX annoyances I'd like to fix on this branch.

  1. If I click to select a component in the UI and then go to click the keyboard focus toggle, the component loses selection. More specifically, the component loses selection anytime you click back to the inspector or to another app and doesn't regain it.

  2. I'm still sometimes seeing the hover behavior still active even when FOLLOW_FOCUS is on.

  3. the key focus listener has to be removed before inspector is closed (in destructor)

Not totally sure this is the right move, but going to look into filtering the global focus callbacks to only apply those related to the UI's component peer.

sudara commented 7 months ago

Ok, that was a bit annoying to figure out.

Part of the "mutually exclusive" complication was that toggle was on a delayed timer — so on toggle, it would re-enable mouse listening to prevent some other UX glitchyness from happening.

The keyboard focus setting will now also be remembered.

I think I've cleanup up all the edge cases, with #104 as a bonus.

There are certainly some unideal things, like I don't feel 100% rock solid about the mouse listener enable/disable RAII situation, but I added a jassert to hopefully catch any issues bubbling up.

Going to live with this a bit longer before merging. Feel free to check it out too.

sudara commented 7 months ago

There's still one issue I'm not sure exists / how to resolve / if I'm just lacking some understanding.

setRoot currently adds a keyListener and setsWantsKeyboardFocus of the root element to true. It does this to listen for some keyboard shortcuts. Which I think doesn't work.

I'm wondering if setsWantsKeyboardFocus is undesirable — for me, when I toggle on key focus, it focuses on the root element. Not only is that hard to see (see #61) but I'm wondering if the focus should actually be given away (to some child) in my app.

This should be reproducible:

I think what's happening is that the root plugin UI is stealing focus, maybe incorrectly because of code in the inspector.

Where my understanding stalls out: I'd like to have some global keyboard shortcuts in my plugin. Does that mean setsWantsKeyboardFocus has to be true? It seems "bad" that a side effect of wanting shortcuts.

sudara commented 7 months ago

Ignoring globalFocusChanged calls from root/inspector just traded one glitchy feel for another, for example:

Reverting back to just ignoring nullptr focus events, otherwise letting them through.