refactor skim viewer

[clason]

Refactor the Skim viewer backend to

• use JavaScript instead of AppleScript (nicer, and should fix Skim forward search not working on macOS 12.1 #2279)
• make the initial start respect g:vimtex_view_skim_activate (alternative to add option to control whether Skim gets focus upon first open #2286, @yongrenjie )
• factor out common script parts into script function
• correct off-by-one line number for forward search
• introduce new option g:vimtex_view_skim_sync to perform forward search after compilation (syncing viewer and editor), default false
• change default for g:vimtex_view_skim_reading_bar to false (location is highlighted anyway on sync)

[lervag]

• use JavaScript instead of AppleScript (nicer, and should fix Skim forward search not working on macOS 12.1 #2279)

Great!

• make the initial start respect g:vimtex_view_skim_activate (alternative to add option to control whether Skim gets focus upon first open #2286, @yongrenjie )

Great!

• factor out common script parts into self variables

Not quite sure if this has any specific merits..?

• correct off-by-one line number for forward search

Great!

• introduce new option g:vimtex_view_skim_sync to perform forward search after compilation (syncing viewer and editor), default true

Does this sync on every callback, or just the initial one? I think there are two different scenarios here:

• Forward search on viewer launch (the viewer may be started manually with \lv or implicitly by callback). I believe this should have an option, perhaps g:vimtex_view_skim_sync_on_start.
• Forward search on callbacks, i.e. after every successful compile. I don't quite see why anyone would want this one as it seems interruptive. Further, this should be not so hard to implement as user customization with an autocommand.

• change default for g:vimtex_view_skim_reading_bar to false (location is highlighted anyway on sync)

I don't have any opinions on this.

[lervag]

Do I remember correctly that this stuff is required to force Skim to update after successful compilation? I.e., without this, Skim would still show the old version of the PDF?

If possibly, I think it would be good to add support for g:vimtex_view_automatic here similar to how it's done for Zathura and MuPDF, see here:

vimtex/autoload/vimtex/view/_template.vim

Lines 236 to 253 in f31b723

	function! s:viewer.xdo_start_from_compiler_callback(outfile) dict abort " {{{1
	if !(self.xdo_check()
	\ && g:vimtex_view_automatic
	\ && g:vimtex_view_automatic_xwin
	\ && !has_key(self, 'started_through_callback')) | return | endif
	" Search for existing window created by latexmk
	" Note: It may be necessary to wait some time before it is opened and
	" recognized. Sometimes it is very quick, other times it may take
	" a second. This way, we don't block longer than necessary.
	for l:dummy in range(30)
	if self.xdo_exists() | return | endif
	sleep 50m
	endfor
	call self._start(a:outfile)
	let self.started_through_callback = 1
	endfunction

[lervag]

Is it possible to implement an _exists method that would be reliable for Skim?

[clason]

No, that is for doing a forward search (syncing editor and viewer, which I know some people like). The update happens in every case (via the app.revert(theDocs) line) as I don't see any reason not to have this.

To be clear: without the sync part, the PDF would still get updated but would keep showing the same view.

[clason]

I can, however, put this behind a g:vimtex_view_automatic guard -- if there is a good reason for it?

[lervag]

I think the main purpose for adding these variables as dictionary keys is to prevent duplicate code. As far as I can see, the start key is only used once. show and sync are used twice. I therefore believe it would be better to remove the start key and instead type it out verbosely (unless we end up removing the _latexmk_append_argument stuff altogether). I'm also wondering if the code could be slightly cleaner if the show and sync keys are instead changed to script variables; it would never make sense for a user outside of this scripts scope to inspect these variables, right?

[clason]

I think the main question is when the config variables are evaluated. I believe the reason to have them in the template is to allow updating it via :VimtexReload?

But this relates to the comment below: I'm happy to do the refactoring in another way; I just wanted to stay as close to the current setup as I don't fully understand the control flow here.

[clason]

Not quite sure if this has any specific merits..?

Makes it a bit easier to maintain (it's hard enough not to lose track of all the quotes for the embedded scripting...). An alternative would be to move the complete thing out into a (private) view function that takes options to control the behavior, depending on whether it's run from a callback or a forward search.

Does this sync on every callback, or just the initial one? I think there are two different scenarios here:

It syncs on every callback. I agree that this may not be desirable (in fact, my motivation for the original refactor to get rid of displayline was exactly not to do that ;)), but there's a tradeoff here between features and complexity; this strikes me as a reasonable one, at least as long as VimTeX doesn't easily provide the information whether it's the first callback or subsequent ones?

[clason]

I think there are three scenarios:

1. a forward search, which I believe should sync unconditionally (after all, that's the meaning of "forward search");
2. a compiler callback, which some people would like to keep synced while others would hate that, hence should be an option;
3. a compiler callback that also starts the viewer -- optionally followed by a forward search, which I believe most people would want.

As far as I can tell, the current architecture doesn't allow me to easily handle 3., since VimTeX itself doesn't know about this, and the latexmk option can't run vimscript functions?

[clason]

@lervag I've refactored again to extract all the common JavaScript lines into a single make_cmd_view script-level function; that indeed looks cleaner (I've also split up the join to avoid the nested ternary).
In principle, the join could be avoided since vimscript allows line continuation inside strings, but that breaks syntax highlighting, so I'm not sure you'd like that?

The sync now defaults to false to preserve old behavior; I think this is now a strict improvements. Further improvements are possible but would need some architecture changes, I believe.

[lervag]

Thanks @clason! I've merged now. But please note: I've made some important general changes based on the discussion we've had. I've now forced -view=none with the latexmk backend so as to have full control of the viewer from vimtex. I expect this to fail somewhat, actually, but I believe it is better to address those issues and find a more robust solution than to stay intertwined with latexmk.

So: Please test the latest version and let me know how it works. Especially: with the default settings with Skim, do things still work as expected?

[clason]

Yes, looks fine to me! Except for the reading bar, I get the same behavior as before with the default settings (and g:vimtex_view_skim_sync works as expected).

[lervag]

Does Skim start automatically after compilation?

Also, I think we want to support g:vimtex_view_automatic also for Skim.

[clason]

Does Skim start automatically after compilation?

Yes, it does.

Also, I think we want to support g:vimtex_view_automatic also for Skim.

Why (honestly asking)? From what I've seen, this is optional since for many viewers, it requires workarounds using external tools and whatnot, which may have undesired side effects (such as slowdown). However, Skim supports this out of the box with the same interface, so it's a freebie.

Can you think of any situation where you would not want this?

[lervag]

Ok, so, to be more clear about what I'm thinking about, please see how the general viewer now works wrt. callbacks:

vimtex/autoload/vimtex/view/_template.vim

Lines 80 to 91 in 72e6285

	function! s:viewer.compiler_callback(outfile) dict abort " {{{1
	if !g:vimtex_view_automatic
	\ || has_key(self, 'started_through_callback') | return | endif
	call self._start(a:outfile)
	let self.started_through_callback = 1
	endfunction
	" }}}1
	function! s:viewer.compiler_stopped() dict abort " {{{1
	unlet! self.started_through_callback
	endfunction

The callbacks are "activated" here (they are active in all cases):

vimtex/autoload/vimtex/view.vim

Lines 25 to 29 in 72e6285

	augroup vimtex_viewer
	autocmd!
	autocmd User VimtexEventCompileSuccess call vimtex#view#compiler_callback()
	autocmd User VimtexEventCompileStopped call vimtex#view#compiler_stopped()
	augroup END

And this shows how they are abstracted:

vimtex/autoload/vimtex/view.vim

Lines 50 to 66 in 72e6285

	function! vimtex#view#compiler_callback() abort " {{{1
	if exists('*b:vimtex.viewer.compiler_callback')
	if !b:vimtex.viewer.check() | return | endif
	let l:outfile = b:vimtex.viewer.out()
	if !filereadable(l:outfile) | return | endif
	call b:vimtex.viewer.compiler_callback(l:outfile)
	endif
	endfunction
	" }}}1
	function! vimtex#view#compiler_stopped() abort " {{{1
	if exists('*b:vimtex.viewer.compiler_stopped')
	call b:vimtex.viewer.compiler_stopped()
	endif
	endfunction

[lervag]

Why (honestly asking)? From what I've seen, this is optional since for many viewers, it requires workarounds using external tools and whatnot, which may have undesired side effects (such as slowdown). However, Skim supports this out of the box with the same interface, so it's a freebie.

Can you think of any situation where you would not want this?

Yes, simple: I don't want \ll to open the viewer automatically. So, I add let g:vimtex_view_automatic = 0 to prevent it. That just works. I use \lv to open the viewer when I want it.

Currently, this workflow is not possibly with Skim, because it does not support g:vimtex_view_automatic.

[clason]

Interesting. But I don't see the issue then: Isn't it trivial to add that approach to the current callback, as in

function! s:viewer.compiler_callback(outfile) dict abort " {{{1 
   if !g:vimtex_view_automatic 
       \ || has_key(self, 'started_through_callback') | return | endif 
  
   call vimtex#jobs#run(s:make_cmd_view(a:outfile, g:vimtex_view_skim_sync))
   let self.started_through_callback = 1 
 endfunction 

[lervag]

For other people reading here - me and @clason discussed things on Gitter and I've pushed a few more updates that we believe resolve these things for now. Feel free to open new issues if something broke!