Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shortcut routing for an active item #7618

Closed
nikitablack opened this issue May 24, 2024 · 16 comments
Closed

Shortcut routing for an active item #7618

nikitablack opened this issue May 24, 2024 · 16 comments
Labels

Comments

@nikitablack
Copy link

nikitablack commented May 24, 2024

Version/Branch of Dear ImGui:

Version 1.90.1, Branch: docking

Back-ends:

imgui_impl_glfw.cpp

Compiler, OS:

Ubuntu 20.04, imgui_impl_glfw

Full config/build information:

No response

Details:

In this scenario:

shortcut

I want to achieve the following:

  1. Register a global shortcut, which is activated when nothing is in focus or the same shortcut was not intercepted by other windows.
  2. Register the same shortcut for a docking window (not shown on the picture).
  3. The text input should intercept the shortcut.

According to the docs, the routing is organized like this:

GlobalHigh > Focused (when owner is active item) > Global > Focused (when focused window) > GlobalLow

Following this logic I added to my main.cpp:

if (ImGui::Shortcut(ImGuiKey_M, 0, ImGuiInputFlags_RouteGlobalLow)) {
    std::cout << "Global M" << std::endl;
}

And for the text input:

ImGui::InputText("##Address", ...);
ImGui::Shortcut(ImGuiKey_M, ImGui::GetID("##Address"), ImGuiInputFlags_RouteFocused);

While preventing to fire a global shortcut during text input usage, it also intercepts the shortcut when the text not in focus, but the window where this text is a child is in focus. In other words, the line ImGui::SetShortcutRouting(ImGuiKey_M, ImGui::GetID("##Address"), ImGuiInputFlags_RouteFocused) returns true even when the text input not focused, but the parent window is.

@ocornut ocornut added the inputs label May 24, 2024
@ocornut
Copy link
Owner

ocornut commented May 24, 2024

I am going to investigate this.

Please note that I have just started making some of this public API, and before doing so I have pushed various breaking changes:

  • b4f564c Renamed ImGuiInputFlags_RouteGlobalLow -> ImGuiInputFlags_RouteGlobal, ImGuiInputFlags_RouteGlobal -> ImGuiInputFlags_RouteGlobalOverFocused, ImGuiInputFlags_RouteGlobalHigh -> ImGuiInputFlags_RouteGlobalHighest
  • 55748cd Renamed ImGuiKeyOwner_None to ImGuiKeyOwner_NoOwner.
  • 900b290 Swapped two last default parameters of Shortcut().
  • 85513de Swapped two last default parameters for owner-aware versions of IsKeyPressed(), IsKeyChordPressed(), IsMouseClicked().

@ocornut
Copy link
Owner

ocornut commented May 24, 2024

ImGui::Shortcut(ImGuiKey_M, ImGui::GetID("##Address"), ImGuiInputFlags_RouteFocused);
While preventing to fire a global shortcut during text input usage, it also intercepts the shortcut when the text not in focus, but > the window where this text is a child is in focus. In other words, the line ImGui::SetShortcutRouting(ImGuiKey_M, ImGui::GetID("##Address"), ImGuiInputFlags_RouteFocused) returns true even when the text input not focused, but the parent window is.

I realize it may be confusing but this is correct as you are using the ImGuiInputFlags_RouteFocused route.
If you passed an arbitrary id (not associated to an item) it would still allow registering itself into focus route.

I believe you would want to do:

if (ImGui::IsItemActive() && ImGui::Shortcut(ImGuiKey_M, ImGuiInputFlags_RouteFocused, ImGui::GetItemID()))
...

Which is effectively essentially what widget code are doing themselves.

But I realize it could make sense to add a helper ImGuiInputFlags_RouteActivated to shorten this?

ocornut added a commit that referenced this issue May 24, 2024
@ocornut
Copy link
Owner

ocornut commented May 24, 2024

I added ImGuiInputFlags_RouteActiveItem to do this check:

if (ImGui::Shortcut(ImGuiKey_M, ImGuiInputFlags_RouteGlobal))
    IMGUI_DEBUG_LOG("Global\n");
....
static char buf[32];
ImGui::InputText("##Address", buf, 32);
if (ImGui::Shortcut(ImGuiKey_M, ImGuiInputFlags_RouteActiveItem, ImGui::GetItemID()))
    IMGUI_DEBUG_LOG("InputText shortcut\n");

