PDF viewer in app (not external link) #2903

New issue

Closed

opened 2019-09-18 17:33:31 +02:00 by tzarebczan · 5 comments

tzarebczan commented

2019-09-18 17:33:31 +02:00

(Migrated from github.com)

We used to be able to open PDFs right inside the app and I believe it broke with one of the Chrome/Electron updates. We should investigate if this is possible when we upgrade Electron.

A user was recently not happy with this behavior and wrote up a script to support pdfs in .lbry files, but I don't think that's the right way to go - it should be possible to open them up right in the app.
https://www.reddit.com/r/lbry/comments/d5uxnd/let_your_users_read_pdf_ebooks_within_lbry_app_no/

We used to be able to open PDFs right inside the app and I believe it broke with one of the Chrome/Electron updates. We should investigate if this is possible when we upgrade Electron. A user was recently not happy with this behavior and wrote up a script to support pdfs in .lbry files, but I don't think that's the right way to go - it should be possible to open them up right in the app. https://www.reddit.com/r/lbry/comments/d5uxnd/let_your_users_read_pdf_ebooks_within_lbry_app_no/

jeffslofish commented

2020-05-18 08:29:11 +02:00

(Migrated from github.com)

Looking into this...

kauffj commented

2020-05-18 23:26:13 +02:00

(Migrated from github.com)

Oops, I actually researched this when doing some weekend hacking and never updated the ticket.

I believe if you update the core Electron dependency that this will work again.

Oops, I actually researched this when doing some weekend hacking and never updated the ticket. I believe if you update the core Electron dependency that this will work again.

jeffslofish commented

2020-05-21 07:47:04 +02:00

(Migrated from github.com)

Here is what I did to get PDFs to work in the app:

Update core Electron dependency to 9.0.0
Set the nodeIntegration option to true (because it now defaults to false) under webPreferences here: https://github.com/lbryio/lbry-desktop/blob/master/electron/createWindow.js#L36
Remove PDF from the const UNSUPPORTED_IN_THIS_APP here: https://github.com/lbryio/lbry-desktop/blob/master/ui/constants/file_render_modes.js#L30
In lbry-redux, add mode: 'no-cors', to the apiCall options, here: https://github.com/lbryio/lbry-redux/blob/master/src/lbry.js#L186

Setting the mode to no-cors seems like a bad thing to do, but I couldn't find any way around it. Perhaps there needs to be a change to the lbry-sdk to make cors work with the latest electron?

This is what happens if the mode isn't set to 'no-cors':

I could submit a pull request with these changes (actually two pull request because some changes are in this repo and one is in lbry-redux) if you like, but I figured you might want to discuss this further before pulling in the latest version of Electron and making these changes.

The PDF issue in Electron was fixed in 9.0.0, which was recently released. There are some deprecations and other changes that require more changes in the code than just setting the Electron dependency to 9.0.0. **Here is what I did to get PDFs to work in the app**: 1. Update core Electron dependency to 9.0.0 2. Set the nodeIntegration option to true (because it now defaults to false) under webPreferences here: https://github.com/lbryio/lbry-desktop/blob/master/electron/createWindow.js#L36 3. Remove PDF from the const UNSUPPORTED_IN_THIS_APP here: https://github.com/lbryio/lbry-desktop/blob/master/ui/constants/file_render_modes.js#L30 4. In lbry-redux, add `mode: 'no-cors',` to the apiCall options, here: https://github.com/lbryio/lbry-redux/blob/master/src/lbry.js#L186 Setting the mode to no-cors seems like a bad thing to do, but I couldn't find any way around it. Perhaps there needs to be a change to the lbry-sdk to make cors work with the latest electron? This is what happens if the mode isn't set to 'no-cors': ![405 method not allowed](https://user-images.githubusercontent.com/1240484/82527267-dda7ae00-9aea-11ea-8b4d-02eaadf9e07a.png) I could submit a pull request with these changes (actually two pull request because some changes are in this repo and one is in lbry-redux) if you like, but I figured you might want to discuss this further before pulling in the latest version of Electron and making these changes.

👍 1

neb-b commented

2021-01-26 17:32:20 +01:00

(Migrated from github.com)

Electron has been updated to version 9 so we should be able to fix this now.

jeffslofish commented

2021-01-27 03:08:31 +01:00

(Migrated from github.com)

Hi! It has been a while, but I am going to try to get setup again and work on this issue now.