Skip to content

Conversation

@dterrahe
Copy link
Member

Made some fixes to shortcut handling after a bit of a deep dive. There's always a chance that this "breaks stuff" so it would be good if people actually reported if anything stopped working for them. Maybe even before the end of the next release cycle, so that there's time to investigate, fix and test.

fixes #19888
The issue there is that some windows managers send hardware events, like "wake up" or "lid closed" as key presses to gtk and eventually dt. It may be that all of them do but maybe due to timing issues they don't make it all the way to the application. Maybe wayland buffers them until the application is "ready" to receive them. Whatever the case may be, these events only send key presses, not releases. And since dt keeps track of all the keys that it thinks are currently held down, these keys never clear. There's a break-lock "solution" that a short key press (doesn't have to be escape key) while other keys are "held" releases all of them, but having to use that every time you start dt gets annoying.
The simple solution is to just "ignore all the hardware event keys". But there's no clean way to recognise them. Gtk puts them all in the 0x1008ff00-0x1008ffff range, but also includes "media" (mute, pause) and "shortcut" (mail, calendar, search) there. Some people might want to use those in dt (I do; otherwise I'd lose the whole green top row on my bloomberg keyboard) but otherwise might want to ignore them and let them be handled by the operating system (which may require returning FALSE in the event handler). The solution I've chosen is to check if the key is used as a shortcut (in any combination with mouse events) and if not to ignore it. Unless you're creating a new shortcut; so don't close your laptop lid while in mapping mode!
It would be very good to hear back if this solves the issue people have and if the test fails for some situations that I haven't tested.

This also means that you're no longer receiving the "key not assigned" messages for keys that are not used yet. I haven't removed the code for that message yet, even though I believe it is now unreachable code, because having more messages for unexpected cases can be helpful. See the next section.

@ralfbrown you may find the following an interesting one as it took me a while to figure out.
Testcase: in darkroom, press&release F1-F5 all at the same time. This should toggle all 5 color labels. What instead happens is that sometimes one, sometimes a few, sometimes all get toggled.

The reason is that when processing the first received shortcut, dt_colorlabels_toggle_label_on_list first calls dt_gui_cursor_set_busy. That sets the mouse cursor, processes outstanding gdk events to make sure the changed mouse is immediately visible and puts the grab on the "progress" area, so that nothing else on the screen can be manipulated with the mouse. When done, dt_gui_cursor_clear_busy switches the mouse back and processes events again. However, that also leads to further keyboard events being picked up. In this case, other shortcuts to toggle color labels. But before it gets that far, the shortcuts system checks if there's maybe a grab on progress, as an indication that a long running process is going on in the foreground and that all shortcuts should temporarily be ignored. Since the grab hasn't been released yet, it indeed concludes that to be the case and drops (some of) the remaining shortcuts.

I solved the problem by moving the call to dt_gui_process_events in dt_gui_cursor_clear_busy to the end, after gtk_grab_remove. But I'm thinking that it could be removed altogether, since after clear_busy the gui should be responsive again and the mouse cursor update would be processed in reasonable time as normal. And if it isn't that's probably because an immediately following event sets it to busy again, so why bother showing the normal mouse cursor in between.

While digging into this, I also noticed an issue when you want to use multiple shortcut keys at once, for example to manipulate a section of a curve. Say you've got F1-F9 mapped to the nodes in the tone equaliser and you have fallbacks enabled. You can then hold F3+F4+F5 and use the scroll wheel to move all three of those nodes simultaneously. If you add F6, that will also start moving. If you now release F3, the others should continue responding to the mouse wheel, but they didn't. Because releasing F3 after a long while would set the "long press" flag and that would invalidate all subsequent shortcut activations. Now fixed.

@dterrahe dterrahe added this to the 5.6 milestone Dec 11, 2025
@dterrahe dterrahe added bugfix pull request fixing a bug priority: medium core features are degraded in a way that is still mostly usable, software stutters scope: UI user interface and interactions labels Dec 11, 2025
@ralfbrown
Copy link
Collaborator

I solved the problem by moving the call to dt_gui_process_events in dt_gui_cursor_clear_busy to the end, after gtk_grab_remove. But I'm thinking that it could be removed altogether, since after clear_busy the gui should be responsive again

I don't remember specifically why I added the process_events, but it's likely there is some place the busy functions get called where the main event loop is not running.

@dterrahe
Copy link
Member Author

The event loop is running, but since we're in the event loop, we're the one blocking it. So it will be unresponsive until we return, which is why we're showing the busy cursor. But to make that actually show, we'll have to process the gui events that show it, which is why we're calling the event loop reentrently. Which makes perfect sense and is needed in dt_gui_cursor_set_busy otherwise the busy cursor would only appear once we are done already. But in dt_gui_cursor_clear_busy there's no hurry to show the "available" cursor again. In fact, if somehow the event loop is still occupied, it is fine if the busy cursor continues showing until the "normal" event loop catches up (or until we switch busy back on for the next operation in the for loop), which is when we're really available again. So I would favor removing dt_gui_process_events from dt_gui_cursor_clear_busy only. Does that make sense to you too?

As a side note, gtk4 discourages reentrantly calling the event loop. This is what gtk_dialog_run used to do which they removed. Instead one is supposed to just show the dialog and then wait for it to send "response" signals. But now it seems GtkDialog itself is also deprecated so one continues playing catchup...

It would also mean (and this kindof makes sense) that any process that could take more than a few ui cycles should move to a different thread. So in this case we process the color label toggle in the ui thread because it usually is done to one image at a time. But if the whole library is selected it could block the ui for minutes. I guess the best way to do this is to have the ui event/shortcut handler simply forward commands (and the act_on images) to a queue which processes them in the background one at a time (and is integrated with the undo stack). This would also solve #19877. I wouldn't be surprised if gtk4 has features to support this (haven't looked!) but I would be hesitant to integrate too much with whatever their current approach is (even ignoring the amount of work involved).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix pull request fixing a bug priority: medium core features are degraded in a way that is still mostly usable, software stutters scope: UI user interface and interactions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"Wake up,left click not assigned" with window managers (DWM,Hyprland)

2 participants