@ocornut ocornut changed the title Shortcut routing needs a refinement. Shortcut routing for an active item May 24, 2024
@nikitablack
Copy link
Author

nikitablack commented May 24, 2024

I believe the changes you posted will not help me. Here's the minimal reproducible example of my problem:

if (ImGui::Shortcut(ImGuiKey_M, 0, ImGuiInputFlags_RouteGlobalLow)) {
    logger::Logger::log("Global M");
}

ImGui::Begin("AAA");
static char str[256];
ImGui::InputText("ABC", str, 256);
ImGui::Shortcut(ImGuiKey_M, ImGui::GetItemID(), ImGuiInputFlags_RouteFocused);
ImGui::End();

Here, the Global M only printed when the window AAA is not in focus, which is not what I want. If I change the global shortcut from ImGuiInputFlags_RouteGlobalLow to ImGuiInputFlags_RouteGlobal, as you suggested, then adding the second window breaks the flow:

if (ImGui::Shortcut(ImGuiKey_M, 0, ImGuiInputFlags_RouteGlobal)) {
    logger::Logger::log("Global M");
}

ImGui::Begin("AAA");
static char str[256];
ImGui::InputText("ABC", str, 256);
ImGui::Shortcut(ImGuiKey_M, ImGui::GetItemID(), ImGuiInputFlags_RouteFocused);
ImGui::End();

ImGui::Begin("BBB");
if (ImGui::Shortcut(ImGuiKey_M, 0, ImGuiInputFlags_RouteFocused)) {
    logger::Logger::log("BBB M");
}
ImGui::End();

In this case:

  • the Global M is not printed when the text input is active, which is good
  • the Global M is printed when the AAA is in focus, but the text input not, which is good
  • the Global M is printed when the BBB is in focus, which is bad, since I'm expecting BBB M to be printed.

The last point happens because Global > Focused (when focused window).

@ocornut
Copy link
Owner

ocornut commented May 24, 2024

If I change the global shortcut from ImGuiInputFlags_RouteGlobalLow to ImGuiInputFlags_RouteGlobal, as you suggested,

I didn't, it is because last week ImGuiInputFlags_RouteGlobalLow got renamed to ImGuiInputFlags_RouteGlobal as pointed in my earlier post. So you ImGuiInputFlags_RouteGlobalLow is currently ImGuiInputFlags_RouteGlobal.
That's where the main confusion is.

My point is this is incorrect (or at least not what you actually want):

ImGui::InputText(...);
ImGui::Shortcut(ImGuiKey_M, ImGui::GetItemID(), ImGuiInputFlags_RouteFocused);

This in latest works as expected:

if (ImGui::Shortcut(ImGuiKey_M, ImGuiInputFlags_RouteGlobal))
    IMGUI_DEBUG_LOG("Global\n");

ImGui::Begin("AAA");
static char buf[32];
ImGui::InputText("##Address", buf, 32);
if (ImGui::Shortcut(ImGuiKey_M, ImGuiInputFlags_RouteActive, ImGui::GetItemID()))
    IMGUI_DEBUG_LOG("AAA\n");
ImGui::End();

ImGui::Begin("BBB");
if (ImGui::Shortcut(ImGuiKey_M))
    IMGUI_DEBUG_LOG("BBB\n");
ImGui::End();

@ocornut
Copy link
Owner

ocornut commented May 24, 2024

And in your older version, instead of:

if (ImGui::Shortcut(ImGuiKey_M, ImGuiInputFlags_RouteGlobal))
    IMGUI_DEBUG_LOG("Global\n");

You can write:

if (ImGui::Shortcut(ImGuiKey_M, 0, ImGuiInputFlags_RouteGlobalLow))
    IMGUI_DEBUG_LOG("Global\n");

And instead of:

if (ImGui::Shortcut(ImGuiKey_M, ImGuiInputFlags_RouteActive, ImGui::GetItemID()))
    IMGUI_DEBUG_LOG("AAA\n");

You can write:

if (ImGui::IsItemActive() && ImGui::Shortcut(ImGuiKey_M, ImGui::GetItemID(), ImGuiInputFlags_RouteFocused))
    IMGUI_DEBUG_LOG("AAA\n");

For the same effect.

@nikitablack
Copy link
Author

nikitablack commented May 24, 2024

Thank you, @ocornut. Your suggested code works. The new changes (ImGuiInputFlags_RouteActive) are much better, than

