When the `tab_order_beside` option is enabled, tabs are opened on either side of the active tab, depending on `tab_order_ltr`, rather than at either end.
However, when loading startup files we don't switch to the last opened document right away, but postpone this to the very end. This affects the order tabs are loaded, as the active one is always the first one at this stage.
Rather than over-complicating the loading order to compensate the various options' effect, temporarily revert to an set of options that is easy to handle, and restore it after loading is finished. This also simplifies the existing logic as a basic forward iteration is now enough.
Fixes #3609. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/3611
-- Commit Summary --
* Fix startup files order when placing tabs next to the current one
-- File Changes --
M src/keyfile.c (25)
-- Patch Links --
https://github.com/geany/geany/pull/3611.patch https://github.com/geany/geany/pull/3611.diff
LGBI
@eht16 @techee @elextr I added that to %2.0, but if it's not new since 1.38 and you don't feel it's simple enough, we can postpone. It's actually not a problem with the default settings, so it's less bad -- although any sane person would have enabled the problematic option :grin:
@eht16 @techee @elextr I added that to %2.0, but if it's not new since 1.38 and you don't feel it's simple enough, we can postpone. It's actually not a problem with the default settings, so it's less bad -- although any sane person would have enabled the problematic option 😁
Looks good to me based on the code itself and I think it could go to 2.0. I just haven't tested it yet (which I'm going to do now).
Looks good to me based on the code itself and I think it could go to 2.0. I just haven't tested it yet (which I'm going to do now).
I just tested it and, yes, it is fixed, and, yes, the re-arrangement is really annoying without this patch so I think it should go to 2.0. (Somehow, I didn't have the option to place new tabs behind the current one enabled so I didn't notice the problem.)
One unrelated glitch I noticed is that when you close Geany with some files opened, and then open it from the command-line with some file as an argument, this file is always opened in the second tab - I would expect it to open behind the tab which was last active when Geany closed. I expect that when the command-line file gets opened, the "current" file is still the first one so it appears behind it. But not really a big problem and maybe even not worth fixing as users hopefully forget which tab was active when they closed Geany :-).
@techee approved this pull request.
@eht16 approved this pull request.
Tested and works cleanly.
I'm all in for merge.
Actually if it's like this in 1.38 already (and possibly earlier) I see no compelling reason to rush this into 2.0, considering that tomorrow is release.
It's not even clear to me if this is even a bug (as on "not working as intended"). I surely never used that pref.
@kugel- commented on this pull request.
/* necessary to set it to TRUE for project session support */ main_status.opening_session_files++;
- i = file_prefs.tab_order_ltr ? 0 : (session_files->len - 1); - while (TRUE) + file_prefs.tab_order_beside = FALSE; + file_prefs.tab_order_ltr = TRUE;
Why do you override the pref temporarily? Is there some nested function call that looks at the pref?
although any sane person would have enabled the problematic option 😁
@b4n Hah, it seems you have proven me as crazy again, my config says `tab_order_beside=false` :)
@b4n commented on this pull request.
/* necessary to set it to TRUE for project session support */ main_status.opening_session_files++;
- i = file_prefs.tab_order_ltr ? 0 : (session_files->len - 1); - while (TRUE) + file_prefs.tab_order_beside = FALSE; + file_prefs.tab_order_ltr = TRUE;
Yes, `notebook_new_tab()`. Call chain is `open_session_file() → document_open_file_full() → document_create() → notebook_new_tab()`.
It's not even clear to me if this is even a bug (as in "not working as intended"). I surely never used that pref.
I hardly can imagine this could be intended behaviour. After start, the previous code opened the tabs in some random (maybe not really random but in some for the user undeterministic order) and after a second start the previous order was restored, on the third start the pseudo random order of the first start is used and so on. At least me would be very confused by this. I don't mind much if this is a new issue or already present in 1.38, since we have a fix already and it is rather trivial, I'm still for merging.
@kugel-
It's not even clear to me if this is even a bug (as in "not working as intended"). I surely never used that pref.
Well, to me it's definitely a bug as the session is *not* restored as it was saved. Both documents order and current document are lost (well, current document is OK if it's the first one -- and that's likely why I didn't notice very often as I usually have the same doc I come back to a lot as the first one).
And having added the feature back then, I can tell you I didn't plan for this :)
@kugel- commented on this pull request.
/* necessary to set it to TRUE for project session support */ main_status.opening_session_files++;
- i = file_prefs.tab_order_ltr ? 0 : (session_files->len - 1); - while (TRUE) + file_prefs.tab_order_beside = FALSE; + file_prefs.tab_order_ltr = TRUE;
Then I think `notebook_new_tab()` should look at `main_status.opening_session_files`. This is the primary indication that we are in "batch open mode" if some code must behave differently. I also used that flag to open docs in a idle callback instead of synchronously (see #3267 / commit 23367de0c5)
And having added the feature back then, I can tell you I didn't plan for this :)
Alright, I am convinced it's a bug now :-)
@b4n pushed 1 commit.
0e0b33b5b8f2e858c06ea91128331f7d2252c15b fixup! Fix startup files order when placing tabs next to the current one
@b4n commented on this pull request.
/* necessary to set it to TRUE for project session support */ main_status.opening_session_files++;
- i = file_prefs.tab_order_ltr ? 0 : (session_files->len - 1); - while (TRUE) + file_prefs.tab_order_beside = FALSE; + file_prefs.tab_order_ltr = TRUE;
@kugel- OK, makes sense. Do you like it better like that (updated)?
@kugel- commented on this pull request.
/* necessary to set it to TRUE for project session support */ main_status.opening_session_files++;
- i = file_prefs.tab_order_ltr ? 0 : (session_files->len - 1); - while (TRUE) + file_prefs.tab_order_beside = FALSE; + file_prefs.tab_order_ltr = TRUE;
yes (although I had a hard time to decode the new block in my brain)
@b4n commented on this pull request.
/* necessary to set it to TRUE for project session support */ main_status.opening_session_files++;
- i = file_prefs.tab_order_ltr ? 0 : (session_files->len - 1); - while (TRUE) + file_prefs.tab_order_beside = FALSE; + file_prefs.tab_order_ltr = TRUE;
Yeah it required a bit of refactoring, because the previous code was a mess…
@elextr @eht16 @techee are you happy with this, and can you give this a new spin?
@techee commented on this pull request.
/* necessary to set it to TRUE for project session support */ main_status.opening_session_files++;
- i = file_prefs.tab_order_ltr ? 0 : (session_files->len - 1); - while (TRUE) + file_prefs.tab_order_beside = FALSE; + file_prefs.tab_order_ltr = TRUE;
Nice! It: 1. Fixes the issue of the reordered tabs 2. Also opens a file from the command-line as the last tab which is much more logical than the second tab before 3. Eliminates some ugly code
The code looks good to me as well.
@eht16 approved this pull request.
@eht16 commented on this pull request.
/* necessary to set it to TRUE for project session support */ main_status.opening_session_files++;
- i = file_prefs.tab_order_ltr ? 0 : (session_files->len - 1); - while (TRUE) + file_prefs.tab_order_beside = FALSE; + file_prefs.tab_order_ltr = TRUE;
I had only a quick look on the code but tested it again and it works fine.
@b4n pushed 1 commit.
4246298dd8b3c70528cdad0554000f5d54f39765 Fix startup files order when placing tabs next to the current one
(I just squashed the commits and rebased on master, will now merge)
Merged #3611 into master.
FFS STOP THE LAST MINUTE MERGING!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
The CI ddin't even finish here.
What bad things should happen? @b4n committed (this is without any irony!).
@elextr regarding the CI it was merely a squash (no diff from the base verified) + a rebase… and yes, I let my build and meson finish :P And it's all green now, ain't it :]
(though, I start to think it's becoming late indeed… I'll only plan to make one more PR for 2.0 GP :])
GP is less concerning, plugins can be disabled, code in Geany can't.
github-comments@lists.geany.org