(view) `b:vimtex.viewer.xwin_id` corresponds to unexpected window when multiple PDFs are open

[ejmastnak]

Description

In an admittedly unusual scenario (described below in the steps to reproduce) the variable b:vimtex.viewer.xwin_id is set to a window that is different from the window holding the PDF corresponding to the LaTeX file edited with VimTeX.

Not a big issue, but it is inconvenient when automatically closing windows using the VimtexEventQuit autocommand and CloseViewers() function workflow given as an example in :help vimtex-events---in certain cases the CloseViewers() function, which uses b:vimtex.viewer.xwin_id, will close the "wrong" PDF, i.e. not the PDF associated with the edited LaTeX file.

This workflow arises when I first open a third-party PDF (e.g. a paper, textbook, etc...), then open a LaTeX file in which I take notes about the content of the third-party PDF. Exiting VimTeX will then close the third-party PDF, instead of the PDF associated with my notes.

Files

minimal.tex

\documentclass{minimal}
\begin{document}
Hello world!
\end{document}

minimal.vim

set nocompatible
let &runtimepath  = './vimtex,' . &runtimepath
let &runtimepath .= ',./vimtex/after'
filetype plugin indent on
syntax enable

" Relevant options and VimTeX configuration below.
let g:vimtex_view_method = 'zathura'

" Close viewers when VimTeX buffers are closed (see :help vimtex-events)
function! CloseViewers()
  if executable('xdotool')
        \ && exists('b:vimtex.viewer.xwin_id')
        \ && b:vimtex.viewer.xwin_id > 0
    call system('xdotool windowclose '. b:vimtex.viewer.xwin_id)
  endif
endfunction

augroup vimtex_event_close
  au!
  au User VimtexEventQuit call CloseViewers()
augroup END

" Only for personal convenience when testing
let mapleader = " "
nmap <leader>i <plug>(vimtex-info)
nmap <leader>v <plug>(vimtex-view)
nmap <leader>c <plug>(vimtex-compile)

Possible change

(I thought I would get your opinion before diving headfirst into PRs, since you know the codebase much better and will probably have a better idea than I would.)

First, it seems the problem is only relevant to mupdf and zathura, since only those viewers have a xwin_id property.

The problematic code seems to be the following block in the s:viewer.xdo_get_id() function in autoload/vimtex/view/_template.vim:

let l:xwin_ids = vimtex#jobs#capture('xdotool search --class ' . self.name)
if len(l:xwin_ids) == 0
  call vimtex#log#warning('Viewer cannot find ' . self.name . ' window ID!')
  let self.xwin_id = 0
else
  " Taking the last ID returned by e.g. `xdotool search --class Zathura` can give unexpected results
  let self.xwin_id = l:xwin_ids[-1]
endif

If the viewer template had access to the viewer's PID in an e.g. self.pid property, my idea would be to first try...

let l:xwin_ids = vimtex#jobs#capture('xdotool search --pid ' . self.pid)

...and only then use the existing...

let l:xwin_ids = vimtex#jobs#capture('xdotool search --class ' . self.name)

...as a fallback, e.g. if self.pid is undefined. That is, first try to get window ID from viewer PID before searching by window class---I imagine searching by PID would be more robust.
It seems that the zathura.vim file does have some knowledge of the Zathura viewer's PID (e.g. the s:viewer.get_pid() function), but I am unsure of the best way to get this information from zathura.vim to _template.vim. (Perhaps you have already considered this and it is more nontrivial than I imagine.)

And of course this is admittedly an edge case/weird workflow, so if you deem it more bother than it's worth, no problem.
It's certainly not a dealbreaker to using VimTeX :)

Steps to reproduce

1. Relevant files:

   $ ls
   minimal.vim
   minimal.tex
   other.pdf       # an arbitrary non-empty PDF file
   vimtex/         # a clean copy of VimTeX for the purposes of testing this issue