if (ImGui::IsItemActive()){
    ImGui::Shortcut(ImGuiKey_M, ImGui::GetItemID(), ImGuiInputFlags_RouteFocused)
}

but still looks strange - after all, when using an InputText it is implied that I don't want any shorcuts to fire (except for the widget itself, of course).

@ocornut
Copy link
Owner

ocornut commented May 24, 2024

but still looks strange - after all, when using an InputText it is implied that I don't want any shorcuts to fire (except for the widget itself, of course).

Not exactly: people want other shortcuts such as Ctrl+S to work.

But here.... just realized there is bug, it shouldn't do it with M only, because key emitting characters should normally automatically be filtered out by the active InputText(), something here got broken.

@ocornut
Copy link
Owner

ocornut commented May 24, 2024

Actually I think this debatable, here by passing GetItemID() you are pretending to be the InputText(), you can receive the shortcut. Things normally work because InputText() doesn't use keys that would typically emit characters.

Can you clarify what you actual use case is, with this M shortcut you want to activate when InputText() is active?

@ocornut
Copy link
Owner

ocornut commented May 24, 2024

The culling logic for keys that maps to characters only apply if you are NOT the active item:

// Specific culling when there's an active item.
if (g.ActiveId != 0 && g.ActiveId != owner_id)
{
    // Cull shortcuts with no modifiers when it could generate a character.
    // e.g. Shortcut(ImGuiKey_G) also generates 'g' character, should not trigger when InputText() is active.
    // but  Shortcut(Ctrl+G) should generally trigger when InputText() is active.
    // TL;DR: lettered shortcut with no mods or with only Alt mod will not trigger while an item reading text input is active.
    // (We cannot filter based on io.InputQueueCharacters[] contents because of trickling and key<>chars submission order are undefined)
    if ((flags & ImGuiInputFlags_RouteFocused) && g.IO.WantTextInput && IsKeyChordPotentiallyCharInput(key_chord))
    {
        IMGUI_DEBUG_LOG_INPUTROUTING("SetShortcutRouting(%s, flags=%04X, owner_id=0x%08X) -> filtered as potential char input\n", GetKeyChordName(key_chord), flags, owner_id);
        return false;
    }

Which IMHO makes sense.

I think sensible answer and design can only come if I understand what you are trying to achieve in the first place with this M shortcut on an active InputText().

@nikitablack
Copy link
Author

I have a list of global actions to perform - a dozen of tasks bound to different single letter keys (I'm porting a legacy app to imgui, the functionality should stay the same). Obviously, I don't want to fire any of the actions when typing in the text input. Also, some windows should intercept single key presses and use them on their own, like 3D navigation with WASD.

The solution you posted kinda work, but requires to abbreviate all occurrences of InputText with all existing shortcuts, which is annoying and will break in the future when new global shortcuts will be introduced. Also, some windows I'm planning to make as plugins, i.e. give the possibility to users bring functionality which does not present in the base application. Obviously, they don't know which keys are bound to global actions.

@ocornut
Copy link
Owner

ocornut commented May 24, 2024

Ah indeed the mixed key<>char filtering without mods should have been applied for global routes as well, my mistake: 7832e6a

I'm writing new sets of tests right now.

ocornut added a commit to ocornut/imgui_test_engine that referenced this issue May 24, 2024
@ocornut
Copy link
Owner

ocornut commented May 24, 2024

I have amended the inputs_routing_char_filter test to include global route:
ocornut/imgui_test_engine@dffee6a
inputs_routing_char_filter

When you have time could you update to latest and make sure everything works as expected?

Thanks for your help and feedback!

@ocornut ocornut closed this as completed May 24, 2024
@nikitablack
Copy link
Author

I updated to latest and everything works now - I don't need to Shortcut an InputText for a single-letter input. Also, with ImGuiInputFlags_RouteActive the code is more clearer. Thank you very much for your support and super fast responses.

@ocornut
Copy link
Owner

ocornut commented May 24, 2024

I am curious however when/how you are using ImGuiInputFlags_RouteActive, because semantically it would make sense if you want to extend/add shortcut to the InputText(), and hopefully you don't need to use it as a workaround for another thing?

@nikitablack
Copy link
Author

That's right, I tested it with Ctrl+S shortcut and an InputText(). Atm, I don't need it though, just tested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants