PR https://github.com/geany/geany/pull/3267 broke handling of several special tab switch cases:
* By showing the tab's child too early, it prevented the switch-page handler to properly work in situations where there was only one page, as the tab switch would happen at page creation yet the document would not be valid yet. * It removed explicit emission of the switch-page signal when the source and target pages are the same after session opening, which happens if the active session page is the first one.
Fix this by mostly reverting 23367de0c5237558ea63cb8c711d46390d978267 and implementing the delay directly in the switch-page handler.
Fixes #3684. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/3891
-- Commit Summary --
* Fix emission of document-activate signal and associated UI glitches
-- File Changes --
M src/callbacks.c (61) M src/document.c (39) M src/editor.c (7) M src/keyfile.c (6) M src/libmain.c (2) M src/notebook.c (2)
-- Patch Links --
https://github.com/geany/geany/pull/3891.patch https://github.com/geany/geany/pull/3891.diff
@b4n commented on this pull request.
- /* FIXME: is that actually useful?? I don't see any difference disabling this code... */
+ if (!main_status.opening_session_files) + document_try_focus(doc, NULL);
This I don't know, I couldn't find a case where it was needed, but I guess there was a reason for it…
@techee approved this pull request.
Yeah, it's much better this way - the callers of `document_show_tab()` don't have to care about the details of whether it should be on idle or not and the whole logic is in one place.
Also fixes the problem of the missing `document-activate` signal from what I tested. Just to clarify, `Notebook::switch-page` is fired also for a single tab, right? The documentation isn't completely clear about it but since `document-activate` is emitted in this case, it seems to be the case.
Also fixes the problem of the missing `document-activate` signal from what I tested. Just to clarify, `Notebook::switch-page` is fired also for a single tab, right? The documentation isn't completely clear about it but since `document-activate` is emitted in this case, it seems to be the case.
Yes, it is. As the docs is indeed not entirely clear, I did some empirical testing with a standalone sample app and it showed to be consistent. And as you noted, it does work for our use-case in practice.
Anybody else wanting to test this? If not, I'll merge this in a couple days and you'll all be *forced* to test it :smiling_imp:
Can't test, but LGBII
@b4n pushed 1 commit.
293157c7a99880523900e29a1e3191800c2018fe Fix emission of document-activate signal and associated UI glitches
I was about to merge, but spotted a potential bug: in the unlikely case `closing_call` (that's *not* quitting, which mislead me) is set when the idle callback runs, it would fail to rested the callback ID yet remove the callback. This could lead to failing to trigger the callback (if next run is `opening_session_files`, unlikely I guess -- didn't check), or trying to remove an invalid (or worse, reused) callback ID.
All I changed is that the ID is properly reset in this case too -- any time the callback returns `G_SOURCE_REMOVE`.
in the unlikely case closing_call
closing_all?
it would fail to rested the callback ID
its important they are rested, we can't have tired callback IDs :grinning:
Merged #3891 into master.
github-comments@lists.geany.org