Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add option to control whether Skim gets focus upon first open #2286

Closed
wants to merge 3 commits into from
Closed

add option to control whether Skim gets focus upon first open #2286

wants to merge 3 commits into from

Conversation

yongrenjie
Copy link
Contributor

Just a tiny PR which introduces an option g:vimtex_view_skim_activate_on_start, to control whether Skim is brought to the foreground when it's first opened (using \ll or \lv). The default value is kept to 1 to match the current behaviour, i.e. bring to foreground. Seems to work nicely on my Mac (Monterey 12.0 here).

(Somewhat related to #180; but rather less general, because it's only Skim and hence only macOS.)

@lervag
Copy link
Owner

lervag commented Dec 27, 2021

Thanks. However, I think this could be solved in a better way. Right now, the startskim attribute seems out of place, because it is only used as the string passed to latexmk to specify the viewer opened by latexmk. I.e., this setting is only used when the viewer is opened implicitly by latexmk after we start compilation with \ll. So, your proposed setting would not have any effect on the \lv mapping.

Instead, perhaps we could instead make these changes:

  • Remove the startskim key from the s:viewer dictionary.
  • Add option g:vimtex_view_skim_latexmk_option which by default would be open -a Skim.

This would allow the configuration possibility you want in a way that seems to
me to be more clear. What do you think?

@clason - comments?

@clason
Copy link
Contributor

clason commented Dec 27, 2021

I don't quite understand what the difference is to the already existing g:vimtex_view_skim_activate option? I also think it would be nice to simplify the control flow here and maybe consolidate the different functions.

@yongrenjie
Copy link
Contributor Author

I.e., this setting is only used when the viewer is opened implicitly by latexmk

Ah, I didn't look into the code very thoroughly, which is probably the source of my misunderstanding — but now I see what you mean. I agree with your suggestion @lervag — it is more technically correct and that's always a good thing! I can rework this a bit later.

I don't quite understand what the difference is to the already existing g:vimtex_view_skim_activate option

@clason As I understand it (and in my experience), that option only applies to forward search, not the initial compilation (with latexmk).

Happy to hear any other suggestions!

@clason
Copy link
Contributor

clason commented Dec 27, 2021

I mean, why not use the _start (or callback) method for the initial compilation as well? Is there an actual need to have a third, potentially different, method? I think most people would actually prefer the first startup to behave as a forward search?

(Which reminds me that I've been meaning to try to simplify the AppleScript code -- the most recent displayline script looks quite a bit more compact and factors out some common lines.)

@yongrenjie
Copy link
Contributor Author

yongrenjie commented Dec 27, 2021

That I'm not entirely sure about; after a bit of experimentation, it appears that the latexmk previewer option might not be needed, because the compiler callback opens Skim for the initial compilation anyway. With my PR, one would be able to control the initial compilation & subsequent compilations independently. (Indeed, the current default behaviour is already different, because Skim is focused initially and not subsequently.) But if that's not worth having, then it seems that it could be simplified.

Not sure where _start comes in, yet. It seems to me that that's triggered on forward searching, not compilation?

@clason
Copy link
Contributor

clason commented Dec 27, 2021

That I'm not entirely sure about; after a bit of experimentation, it appears that the latexmk previewer option might not be needed, because the compiler callback opens Skim for the initial compilation anyway.

If that is the case, I'd vote for removing that (unused?) option completely to prevent confusion.

With my PR, one would be able to control the initial compilation & subsequent compilations independently.

Yes, the question is exactly whether that is desired. (Not for me, but other people may have different opinions, and those I'd like to hear.)

(Indeed, the current default behaviour is already different, because Skim is focused initially and not subsequently.)

Yes, that is very much by design -- imagine if your editor lost focus every time you saved ;)

Not sure where _start comes in, yet. It seems to me that that's triggered on forward searching, not compilation?

Yes, that is correct. My point was that "first open" and "explicit forward search" probably share much of the same desired behavior?

@yongrenjie
Copy link
Contributor Author

Ok, good, I'm on the same page! 😄

Anyway, I renamed the option for the time being according to @lervag's comment. Feel free to ignore if you all want to move in a different direction.

@clason
Copy link
Contributor

clason commented Dec 28, 2021

If I understand correctly, the motivation for this change is not to have Skim steal focus when first opening after a compilation?

If so, my proposal would be to make a different change (not in this PR necessarily):

  1. remove the latexmk_previewer variable and invocation completely and just rely on the compiler callback;
  2. add a sync variable to control whether the callback includes a forward search.

@lervag what is the role of the s:viewer._latexmk_append_argument() function? Can it simply be removed (I suspect that it is left over from a time before the current callback architecture)?

EDIT: I could combine this with the switch to JavaScript, see #2279 (comment)

@clason clason mentioned this pull request Dec 29, 2021
@lervag
Copy link
Owner

lervag commented Dec 30, 2021

I don't quite understand what the difference is to the already existing g:vimtex_view_skim_activate option

As I understand it (and in my experience), that option only applies to forward search, not the initial compilation (with latexmk).

what is the role of the s:viewer._latexmk_append_argument() function? Can it simply be removed (I suspect that it is left over from a time before the current callback architecture)?

Yes - the point is that latexmk has its own feature for opening the viewer, and that VimTeX has traditionally "played nice" with this feature by passing on the necessary option to make latexmk open the correct viewer in the correct way. This would typically imply that things are more or less seamless.

However, as is apparent e.g. from this issue, the result is not fully seamless, as we have things like g:vimtex_view_skim_activate that is not relevant if Skim is opened implicitly by latexmk.

If I understand correctly, the motivation for this change is not to have Skim steal focus when first opening after a compilation?

If so, my proposal would be to make a different change (not in this PR necessarily):

1. remove the `latexmk_previewer` variable and invocation completely and just rely on the compiler callback;

2. add a `sync` variable to control whether the callback includes a forward search.

EDIT: I could combine this with the switch to JavaScript, see #2279 (comment)

It's not an unreasonable proposal. But let's consider it properly. So, the idea is to generally prevent latexmk from opening the viewer and instead open the viewer from VimTeX through callbacks. The benefits:

  • It is more or less already implemented and making this change should be easy.
  • VimTeX can have full control of the viewer, and we don't need extra code/options to "patch" things to make them seamless.

The cons:

  • I believe there are no real cons here. But there may be an issue with requirements that I'm not thinking of right now.

I don't mind having all of this implemented in #2289. So perhaps we should continue the discussion there?

@yongrenjie
Copy link
Contributor Author

OK, I checked out #2289 and it "works for me" from the perspective of a user. When I made this PR I had in mind to not remove any configurability / break somebody's workflow, but personally, I totally don't care about having separate behaviour for compilation & forward search. Essentially, I never want Skim to be focused :-)

I'll close this now and look forward to that being merged into master. Thanks for picking it up, @clason.

@yongrenjie yongrenjie closed this Dec 30, 2021
lervag pushed a commit that referenced this pull request Dec 31, 2021
Refactor the Skim viewer backend to
* use JavaScript instead of AppleScript (nicer, and should fix #2279)
* make the initial start respect `g:vimtex_view_skim_activate`
  (alternative to #2286)
* 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 added a commit that referenced this pull request Dec 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants