fix(windows): set console icon later in startup #20634

justinmk · 2022-10-13T13:13:22Z

Problem:
Windows console icon is set early in startup, but there are some cases where os_exit is called and we don't restore the original icon.

Solution:

Move os_icon_init() later in the startup sequence, and only if use_builtin_ui==true.
Rename functions: use os_ prefix for platform-specific code.

justinmk · 2022-10-13T13:15:01Z

src/nvim/main.c

@@ -600,6 +588,12 @@ int main(int argc, char **argv)
    TIME_MSG("UIEnter autocommands");
  }

+#ifdef MSWIN
+  if (use_builtin_ui) {
+    os_icon_init();


Is there any reason to set the icon if we are not using the TUI (use_builtin_ui) ?

@justinmk With use_builtin_ui, the icon isn't being set, as of v0.9.0-dev-823+g41aa5ce3eb49. I haven't bisected.

It's probably because of the PR where TUI was split from the main nvim process. We could remove this if check or OR use_remote_ui with use_builtin_ui. I don't know which is better.

er... use_builtin_ui should be true, why isn't it?

E:\projects\neovim\master> gdb .\build\debug\bin\nvim.exe Reading symbols from .\build\debug\bin\nvim.exe... gdb> b main.c:303 Breakpoint 1 at 0x140001b27: file E:/projects/neovim/master/src/nvim/main.c, line 303. gdb> r Starting program: E:\projects\neovim\master\build\debug\bin\nvim.exe [New Thread 1840.0x6a8] [New Thread 1840.0x12c8] [New Thread 1840.0x1214] [New Thread 1840.0x2070] [New Thread 1840.0x17c8] [New Thread 1840.0x275c] Thread 1 hit Breakpoint 1, wmain (argc=0x1, argv_w=0x11a4b3daa10) at E:/projects/neovim/master/src/nvim/main.c:303 303 bool use_builtin_ui = (has_term && !headless_mode && !embedded_mode && !silent_mode); gdb> n 308 server_init(params.listen_addr); gdb> p use_builtin_ui $1 = 0x1

Note that the value of use_builtin_ui is true. But I noticed that this nvim process, the one gdb is attached to, runs into this line, and after executing it, the nvim window appears. At that point, I don't have gdb's ui anymore.

neovim/src/nvim/main.c

Lines 380 to 382 in 2af31fc

if (ui_client_channel_id) {

ui_client_run(remote_ui); // NORETURN

}

^This line is above/before the line where Windows taskbar+console icon is set.

neovim/src/nvim/main.c

Lines 591 to 596 in 2af31fc

#ifdef MSWIN

if (use_builtin_ui) {

os_icon_init();

}

os_title_save();

#endif

ok. maybe we need to check ui_client_channel_id then

Nope. Doesn't satisfy if(ui_client_channel_id) either.

I think it should either be if(use_builtin_ui || use_remote_ui) or just if(use_remote_ui). But I'm struggling with understanding how the main and embedded processes are being run. Can't tell what the conditional should be.

I think it should either be if(use_builtin_ui || use_remote_ui)

👍 that seems fine.

src/nvim/main.c

src/nvim/os/os_win_console.c

3N4N · 2022-10-13T16:53:36Z

Nvim is crashing. I'll try and find out the problem.

3N4N · 2022-10-13T18:10:38Z

I don't know how to make sense of this:

os_getenv (name=0x7ff606d8d22c <key+257> "VIMRUNTIME") at E:/projects/neovim/feat-winicon/src/nvim/os/env.c:61
61        size_t size = 64;
gdb> fin
Run till exit from #0  os_getenv (name=0x7ff606d8d22c <key+257> "VIMRUNTIME") at E:/projects/neovim/feat-winicon/src/nvim/os/env.c:61
0x00007ff606ab2b57 in os_icon_init () at E:/projects/neovim/feat-winicon/src/nvim/os/os_win_console.c:82
82        const char *vimruntime = os_getenv("VIMRUNTIME");
Value returned is $1 = 0x19da910c870 "C:/apps/nvim/share/nvim\\runtime"
                       ^^^^^^^^^^^^^
gdb> x/s 0x19da910c870
0x19da910c870:  "C:/apps/nvim/share/nvim\\runtime"
gdb> p vimruntime
$2 = 0x19da7ae97c0 "\002"
     ^^^^^^^^^^^^^
gdb>

That's where nvim is crashing: in snprintf call, because vimruntime is pointing to a wrong place in the memory.

justinmk · 2022-10-13T23:50:07Z

Nvim is crashing. I'll try and find out the problem.

does moving os_icon_init(); earlier in startup fix it?

3N4N · 2022-10-14T02:24:42Z

does moving os_icon_init(); earlier in startup fix it?

No. And same gdb log.

erw7 · 2022-10-14T10:56:10Z

https://github.com/neovim/neovim/actions/runs/3246436285/jobs/5325170873#step:5:765

2022-10-13T23:51:00.0037291Z [315/407] Building C object src\nvim\CMakeFiles\nvim.dir\os\os_win_console.c.obj
2022-10-13T23:51:00.0037737Z D:\a\neovim\neovim\src\nvim\os\os_win_console.c(82): warning C4013: 'os_getenv' undefined; assuming extern returning int
2022-10-13T23:51:00.0038238Z D:\a\neovim\neovim\src\nvim\os\os_win_console.c(85): warning C4013: 'os_path_exists' undefined; assuming extern returning int
2022-10-13T23:51:00.0038762Z D:\a\neovim\neovim\src\nvim\os\os_win_console.c(82): warning C4047: 'initializing': 'const char *' differs in levels of indirection from 'int'

erw7 · 2022-10-14T11:05:19Z

Is this feature absolutely necessary? GetConsoleWindow is deprecated on the following page. There are plans for Windows Terminal to become the default command line application in Windows 11 in the future. This feature does not work on Windows terminals, so there is little point in implementing this now.

Ref. https://learn.microsoft.com/en-us/windows/console/getconsolewindow#remarks

src/nvim/os/os_win_console.c

Problem: Windows console icon is set early in startup, but there are some cases where `os_exit` is called and we don't restore the original icon. Solution: - Move `os_icon_init()` later in the startup sequence, and only if `use_builtin_ui==true`. - Rename functions: use `os_` prefix for platform-specific code.

3N4N · 2022-10-14T15:13:44Z

Is this feature absolutely necessary? GetConsoleWindow is deprecated on the following page. There are plans for Windows Terminal to become the default command line application in Windows 11 in the future. This feature does not work on Windows terminals, so there is little point in implementing this now.

I agree that this feature is pointless, but not because Windows is going to make Windows Terminal the default console. The old console is always gonna be there, because of the same reason these old API are gonna be there: tons of programs use them. And many people, including me, use the old console for quick spawn-task-destroy workflow. Windows Terminal is very slow to start compared to the old console. Opening text files from file explorer by double clicking -- for this kind of task, the old console is preferable.

But I do agree this is pointless, because who cares what the icon of a window is.

justinmk · 2022-10-14T15:19:26Z

This feature does not work on Windows terminals, so there is little point in implementing this now.

Ah, I thought this was for Windows terminal too. Is there a similar API for Windows terminal?

This is a small, isolated, low-risk amount of code so I think it's ok to keep it around for now. We can always remove it later.

3N4N · 2022-10-14T15:30:15Z

Ah, I thought this was for Windows terminal too. Is there a similar API for Windows terminal?

I looked yesterday. Couldn't find any.

Windows Terminal also doesn't respect :h titlestring, at least in my version (maybe there is a WT setting I'm unaware of). I was researching the new APIs for that.

