After losing focus, the key release of Ctrl+tab isn't delivered to the handler and the MRU window is shown "forever" (or until it's removed by repeated ctrl+tab).
There are 2 situations when focus loss may happen: 1. In the 600ms interval when the MRU popup isn't shown yet but tabs are already being switched. This is handled by checking whether the main window has focus in on_switch_timeout() and the MRU window isn't shown when the focus is lost. 2. After the MRU popup is shown. This is handled by connecting to "focus-out-event" of the main window and stopping the switch in its handler.
Fixes #3330. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/3907
-- Commit Summary --
* Fix stuck MRU dialog when Geany loses focus
-- File Changes --
M src/notebook.c (47)
-- Patch Links --
https://github.com/geany/geany/pull/3907.patch https://github.com/geany/geany/pull/3907.diff
The example given in #3330 is a bit artificial but it may happen that tab switching triggers some dialog popup which steals focus and then this situation happens.
This happens regularly with the LSP plugin where, for now until there's the official API, I show warning that both TM and LSP are active if there are tags in the current document. But I have seen it on other occasions too.
It seems to me that you (and others probably) are violating Geany's non-reentrancy. It is _not_ written to have operations happen in any way other than running to completion. Having something happen on another thread/mainloop while Geany is waiting for something will leave Geany stranded if that operation consumes the input that was meant for Geany. Thats why almost all popups and dialogs are modal, that stops the user causing something else to happen, but that doesn't stop plugins and sometimes even Geany itself doing it and breaking code that isn't written to handle it.
It seems to me that you (and others probably) are violating Geany's non-reentrancy.
I'm not sure what you mean by this. It's Geany's bug that it doesn't handle this situation correctly. The majority of keybindings work by waiting for a keybinding to happen once which typically works (or doesn't work if something steals focus in the meantime but that's alright and expected).
This particular keybinding is different though - it is triggered by Ctrl+Tab but then you keep holding Ctrl and list through documents through just pressing tab. So Ctrl is held by users for a relatively long time in which many things may happen and e.g. some popup stealing focus may appear - then the Ctrl release isn't delivered to Geany. And it's Geany's responsibility to handle it correctly.
The popup should be modal, nothing else should be able to steal focus. Almost all of Geany is written on the assumption that focus stealing, calls to Geany functions while other Geany functions are waiting for input and the like do not happen. They are likely to misbehave if unexpected things happen, like focus getting stolen from a modal dialog, by a plugin doing something at an unexpected moment (thats another of the recent Issues IIRC).
Fixing individual issues is going to be a game of whack a mole because almost all of Geany is not written to handle it, the whole thing needs to be carefully thought about, not just each individual bug swatted as it comes up when the timing happens to be wrong on someones system.
The popup should be modal, nothing else should be able to steal focus.
If I remember correctly, the popup cannot be modal as when you switch between documents, you need the Scintilla editor to have focus. I don't remember why exactly, I can try.
Fixing individual issues is going to be a game of whack a mole because almost all of Geany is not written to handle it
This is the only case where dialog appearance depends on a key being pressed and its disappearance on a key being released. Nothing else needs to be changed. I don't completely understand why to defend a stuck popup being shown as something non-buggy.
I don't completely understand why to defend a stuck popup being shown as something non-buggy.
Oh, I'm not defending it as non-buggy, I'm saying that I suspect there to be _lots of similar bugs_, as you said this one shows up easily because of the long timing between key down and key up, but it could in principle happen at any place Geany depends on events occurring in order (which is lots I suspect).
And its not just key events, anywhere there is an assumed order the save dialog assumes the document does not change while it is open, running a plugin at an awkward moment changing the document, or running on KDE that does not (or used not) respect modality and allows the user to change the current document resulting in the wrong document being saved, a similar one a while ago was getting an open request from the socket while a dialog was open caused the next operation to be applied to the new document not the original one.
The popup should be modal, nothing else should be able to steal focus.
If I remember correctly, the popup cannot be modal as when you switch between documents, you need the Scintilla editor to have focus. I don't remember why exactly, I can try.
Just out of my head, without testing anything, I think if the popup were made modal, the whole tab switching wouldn't work as for the switching itself the main window needs all the events to be delivered to it.
Just out of my head, without testing anything, I think if the popup were made modal, the whole tab switching wouldn't work as for the switching itself the main window needs all the events to be delivered to it.
Just tried to make it modal and it's not a good idea at all - if both the tab switching popup is displayed and there's another modal popup created by the tab switch, the two have the potential to block each other and the situation is much worse than now.
The patch I posted has problems, this focus stuff is tricky. I may end up deciding the current behavior is a feature :-).
If as you said on the original issue, just pressing ctrl+tab again dismisses it, then its acceptable IMO.
It's just that many things in Geany are written with a great attention to detail and it feels a bit clumsy when things like - the problem from this PR - https://github.com/geany/geany/pull/3909 - https://github.com/geany/geany/pull/3560
appear (but these are really the only ones of this nature I'm aware of). None of them are really serious problems, I've just been looking at them for a long time and thought it would be nice to have them fixed.
@b4n pushed 2 commits.
c8a9249e7a4a7587185fe49a472ffe0b10234cc2 Fix stuck MRU dialog when Geany main window loses focus, for realsies aac6b16d3eee1093b1a846320304992be85877c1 Close MRU dialog on Escape as well
@techee I shamelessly pushed 2 more commits, please see if that works better for you as well (your commit didn't seem to work really well for me…)
@elextr I agree with @techee that this is not plugins doing careless stuff, it's just this MRU handling is fairly poor regarding corner cases. How comes one can actually open a menu while it's running? And should it really cause problems? Should we *really* leave an ominous popup over everything just because the WM stole focus? I really think it's just plain Geany bugs. And if it's triggered by what plugins do *as well*, at worse plugins should take more care not to interfere with MRU switching, but MRU switching shouldn't explode mid-flight. Agreed, if there were *no other means* of triggering the issue than a plugin doing a very specific thing, we could potentially blame it on the plugin; but here it's far worse than that.
@b4n I said:
Oh, I'm not defending it as non-buggy, I'm saying that I suspect there to be lots of similar bugs
It seems like you are not getting the meaning of what I say.
Of course its Geany bugs, Geany does not meet the conditions for reentrant software, but thats not just MRU or this or that, as the examples I gave over a wide range of circumstances showed, and thats just the ones somebody has reported and we have investigated enough to understand. Geany is not tolerant of events out of their normal order, nobody has been careless or bad, it has simply not been written to be so.
But as we keep seeing, there are situations where that will happen, as you say out of our control, although things like plugins _are_ within our control, so they should not be violating limitations of Geany, and that is a _plugin_ bug which should be fixed there.
But it is unlikely that anyone will fix the underlying bugs in Geany, so for the situations not caused by plugins we play whac-a-mole[^1] every time such a bug pops up, and simply ignore bugs that are rare enough that they are not reported or not reproducible, thats fine if its a deliberate decision, just be aware we have molehills throughout our back yard and try not to stumble into them :grinning:.
[^1]: https://en.wikipedia.org/wiki/Whac-A-Mole
@techee pushed 2 commits.
4cfbf9e14e79d53bdffc1febcad701a526baf3b2 Connect notify::is-active on main_widgets.window only once a2548b66de7bd79c9811ebd244a952edbcf5a46a Make sure that there is at most one scheduled on_switch_timeout()
@techee I shamelessly pushed 2 more commits, please see if that works better for you as well (your commit didn't seem to work really well for me…)
Thanks! It really seems to work! Nice!
I've added two more commits on top of that - not sure if they are completely needed but may prevent some hard-to-find bugs: 1. I moved the connection of `notify::is-active` to `notebook_init()` so it's performed only once. 2. For the `g_timeout_add(600, on_switch_timeout, NULL)` I store the returned `source_id` and make sure that `on_switch_timeout()` is scheduled for execution only once - the previous code didn't guarantee that and there could be some surprising timing issues because of that.
github-comments@lists.geany.org