2. Open other.pdf in Zathura: zathura other.pdf & (from the same shell that will then run NeoVim, hence opening in the background). Note that opening other.pdf in a separate shell from the one subsequently used to open NeoVim does not reproduce the issue.
3. Open NeoVim nvim --clean -u minimal.vim minimal.tex (I also use --clean to avoid loading my own ftplugin)
4. :VimtexCompile
5. :VimtexView (Zathura successfully opens and displays minimal.pdf)
6. :echo b:vimtex.viewer.xwin_id (or call :VimtexInfo)
7. Use e.g. xdotool selectwindow from a separate shell to confirm that the echoed window ID corresponds to the window containing other.pdf, and not to the window containing minimal.pdf
8. Exit Vim (e.g. :wq)

Expected behavior

b:vimtex.viewer.xwin_id from the previous step is the window ID of the window containing minimal.pdf.
(And minimal.pdf is closed by the CloseViewers() function in minimal.vim).

Actual behavior

b:vimtex.viewer.xwin_id is the window ID of the window containing other.pdf.
(And other.pdf is closed by the CloseViewers() function in minimal.vim).

Do you use a latexmkrc file?

No

VimtexInfo

System info:
  OS: Linux 5.16.11-arch1-1
  Vim version: NVIM v0.7.0
  Has clientserver: true
  Servername: /tmp/nvimzxz3Eo/0

VimTeX project: minimal
  base: minimal.tex
  root: /home/ej/tmp/vimtex-xdo-bug
  tex: /home/ej/tmp/vimtex-xdo-bug/minimal.tex
  main parser: current file verified
  document class: minimal
  compiler: latexmk
    engine: -pdf
    options:
      -verbose
      -file-line-error
      -synctex=1
      -interaction=nonstopmode
    callback: 1
    continuous: 1
    executable: latexmk
  viewer: Zathura
    xwin id: 44040195
    cmd_start: zathura  -x "/usr/bin/nvim --headless -c \"VimtexInverseSearch %{line} '%{i
  qf method: LaTeX logfile

[lervag]

Thanks for the detailed and good issue description!

First: I can't reproduce. It's hard to say what I'm doing wrong, but I have an idea...

First, it seems the problem is only relevant to mupdf and zathura, since only those viewers have a xwin_id property.

That's correct.

The problematic code seems to be the following block in the s:viewer.xdo_get_id() function in autoload/vimtex/view/_template.vim:

There are two functions, actually: xdo_get_id and xdo_exists. The former will get an ID just after starting the viewer, whereas the first will search for an existing window. The search is more advanced and uses the --pid option for xdotool if possible.

So, why can't I reproduce? I believe it may be an error in your test case: The xdo_get_id will search for PDFs based on the name of the PDF file. When the other file is named other.pdf and the LaTeX project is called test, then things work as expected. However, if the files happen to have the same name, then the current search may not work (if it works, we are just lucky).

If the viewer template had access to the viewer's PID in an e.g. self.pid property, my idea would be to first try...

Zathura has a self.get_pid(), but it may also be error prone. Although, it may be more accurate than the xdotool search, so perhaps you are right we should use it here as well.

Could you check if I'm right that the example is not reproducible?

[lervag]

I tried to create a more automatic example. I run the following with nvim --clean --headless -u test.vim:

set nocompatible
let &runtimepath  = '../../../,' . &runtimepath
filetype plugin indent on

let g:vimtex_view_method = 'zathura'

call system('zathura other.pdf&')
sleep 100m
let s:xwin_ids_other = join(systemlist('xdotool search --class Zathura'))

silent edit test.tex
VimtexCompile
sleep 300m
call system('pkill zathura')

echo 'Other ID: ' s:xwin_ids_other
echo 'Viewer ID:' b:vimtex.viewer.xwin_id

quitall

[ejmastnak]

Thanks for quick response, information, and automatic example!

So, why can't I reproduce? I believe it may be an error in your test case: The xdo_get_id will search for PDFs based on the name of the PDF file. When the other file is named other.pdf and the LaTeX project is called test, then things work as expected. However, if the files happen to have the same name, then the current search may not work (if it works, we are just lucky).

If I understand correctly, you are suggesting I try to reproduce the issue, but change the name of minimal.vim and minimal.tex to test.vim and test.tex, respectively (while leaving other.pdf as is)?
I tried doing so, and interestingly:

• When running your automatic test, the window IDs are assigned as expected, e.g.

  $ nvim --clean --headless -u test.vim
  VimTeX: Compiler started in continuous mode
  VimTeX: Compilation completed
  Other ID:  94371841 94371843   # confirmed with xdotool selectwindow that this corresponds to other.pdf
  Viewer ID: 96468995            # confirmed with xdotool selectwindow that this corresponds to test.pdf

• But when repeating my original steps to reproduce manually (after having changed minimal.{vim,tex} to test.{vim,tex}) I still reproduce my original issue.

[ejmastnak]

Here is a reproduction of the issue on my end, perhaps you can see if I'm missing/misinterpreting anything obvious?
The test.vim is the same as the minimal.vim when I first opened the issue.

• Open other.pdf in Zathura
• Compile test.tex, test.pdf opens
• Exit Vim/VimTeX, and other.pdf is closed by the CloseViewers function instead of test.pdf
  (Indicating the window IDs of other.pdf and test.pdf are reversed.

Edit: Apologies for quality, it looks much better if you open the GIF in dedicated browser tab/window

[lervag]

If I understand correctly, you are suggesting I try to reproduce the issue, but change the name of minimal.vim and minimal.tex to test.vim and test.tex, respectively (while leaving other.pdf as is)?

Sorry, no, not quite. The change between minimal and test is irrelevant. I'll try to explain again below.

I tried doing so, and interestingly:

• When running your automatic test, the window IDs are assigned as expected, e.g.
• But when repeating my original steps to reproduce manually (after having changed minimal.{vim,tex} to test.{vim,tex}) I still reproduce my original issue.

I may have missed something in the original steps, because I can't reproduce when I try to follow them. :\

However, I was more or less under the impression that my automated test had the same steps as what you listed.

Here is a reproduction of the issue on my end, perhaps you can see if I'm missing/misinterpreting anything obvious?

Okay, this is strange. Perhaps you could do some debugging on your end. Let's start in the get_id function:

vimtex/autoload/vimtex/view/_template.vim

Lines 132 to 149 in ab3ee41

	function! s:viewer.xdo_get_id() dict abort " {{{1
	if !self.xdo_check() | return 0 | endif
	if self.xwin_id <= 0
	" Allow some time for the viewer to start properly
	sleep 500m
	let l:xwin_ids = vimtex#jobs#capture('xdotool search --class ' . self.name)
	if len(l:xwin_ids) == 0
	call vimtex#log#warning('Viewer cannot find ' . self.name . ' window ID!')
	let self.xwin_id = 0
	else
	let self.xwin_id = l:xwin_ids[-1]
	endif
	endif
	return self.xwin_id
	endfunction

Could you echo the output of the capture? E.g., add something like this:

    let l:xwin_ids = vimtex#jobs#capture('xdotool search --class ' . self.name)
    unsilent echom string(l:xwin_ids)

On my system, this gives a list of IDs, and the last opened window is the last one in the list. I believe this is the heuristic that fails on your end.

Going further, I believe the more advanced testing that is done here may be a possible solution:

vimtex/autoload/vimtex/view/_template.vim

Lines 163 to 185 in ab3ee41

	" If xwin_id is unset, check if matching viewer windows exist
	if self.xwin_id == 0
	let l:pid = has_key(self, 'get_pid') ? self.get_pid() : 0
	if l:pid > 0
	let xwin_ids = vimtex#jobs#capture(
	\ 'xdotool search --all --pid ' . l:pid
	\ . ' --name ' . fnamemodify(self.out(), ':t'))
	let self.xwin_id = get(xwin_ids, 0)
	else
	let xwin_ids = vimtex#jobs#capture(
	\ 'xdotool search --name ' . fnamemodify(self.out(), ':t'))
	let ids_already_used = filter(map(
	\ deepcopy(vimtex#state#list_all()),
	\ {_, x -> get(get(x, 'viewer', {}), 'xwin_id')}),
	\ 'v:val > 0')
	for id in xwin_ids
	if index(ids_already_used, id) < 0
	let self.xwin_id = id
	break
	endif
	endfor
	endif
	endif

[ejmastnak]

I may have missed something in the original steps, because I can't reproduce when I try to follow them. :
However, I was more or less under the impression that my automated test had the same steps as what you listed.

Huh, it may well be that something in my original steps is unclear, although I've tried to make them as unambiguous as possible. As a sanity check I just followed them verbatim and can confirm I'm reproducing. Perhaps something else is going on on my system.

If I understand correctly, you are suggesting I try to reproduce the issue, but change the name of minimal.vim and minimal.tex to test.vim and test.tex, respectively (while leaving other.pdf as is)?

Sorry, no, not quite. The change between minimal and test is irrelevant. I'll try to explain again below.

Yeah, I figured I was interpreting incorrectly and the rename would be irrelevant, but thought I'd try just in case.

Debugging

Diff:

     let l:xwin_ids = vimtex#jobs#capture('xdotool search --class ' . self.name) 
     + unsilent echom string(l:xwin_ids)
     if len(l:xwin_ids) == 0 

Steps:

1. zathura other.pdf &
2. nvim --clean -u test.vim test.tex
3. :VimtexCompile
4. :messages

   ['31457281', '44040193', '44040195', '31457283']

5. Confirm with xdotool selectwindow that 44040195 corresponds to test.pdf and 31457283 to other.pdf. I.e. the issue persists.

On my system, this gives a list of IDs, and the last opened window is the last one in the list. I believe this is the heuristic that fails on your end.

As you expected, the last ID in the list returned by xdotool search --class Zathura does not correspond to the last-opened PDF, which is test.pdf, but instead to the previously-opened other.pdf.
Let me mess around a bit and see what I can come up with.

[ejmastnak]

Going further, I believe the more advanced testing that is done here may be a possible solution:

Correct! I just confirmed that the logic used in s:viewer.xdo_exists() solves my issue.

I tested this by replacing, in _template.vim, the class-based ID-finding code currently in s:viewer.xdo_get_id() with the PID-based logic in s:viewer.xdo_exists():

ej-t460% diff _template-old.vim _template-new.vim
139,142c139,144
<     let l:xwin_ids = vimtex#jobs#capture('xdotool search --class ' . self.name)
<     if len(l:xwin_ids) == 0
<       call vimtex#log#warning('Viewer cannot find ' . self.name . ' window ID!')
<       let self.xwin_id = 0
---
>     let l:pid = has_key(self, 'get_pid') ? self.get_pid() : 0
>     if l:pid > 0
>       let xwin_ids = vimtex#jobs#capture(
>             \   'xdotool search --all --pid ' . l:pid
>             \ . ' --name ' . fnamemodify(self.out(), ':t'))
>       let self.xwin_id = get(xwin_ids, 0)
144c146,157
<       let self.xwin_id = l:xwin_ids[-1]
---
>       let xwin_ids = vimtex#jobs#capture(
>             \ 'xdotool search --name ' . fnamemodify(self.out(), ':t'))
>       let ids_already_used = filter(map(
>             \   deepcopy(vimtex#state#list_all()),
>             \   {_, x -> get(get(x, 'viewer', {}), 'xwin_id')}),
>             \ 'v:val > 0')
>       for id in xwin_ids
>         if index(ids_already_used, id) < 0
>           let self.xwin_id = id
>           break
>         endif
>       endfor
145a159
>

Of course this probably isn't the best place to put the code long-term, just as a proof of concept. What do you think in principle of promoting PID-based window ID search to higher priority in the upstream? I notice you wrote yesterday that

Zathura has a self.get_pid(), but it may also be error prone.

Perhaps finding viewer PID opens up a whole new can of worms. Honestly, I am happy to just patch locally at this point, since it is a simple fix on my end and I'm not sure other users even have this problem.

Though I am still curious why the class-based xdotool search --class Zathura returns windows in an unexpected order on my system and not on yours (and probably not for most other users either, since no one has raised the issue).

[lervag]

Correct! I just confirmed that the logic used in s:viewer.xdo_exists() solves my issue.

Great. Then it might be prudent to combine these two methods somehow to avoid code duplication.

Of course this probably isn't the best place to put the code long-term, just as a proof of concept. What do you think in principle of promoting PID-based window ID search to higher priority in the upstream?

I think we can give it a shot. But we should avoid code duplication, so we need to think a little bit about how to do this in a good way.

Though I am still curious why the class-based xdotool search --class Zathura returns windows in an unexpected order on my system and not on yours (and probably not for most other users either, since no one has raised the issue).

It may be due to your window manager? Or terminal? I have no idea, really. But your issue is a simple proof that the method is insufficient. And it seems like there are two alternatives. The PID approach, but also xdotool search --name ....

I would think we could write something like this:

" FUNCTION GET_ID

let l:pid = has_key(self, 'get_pid') ? self.get_pid() : 0
if l:pid > 0
  let xwin_ids = vimtex#jobs#capture(
        \   'xdotool search --all --pid ' . l:pid
        \ . ' --name ' . fnamemodify(self.out(), ':t'))
  let self.xwin_id = get(xwin_ids, 0)
  return
endif

" Try search --name
let xwin_ids = vimtex#jobs#capture(
      \ 'xdotool search --name ' . fnamemodify(self.out(), ':t'))
let ids_already_used = filter(map(
      \   deepcopy(vimtex#state#list_all()),
      \   {_, x -> get(get(x, 'viewer', {}), 'xwin_id')}),
      \ 'v:val > 0')
for id in xwin_ids
  if index(ids_already_used, id) < 0
    let self.xwin_id = id
    return
  endif
endfor

" Fallback to search class
let l:xwin_ids = vimtex#jobs#capture('xdotool search --class ' . self.name)
if len(l:xwin_ids) == 0
  call vimtex#log#warning('Viewer cannot find ' . self.name . ' window ID!')
  let self.xwin_id = 0
else
  let self.xwin_id = l:xwin_ids[-1]
endif

[ejmastnak]

I think we can give it a shot. But we should avoid code duplication, so we need to think a little bit about how to do this in a good way.

Great. Yes, avoiding code duplication is important. Let me think it over for a bit.

I would think we could write something like this:

Looks good! I imagine this playing a similar role to the current xdo_get_id(). Now just how to avoid duplication with some of the existing code in xdo_exists()... I'll think about it and get back in a bit.

[ejmastnak]

Okay, so at a high level, it looks to me like xdo_get_id and xdo_exists rely on three "atomic" steps:

Searching for winID by --class

" Currently in xdo_exists
if self.xwin_id > 0
  let xwin_ids = vimtex#jobs#capture('xdotool search --class ' . self.name)
  if index(xwin_ids, self.xwin_id) < 0
    let self.xwin_id = 0
  endif
endif

" Currently in xdo_get_id
let l:xwin_ids = vimtex#jobs#capture('xdotool search --class ' . self.name)
if len(l:xwin_ids) == 0
  call vimtex#log#warning('Viewer cannot find ' . self.name . ' window ID!')
  let self.xwin_id = 0
else
  let self.xwin_id = l:xwin_ids[-1]
endif

Searching for winID by --pid and --name

" Currently in xdo_exists
let l:pid = has_key(self, 'get_pid') ? self.get_pid() : 0
if l:pid > 0
  let xwin_ids = vimtex#jobs#capture(
        \   'xdotool search --all --pid ' . l:pid
        \ . ' --name ' . fnamemodify(self.out(), ':t'))
  let self.xwin_id = get(xwin_ids, 0)