justinmk · 2022-10-14T16:09:20Z

@3N4N would you mind testing this once more?

3N4N · 2022-10-14T16:53:48Z

Yes, it works as expected.

erw7 · 2022-10-15T09:10:07Z

Windows Terminal also doesn't respect :h titlestring, at least in my version (maybe there is a WT setting I'm unaware of). I was researching the new APIs for that.

No, Console Virtual Terminal Sequences (conpty) and Windows Terminal supports Window Title changes. The reason 'titlestring' does not work in neovim is because the terminfo in vtpcon does not have tsl,fsl (I think that feature had not yet been implemented at the time windows.ti was written). If you set TERM to xterm-256color, 'titlestring' will work.

https://learn.microsoft.com/en-us/windows/console/console-virtual-terminal-sequences#window-title

I think vtpcon has lost its usefulness because of the implementation of many features of xterm in conpty. In addition, Windows 8.1 will be EOL on 1/10/2023. Therefore, windows.ti and all changes related to it should be removed in 0.9.

I was researching the new APIs for that.

@3N4N The Windows API should not be used unless necessary. If terminal functions and libuv can absorb OS differences, they should be used first. If you are interested in contributing to the improvement of Windows tui, please refer to the following document.

Ref. https://invisible-island.net/xterm/ctlseqs/ctlseqs.txt
Ref. https://invisible-island.net/ncurses/man/terminfo.5.html
Ref. https://learn.microsoft.com/en-us/windows/console/console-virtual-terminal-sequences

