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

fix: prevent pdf flickering #355

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

Zir0h
Copy link
Contributor

@Zir0h Zir0h commented Nov 5, 2023

Possible fix for #220
Looks like the page is rendered twice when doing next/previous, this causes it to jump around on the UI

Copy link

github-actions bot commented Nov 5, 2023

PR Preview Action v1.4.4
🚀 Deployed preview to https://teia-community.github.io/teia-ui/previews/pr-355/
on branch gh-pages at 2023-11-28 18:18 UTC

@melMass
Copy link
Member

melMass commented Nov 7, 2023

I can still see the flicker, what you removed was actually an attempt at removing the flicker (that failed).
I just tested it briefly, but with this PR all page flicker, whereas in master page flicker only once but once cached they don't anymore

@melMass
Copy link
Member

melMass commented Nov 7, 2023

It's like 1 frame 😅
pdf

@Zir0h
Copy link
Contributor Author

Zir0h commented Nov 7, 2023

I can still see the flicker, what you removed was actually an attempt at removing the flicker (that failed). I just tested it briefly, but with this PR all page flicker, whereas in master page flicker only once but once cached they don't anymore

aah okay! could it be related to the pdfjs version maybe? we're currently 2 major versions behind it seems 😅

@Zir0h
Copy link
Contributor Author

Zir0h commented Nov 7, 2023

In the pdfjs examples they use a rendering queue, see queueRenderPage

@melMass
Copy link
Member

melMass commented Nov 7, 2023

we're currently 2 major versions behind it seems 😅

Yeah I think that would be wise to update 😅

In the pdfjs examples they use a rendering queue, see queueRenderPage

This looks like the proper implementation of my attempt

@Zir0h
Copy link
Contributor Author

Zir0h commented Nov 7, 2023

updating didn't change much, it still goes like page 1 => blank page for a flash => page 2 => blank page for a flash => page 3, it's weird

@Zir0h Zir0h marked this pull request as draft November 7, 2023 18:12
@Zir0h
Copy link
Contributor Author

Zir0h commented Nov 8, 2023

The example in react-pdf also flickers 😆

@melMass
Copy link
Member

melMass commented Nov 8, 2023

The example in react-pdf also flickers 😆

Ohhh indeed ahah🤦 !
But the one you posted earlier is a good reference.
I will have time to dive and test more tomorrow

@Zir0h
Copy link
Contributor Author

Zir0h commented Nov 8, 2023

The example in react-pdf also flickers 😆

Ohhh indeed ahah🤦 ! But the one you posted earlier is a good reference. I will have time to dive and test more tomorrow

This looks like what you were trying to do before? With some hidden page shenanigans 😆

@Zir0h
Copy link
Contributor Author

Zir0h commented Nov 21, 2023

we're currently 2 major versions behind it seems 😅

Yeah I think that would be wise to update 😅

In the pdfjs examples they use a rendering queue, see queueRenderPage

This looks like the proper implementation of my attempt

This code also has the same issue. I think that the minted pdf files on teia are more "image" type than "text", so rendering them is always harder on the browser than just ASCII :/

@Zir0h
Copy link
Contributor Author

Zir0h commented Nov 21, 2023

objkt.com uses https://www.npmjs.com/package/ng2-pdf-viewer which works very well, but I suppose there's no way to make that work on react?

//edit: without using iframes (because that would be easy I think, like we do for previews with iframe.teia.art)

@Zir0h
Copy link
Contributor Author

Zir0h commented Nov 22, 2023

https://vadimdez.github.io/ng2-pdf-viewer/ this is snappy 😄 just put an ipfs link as source and uncheck "show all pages"

@melMass
Copy link
Member

melMass commented Nov 22, 2023

vadimdez.github.io/ng2-pdf-viewer this is snappy 😄 just put an ipfs link as source and uncheck "show all pages"

Angular is widely different not sure how we could do that sanely tbh.

@melMass melMass added 😇 help wanted Extra attention is needed 🤔 Priority: Medium This issue may be useful, and needs some attention. labels Nov 24, 2023
@melMass melMass marked this pull request as ready for review November 28, 2023 17:58
@melMass
Copy link
Member

melMass commented Nov 28, 2023

Will merge tomorrow if no one see objections.

@Zir0h just migrated the iframe thing we removed a few months back to it's own repo -> https://github.com/teia-community/teia-iframe

The idea proposed by Z0 would be to add support for pdf there so that we can use whatever work best

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
😇 help wanted Extra attention is needed 🤔 Priority: Medium This issue may be useful, and needs some attention.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants