search

James-Yu / LaTeX-Workshop

Boost LaTeX typesetting efficiency with preview, compile, autocomplete, colorize, and more.
MIT License
10.42k stars 519 forks source link

Better viewer trimming #4292

Closed James-Yu closed 2 weeks ago

James-Yu commented 3 weeks ago

This PR provides a new implementation to replace the original PDF viewer trimming logic.

The old one heavily relies on Javascript computation on the size/margin of viewer, and the implementation is quite inter-twined.

This new implementation instead is CSS-based. We create a new css-var called --trim-factor, which will be multiplexed onto the --scale-factor of PDF.js for high-resolution rendering of PDF.

Further, the viewer relies on --trim-factor to calculate the width/height of each page, then set a negative margin to the top and left to centralize each page, effectively trimming the margins.

This PR also moves the trimming selector from main toolbar to the dropdown menu: this decision is based on the small size of empty space in the main toolbar. The previous place often caused layout glitches.

Further, the trimming selector is improved to be a numerical input, such that users have a more precise control over the trimming percentage.

Further +1, the structure of latexworkshop.ts is changed by extracting features in other component files. Clean up a bit.

James-Yu commented 3 weeks ago

The someday in #3754 has come. @bwegge hello! I'm not sure if you are still using this extension. If so, may I invite you to have a test on this PR? The latest artifact (as of now) can be found at https://github.com/James-Yu/LaTeX-Workshop/actions/runs/9595844297 .

James-Yu commented 3 weeks ago

I was quite confident on the new implementation until I revisited #3754 and noticed so many edge but practical usages 🤣

James-Yu commented 3 weeks ago

@quoc-ho The latest commit should work well with pages of different sizes.

quoc-ho commented 3 weeks ago

There's some scrolling issue with pdf reload when trimming level is changed. Steps to reproduce:

After each compile, the scrolling position will move up. This issue will disappear if the pdfviewer is closed and reopened.

James-Yu commented 3 weeks ago

After each compile, the scrolling position will move up. This issue will disappear if the pdfviewer is closed and reopened.

Caused by a racing condition on setting the scrollTop of viewer container. Fixed in ec788b8.

quoc-ho commented 3 weeks ago

The newest commit makes things flicker much more than before because the first page is displayed briefly before jumping to the original location. This happens even without trimming enabled. With #4281 incorporated, the flicker is still there (but slightly less). This is a new source of flicker.

quoc-ho commented 2 weeks ago

The newest commit doesn't fix the introduction of more flicker. Moreover, the previous problem with scrolling position reappears.

quoc-ho commented 2 weeks ago

Unfortunately, the extra flicker is still there (where the first page is briefly displayed) with trimming on. Moreover, the only zoom level that works with trimming is Automatic Zoom. Changing the zoom level reintroduces the bug with the scrolling position and moreover, the zoom level gets changed after a reload.

James-Yu commented 2 weeks ago

Flickering is not the top priority issue to be handled in this PR. I am first working on the appearance of trimming.

Regarding zoom level, does trimming work on fit-width and fit-page? Besides auto, these two should be the only mode trimming work, as all others are the absolute percentage zooming, which trimming should not change the scale.

Further, I've tested fit-page and 200% zooming, no offset in reloading pdf found. May I know which zoom level and browser/in-editor viewer you are using?

James-Yu commented 2 weeks ago

cc8dbc3 should fix the zoom-level-reset problem. I am yet to reproduce the scrolling offset issue.

One more tip, you can use Refresh all LaTeX PDF viewers command to quickly reload pdfs, without needing to re-compile.

James-Yu commented 2 weeks ago

The latest commit should be working in any cases (hopefully).

The jump-to-page-1-then-back was indeed not a regression. Original implementation also behave as is. I will look into improving that after the new trimming is polished.

quoc-ho commented 2 weeks ago

Everything seems to work, even when incorporating my newest patch for the flicker! For some reason, the scrolling to top is not even visible anymore. 🎉🎉🎉🎉🎉

quoc-ho commented 2 weeks ago

I celebrated too early. The issue with losing the scroll position happens when there's a landscape page in the middle. Also, sometimes, the portrait pages are aligned centrally compared to the landscape page and sometimes they are aligned on the left, seemingly depending on where the landscape page is in the viewport. And I know that it's not the main issue here, but maybe it's related: when there are landscape pages, the scrolling to top problem is more visible. The recording below was taken without my patch.

https://github.com/James-Yu/LaTeX-Workshop/assets/10014608/0b719d14-caa2-43f7-97e8-68c4e709ab50

quoc-ho commented 2 weeks ago

On this branch, Page-width/page-fit depends on the current page rather than the largest page in the document whereas on the master branch, things behave differently. So this seems to be a minor issue having to do with computing the right number.

James-Yu commented 2 weeks ago

The issue with losing the scroll position happens when there's a landscape page in the middle.

On it.

edit: fixed in 299e786. When the scale changes due to different current page, the position can be incorrect. This is a pdf.js viewer problem, the absolute scrollTop is correct.

Also, sometimes, the portrait pages are aligned centrally compared to the landscape page and sometimes they are aligned on the left, seemingly depending on where the landscape page is in the viewport.

That was really a good catch. However, this is a pdf.js viewer issue. Same thing happens with main branch, and I confirmed on the viewer.mjs code and figured that the height and width of pages are calculated based on the current page. So the scale is not fixed for such pdfs. I don't think it's possible to fix it on the extension side though.

And I know that it's not the main issue here, but maybe it's related: when there are landscape pages, the scrolling to top problem is more visible.

On it soon.

Page-width/page-fit depends on the current page rather than the largest page in the document.

pdf.js viewer issue. L12614 and L12180-L12181 to be specific.

quoc-ho commented 2 weeks ago

The behavior is still different from the one in the master branch, and I think the one in the master branch is more desirable. Namely, in the master branch, the zoom level (in the case of page width/fit) might change on refresh but it only changes at most once. On the current branch, it changes whenever the current page switches from a landscape to a portrait page (or vice versa). So, a small scroll can mix everything up.

It might be the case that the behavior in the current branch is the default one for pdfjs while the one on the master branch happens to be better :).

James-Yu commented 2 weeks ago

This new implementation of trimming should be in a good shape to merge. Un-appeared bugs will be fixed.

Now will turn to 1) clean up latexworkshop.ts and remove/re-implement server-client communication code, 2) reduce flickering.