justinmk · 2022-10-15T10:46:33Z

Windows CI failure on master. Same failure observed in master CI as far back as d9a80b8 (well before #20607 was merged), so unrelated.

2022-10-15T10:32:00.6539774Z stack traceback:
2022-10-15T10:32:00.6540589Z 	test/functional\core\job_spec.lua:692: in function <test/functional\core\job_spec.lua:665>
2022-10-15T10:32:00.6541174Z 
2022-10-15T10:32:00.6576727Z nan ms test\functional\helpers.lua:109: EOF was received from Nvim. Likely the Nvim process crashed.
2022-10-15T10:32:00.6577335Z 
2022-10-15T10:32:00.6578315Z stack traceback:
2022-10-15T10:32:00.6578950Z 	test\functional\helpers.lua:109: in function 'nvim'
2022-10-15T10:32:00.6579635Z 	test/functional\core\job_spec.lua:33: in function <test/functional\core\job_spec.lua:30>
2022-10-15T10:32:00.6580117Z 
2022-10-15T10:32:00.6668242Z nan ms test\functional\helpers.lua:109: EOF was received from Nvim. Likely the Nvim process crashed.
2022-10-15T10:32:00.6668919Z 
2022-10-15T10:32:00.6669321Z stack traceback:
2022-10-15T10:32:00.6669941Z 	test\functional\helpers.lua:109: in function 'nvim'
2022-10-15T10:32:00.6670677Z 	test/functional\core\job_spec.lua:33: in function <test/functional\core\job_spec.lua:30>
2022-10-15T10:32:00.6671188Z 
2022-10-15T10:32:00.6770940Z nan ms test\functional\helpers.lua:109: EOF was received from Nvim. Likely the Nvim process crashed.
2022-10-15T10:32:00.6771680Z 
2022-10-15T10:32:00.6772063Z stack traceback:
2022-10-15T10:32:00.6772649Z 	test\functional\helpers.lua:109: in function 'nvim'
2022-10-15T10:32:00.6773360Z 	test/functional\core\job_spec.lua:33: in function <test/functional\core\job_spec.lua:30>
2022-10-15T10:32:00.6773850Z 
2022-10-15T10:32:00.6863958Z nan ms test\functional\helpers.lua:109: EOF was received from Nvim. Likely the Nvim process crashed.

3N4N · 2022-10-16T09:03:38Z

Setting tab icon through VT sequence in Windows Terminal is being tracked in this issue: microsoft/terminal#1868

And thanks erw7 for the reading materials.

Problem: After TUI refactor commit, code for setting Windows taskbar icon wasn't being executed because of a backdated conditional. Solution: Update the conditional to take TUI refactor into account. Ref: #20634 (comment)

Problem: After TUI refactor commit, code for setting Windows taskbar icon wasn't being executed because of a backdated conditional. Solution: Update the conditional to take TUI refactor into account. Ref: neovim#20634 (comment)

justinmk mentioned this pull request Oct 13, 2022

feat(Windows): show icon in titlebar and taskbar #20607

Merged

justinmk commented Oct 13, 2022

View reviewed changes

src/nvim/main.c Outdated Show resolved Hide resolved

3N4N reviewed Oct 13, 2022

View reviewed changes

src/nvim/os/os_win_console.c Outdated Show resolved Hide resolved

3N4N reviewed Oct 14, 2022

View reviewed changes

src/nvim/os/os_win_console.c Show resolved Hide resolved

refactor(windows): move os_icon_xx functions

1ca3a37

justinmk force-pushed the icon branch from 349f380 to 1ca3a37 Compare October 14, 2022 15:17

justinmk mentioned this pull request Oct 15, 2022

TUI: remove vtpcon (Windows 8) #20666

Open

justinmk merged commit d52e3b9 into neovim:master Oct 15, 2022

justinmk deleted the icon branch October 15, 2022 10:22

3N4N mentioned this pull request Feb 15, 2023

fix: set taskbar and window icon in windows #22270

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(windows): set console icon later in startup #20634

fix(windows): set console icon later in startup #20634

justinmk commented Oct 13, 2022

justinmk Oct 13, 2022

3N4N Jan 27, 2023

3N4N Jan 27, 2023

justinmk Feb 9, 2023

3N4N Feb 9, 2023 •

edited

Loading

justinmk Feb 9, 2023

3N4N Feb 10, 2023

3N4N Feb 10, 2023 •

edited

Loading

justinmk Feb 15, 2023

3N4N commented Oct 13, 2022 •

edited

Loading

3N4N commented Oct 13, 2022

justinmk commented Oct 13, 2022

3N4N commented Oct 14, 2022

erw7 commented Oct 14, 2022

erw7 commented Oct 14, 2022

3N4N commented Oct 14, 2022 •

edited

Loading

justinmk commented Oct 14, 2022

3N4N commented Oct 14, 2022

justinmk commented Oct 14, 2022

3N4N commented Oct 14, 2022

erw7 commented Oct 15, 2022 •

edited

Loading

justinmk commented Oct 15, 2022 •

edited

Loading

3N4N commented Oct 16, 2022

	if (ui_client_channel_id) {
	ui_client_run(remote_ui); // NORETURN
	}

	#ifdef MSWIN
	if (use_builtin_ui) {
	os_icon_init();
	}
	os_title_save();
	#endif

fix(windows): set console icon later in startup #20634

fix(windows): set console icon later in startup #20634

Conversation

justinmk commented Oct 13, 2022

justinmk Oct 13, 2022

Choose a reason for hiding this comment

3N4N Jan 27, 2023

Choose a reason for hiding this comment

3N4N Jan 27, 2023

Choose a reason for hiding this comment

justinmk Feb 9, 2023

Choose a reason for hiding this comment

3N4N Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

justinmk Feb 9, 2023

Choose a reason for hiding this comment

3N4N Feb 10, 2023

Choose a reason for hiding this comment

3N4N Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

justinmk Feb 15, 2023

Choose a reason for hiding this comment

3N4N commented Oct 13, 2022 • edited Loading

3N4N commented Oct 13, 2022

justinmk commented Oct 13, 2022

3N4N commented Oct 14, 2022

erw7 commented Oct 14, 2022

erw7 commented Oct 14, 2022

3N4N commented Oct 14, 2022 • edited Loading

justinmk commented Oct 14, 2022

3N4N commented Oct 14, 2022

justinmk commented Oct 14, 2022

3N4N commented Oct 14, 2022

erw7 commented Oct 15, 2022 • edited Loading

justinmk commented Oct 15, 2022 • edited Loading

3N4N commented Oct 16, 2022

3N4N Feb 9, 2023 •

edited

Loading

3N4N Feb 10, 2023 •

edited

Loading

3N4N commented Oct 13, 2022 •

edited

Loading

3N4N commented Oct 14, 2022 •

edited

Loading

erw7 commented Oct 15, 2022 •

edited

Loading

justinmk commented Oct 15, 2022 •

edited

Loading