else " ...fall back to search by `--name` only

" Proposed in updated xdo_get_id
let l:pid = has_key(self, 'get_pid') ? self.get_pid() : 0
if l:pid > 0
  let xwin_ids = vimtex#jobs#capture(
        \   'xdotool search --all --pid ' . l:pid
        \ . ' --name ' . fnamemodify(self.out(), ':t'))
  let self.xwin_id = get(xwin_ids, 0)
  return
endif

Searching for winID by --name only

" Currently in xdo_exists
let xwin_ids = vimtex#jobs#capture(
      \ 'xdotool search --name ' . fnamemodify(self.out(), ':t'))
let ids_already_used = filter(map(
      \   deepcopy(vimtex#state#list_all()),
      \   {_, x -> get(get(x, 'viewer', {}), 'xwin_id')}),
      \ 'v:val > 0')
for id in xwin_ids
  if index(ids_already_used, id) < 0
    let self.xwin_id = id
    break
  endif
endfor

" Proposed in updated xdo_get_id
let xwin_ids = vimtex#jobs#capture(
      \ 'xdotool search --name ' . fnamemodify(self.out(), ':t'))
let ids_already_used = filter(map(
      \   deepcopy(vimtex#state#list_all()),
      \   {_, x -> get(get(x, 'viewer', {}), 'xwin_id')}),
      \ 'v:val > 0')
for id in xwin_ids
  if index(ids_already_used, id) < 0
    let self.xwin_id = id
    return
  endif
endfor

What do you think of outsourcing these three tasks to atomic script-local functions in _template, e.g.

• s:viewer.find_win_id_by_class
• s:viewer.find_win_id_by_pid
• s:viewer.find_win_id_by_name

(or similar), and then calling these functions from xdo_get_id and xdo_exists?
I think that would avoid code duplication.

If each of these three functions returns 0 as an error case (if now winID is found) and a winID > 0 otherwise, I think the functions would be compatible with the existing logic in _template.vim.
I would be happy to give this a try and submit a PR.

[lervag]

Okay, so at a high level, it looks to me like xdo_get_id and xdo_exists rely on three "atomic" steps: ...

Yes, I think you are right.

What do you think of outsourcing these three tasks to atomic script-local functions in _template, e.g.

• s:viewer.find_win_id_by_class
• s:viewer.find_win_id_by_pid
• s:viewer.find_win_id_by_name

(or similar), and then calling these functions from xdo_get_id and xdo_exists? I think that would avoid code duplication.

Yes, I think that's a perfect first step. However, I think xdo_exists might not need the find_win_id_by_class, because it needs a stricter search. Also, I'd like to keep the xdo prefix for these functions to clearly indicate that they are only relevant for the xdotool flavor of viewers.

I would be happy to give this a try and submit a PR.

Great, I would be very happy to review and merge a PR! I'll try to not be too slow in my responses :)

[ejmastnak]

Yes, I think that's a perfect first step. However, I think xdo_exists might not need the find_win_id_by_class, because it needs a stricter search.

Great, and sounds good.

Also, I'd like to keep the xdo prefix for these functions to clearly indicate that they are only relevant for the xdotool flavor of viewers.

Yes, good idea. That's better for sure.

I would be happy to give this a try and submit a PR.

Great, I would be very happy to review and merge a PR!

Excellent, I'll give it a try tomorrow.

I'll try to not be too slow in my responses :)

I don't think you have anything to worry about here---I've yet to find a OSS developer who responds to issues and PRs faster than you do!