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

OSX backend: Modifier Keys broken, including Command + C/V/Z #4253

Closed
richardlalancetteyoui opened this issue Jun 21, 2021 · 18 comments
Closed

Comments

@richardlalancetteyoui
Copy link

Version/Branch of Dear ImGui:

Version: 1.83
Branch: master (98876b4)

Back-end/Renderer/Compiler/OS

Back-ends: imgui_impl_osx, imgui_impl_opengl2
Compiler: Apple Clang
Operating System:
Screen Shot 2021-06-21 at 11 12 50 AM

My Issue/Question:
Copy/Paste broken?
The issue I am seeing is using Command+C and Command+V.
Instead of copying to the clipboard and pasting from the clipboard, a C and a V are inserted into the text.

I am convince this is related to ResetKeys() in imgui_impl_osx.mm

Screenshots/Video
Command C/V/Z busted here.

Screen.Recording.2021-06-21.at.11.28.14.AM.mov

Standalone, minimal, complete and verifiable example: (see #2261)
Screen Shot 2021-06-21 at 11 18 49 AM

@richardlalancetteyoui
Copy link
Author

richardlalancetteyoui commented Jun 21, 2021

Narrowed down to this line.

//io.KeyCtrl = io.KeyShift = io.KeyAlt = io.KeySuper = false;

If I remove this line, then the bug goes away, but another bug shows up. Pasting will paste repetively if I hold Command down.

@ocornut
Copy link
Owner

ocornut commented Jun 21, 2021

6d53884 added the modifiers clear which triggered the bug, but it's merely consistent with clearing the keys.
That backend is a severe mess (partly because of a limitation in the IO system, which will be lifted by io_queue work), generally we recommend people using the SDL or GLFW backend on OSX.

In earlier e54b61a (second to initial commit)

  • in ImGui_ImplOSX_HandleEvent() the "// We must reset in case we're pressing a sequence of special keys while keeping the command pressed" block was modified by, either version are equally bizarre.
  • NSEventTypeFlagsChanged handling also calls resetKeys()

Someone would need to do a deep dive in figuring out why the code was designed this way with the resetKeys() calls.

@ocornut ocornut added the bug label Jun 21, 2021
@richardlalancetteyoui
Copy link
Author

OK, got little further.

This is getting called multiple times.

        g_pasteEventCount++;
        NSPasteboard *pasteboard = [NSPasteboard generalPasteboard];
        NSString *available = [pasteboard availableTypeFromArray:[NSArray arrayWithObject:NSPasteboardTypeString]];
        if (![available isEqualToString:NSPasteboardTypeString])
            return NULL;
        NSString *string = [pasteboard stringForType:NSPasteboardTypeString];
        if (string == nil)
            return NULL;
        const char *string_c = (const char *)[string UTF8String];
        size_t string_len = strlen(string_c);
        static ImVector<char> s_clipboard;
        s_clipboard.resize((int)string_len + 1);
        strcpy(s_clipboard.Data, string_c);
        ///[NSPasteboard generalPasteboard].clearContents;
        return s_clipboard.Data;
    };```

@richardlalancetteyoui
Copy link
Author

richardlalancetteyoui commented Jun 22, 2021

Little further:

The following seems to be set to allow repeat by default. If I pass in false to the repeat parameter, the bug goes away.
But that causes successive COMMAND+V to work only once for as long as I have COMMAND down.
...

const bool is_paste = ((is_shortcut_key && IsKeyPressedMap(ImGuiKey_V, false)) || (is_shift_key_only && IsKeyPressedMap(ImGuiKey_Insert))) && !is_readonly;

@ocornut ocornut changed the title Modifier Keys broken, including Command + C/V/Z OSX backend: Modifier Keys broken, including Command + C/V/Z Jun 22, 2021
@ocornut
Copy link
Owner

ocornut commented Jun 22, 2021

I think the issue is strictly at the imgui_impl_osx.mm level and you can look at the inputs provided by the backend in Demo>Inputs, Navigation & Focus>Keyboard State

@rokups
Copy link
Contributor

rokups commented Jun 23, 2021

The fix (0b8a247) was merged. Funny story. Turns out we had KeyCtrl and KeySuper mixed up in a shortcut check. But it did work when code was first added, because older version of sample swapped these keys to get PC-like shortcuts on a Mac. Eventually that change was lost and this function broke and nobody noticed for a long while since this backend is hardly used. It really is better idea to use SDL/GLFW as @ocornut said. 👍🏻

@ocornut
Copy link
Owner

ocornut commented Jun 23, 2021

Closing as fixed, Richard let us know if you find other problems this is backend.

@ocornut ocornut closed this as completed Jun 23, 2021
@richardlalancetteyoui
Copy link
Author

Great! @rokups thank you so much!
Merci Omar.

@richardlalancetteyoui
Copy link
Author

Yup, fixed!
Onto next issue. Thanks guys.

@richardlalancetteyoui
Copy link
Author

richardlalancetteyoui commented Jun 23, 2021

Going to use this thread to continue with the root issue that brought me to update:

In
const bool is_paste = ((is_shortcut_key && IsKeyPressedMap(ImGuiKey_V)) || (is_shift_key_only && IsKeyPressedMap(ImGuiKey_Insert))) && !is_readonly;
when I release ImGuiKey_V, and leave Command Down, is_paste returns true still.

Before I dive in, is it possible that g.IO.KeyMap would also need clearing?
This is used in inline bool IsKeyPressedMap(ImGuiKey key, bool repeat = true) { ImGuiContext& g = *GImGui; const int key_index = g.IO.KeyMap[key]; return (key_index >= 0) ? IsKeyPressed(key_index, repeat) : false; }?

@ocornut
Copy link
Owner

ocornut commented Jun 23, 2021 via email

@richardlalancetteyoui
Copy link
Author

richardlalancetteyoui commented Jun 23, 2021

Auto-repeat is fine, but the V is not down, so I would expect the paste command to not repeat.

@richardlalancetteyoui
Copy link
Author

richardlalancetteyoui commented Jun 23, 2021

Ah ok. I suspect something is happening in here.

bool ImGui::IsKeyPressed(int user_key_index, bool repeat)
{
    ImGuiContext& g = *GImGui;
    if (user_key_index < 0)
        return false;
    IM_ASSERT(user_key_index >= 0 && user_key_index < IM_ARRAYSIZE(g.IO.KeysDown));
    const float t = g.IO.KeysDownDuration[user_key_index];
    if (t == 0.0f)
        return true;
    if (repeat && t > g.IO.KeyRepeatDelay)
        return GetKeyPressedAmount(user_key_index, g.IO.KeyRepeatDelay, g.IO.KeyRepeatRate) > 0;
    return false;
}

@ocornut
Copy link
Owner

ocornut commented Jun 23, 2021 via email

@richardlalancetteyoui
Copy link
Author

richardlalancetteyoui commented Jun 23, 2021

Ah gotcha! It's whatever the backend feeds into input that is an issue. Gotcha.

@richardlalancetteyoui
Copy link
Author

richardlalancetteyoui commented Jun 24, 2021

Yup, looks like this is never called.

-(void)keyUp:(NSEvent *)event
Breakpoint won't get hit.
edit: That is if the command is down while releasing V key.

Onward!

Screen Shot 2021-06-24 at 1 31 18 PM

@richardlalancetteyoui
Copy link
Author

richardlalancetteyoui commented Jun 24, 2021

Fixed with this, if you guys want to bring this in.

-(void)updateAndDrawDemoView
{
    // Start the Dear ImGui frame
	ImGui_ImplOpenGL2_NewFrame();
	ImGui_ImplOSX_NewFrame(self);
    ImGui::NewFrame();

    [NSEvent addLocalMonitorForEventsMatchingMask:NSEventMaskKeyUp handler:^NSEvent*(NSEvent *event)
    {
        ImGuiIO& io = ImGui::GetIO();

        if (io.KeySuper)
        {
            memset(io.KeysDown, 0, sizeof(io.KeysDown));
        }

        return event;
    }];


...

rokups added a commit to rokups/imgui that referenced this issue Jun 25, 2021
…state when using shortcuts with CMD. (ocornut#4253)

Fix follows event capture scheme of example_apple_metal, where this issue is not present.
ocornut pushed a commit that referenced this issue Jun 25, 2021
…state when using shortcuts with CMD. (#4253)

Fix follows event capture scheme of example_apple_metal, where this issue is not present.
@ocornut
Copy link
Owner

ocornut commented Jun 25, 2021

Instead the apple+opengl2 example didn't have the same events registration as apple+metal
Fixed by https://github.com/ocornut/imgui_private/commit/2a6870be2ff6bb32642a984b72d3cadb69e7112a

There are still too many differences between both examples, we should aim to reduce them.

ocornut pushed a commit that referenced this issue Dec 13, 2021
…mouse cursor shape unconditionally. (#4759, #4253, #1873)

Note the original FIXME: refered to GLFWs Cocoa implementation, which is largely what this commit provides.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants