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

PocketBook: Open links in the on-device web browser #11787

Merged
merged 2 commits into from May 18, 2024

Conversation

liskin
Copy link
Contributor

@liskin liskin commented May 9, 2024

Many PocketBook devices include a web browser, but when clicking a link in KOReader, there was no option to open the link in the browser, there was only an option to show a QR code (which can then be scanned by a smartphone).

This commit implements canOpenLink/openLink on PocketBook using the "browser.app", if available.

Tested on PB740 (InkPad 3).

Fixes: #11782
Related: #6597


This change is Reviewable

@liskin liskin force-pushed the pocketbook-open-link-browser branch from 7b5ebb5 to a1969c3 Compare May 9, 2024 13:34
Copy link
Member

@Frenzie Frenzie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@Frenzie Frenzie added this to the 2024.05 milestone May 9, 2024
@liskin
Copy link
Contributor Author

liskin commented May 13, 2024

I asked @sdasda7777 to test this on PB624 (Basic Touch, released in 2013, stuck on firmware 4.4) and had to make some adjustments (that also led to the discovery of PackParameters which abstracts away the low-level format of the data argument to SendRequest*). Waiting for confirmation that the fixup works there.

@liskin liskin force-pushed the pocketbook-open-link-browser branch from 3cd6b43 to 62d3310 Compare May 15, 2024 20:38
@liskin
Copy link
Contributor Author

liskin commented May 15, 2024

We're still having some trouble making this work on PB624, but while researching how stuff works there, I've managed to simplify the implementation to a single inkview.OpenBook2 call, so that's good.

The troublesome scenario on the PB624 is this:

  1. open KOReader
  2. click link and wait until it opens in the browser
  3. hold home button to switch back to KOReader without closing the browser
  4. click another link and check that it opened in the browser

For some reason, the last step fails—the browser doesn't switch to the new URL. But we discovered that it doesn't work with the built-in reader software either, so we could perhaps just call it broken and leave it as is.

Or, we could check if the firmware is super old and if it is, kill the browser and then open a new one. I've already wasted so much time on this that a little extra won't hurt 😆

@liskin liskin force-pushed the pocketbook-open-link-browser branch from 62d3310 to bfedf1a Compare May 15, 2024 21:37
@liskin
Copy link
Contributor Author

liskin commented May 16, 2024

Okay so I can't make the browser killing work even on the InkPad 3, but I looked a bit deeper into how browser.app on PB624 implements the request handler and it seems that OpenBook just re-execs the entire browser binary, which might just work. And it still works on newer devices, so I switched the impl to it. Waiting for @sdasda7777 to confirm it's better on the old device.

And even if it isn't, we now have a single-line implementation that, if nothing else, doesn't crash KOReader because that API has been there since forever. I'm calling it a day. :-)

@Frenzie
Copy link
Member

Frenzie commented May 16, 2024

Would you mind posting the other thing for posterity? Force pushing seems like a bit of a pity in this case. :-)

@liskin
Copy link
Contributor Author

liskin commented May 16, 2024

Oh, absolutely, I can make this into two commits, where one implements the most complicated thing I had with the comments about OpenTask and then the second one replaces it with the final impl. Gimme a sec

Many PocketBook devices include a web browser, but when clicking a link
in KOReader, there was no option to open the link in the browser, there
was only an option to show a QR code (which can then be scanned by a
smartphone).

This commit implements `canOpenLink`/`openLink` on PocketBook using the
"browser.app", if available.

Tested on PB740 (InkPad 3) and PB624 (Basic Touch).
On the older device (PB624) it doesn't work quite as well—an already
running browser ignores the request to switch URL, but it doesn't work
with the built-in book reader software either.

Fixes: koreader#11782
Related: koreader#6597
Somewhat unintuitively, OpenBook does exactly what we want if we pass
the browser binary as the book and the URL as parameters.

It does a couple extra things we don't need, like checking if the
browser binary is being viewed by the built-in epub reading software,
and possibly some more stuff I don't quite understand, but at the end it
does almost exactly what OpenTask from the new PocketBook SDK does.

Actually, OpenBook2 is closer to OpenTask, as it takes `const char **argv`
rather than a single argument, but: we don't need multiple arguments,
and the single-argument version might work better on older PocketBook
firmwares (like the PB624 which is stuck with InkView 4.4).
@liskin liskin force-pushed the pocketbook-open-link-browser branch from bfedf1a to 916050d Compare May 16, 2024 11:27
@liskin
Copy link
Contributor Author

liskin commented May 16, 2024

(force)pushed 🙂

@liskin
Copy link
Contributor Author

liskin commented May 16, 2024

Okay, final update: PB624 still won't switch the URL, even with OpenBook. As I said earlier, this is no worse than the built-in reader, so there's not much we can do. I consider the PR ready to merge then.

(I'll be happy to investigate if there's bugs with any 5.* or 6.* firmware devices, but 4.4 is really to old to spend more time on it.)

@Frenzie
Copy link
Member

Frenzie commented May 16, 2024

Thanks, then I'll squash and merge with GH. You're sure you're ready? 😆

@liskin
Copy link
Contributor Author

liskin commented May 16, 2024

Well if you squash you won't preserve the intermediate state...
But yeah, I'm not going to do any more testing myself and I don't know who else to ask to test on their device so let's get it in and out to users 👍

@Frenzie
Copy link
Member

Frenzie commented May 16, 2024

Well if you squash you won't preserve the intermediate state...

But we will, right here. I could rebase it if you really prefer but I just wanted it in the PR.

@liskin
Copy link
Contributor Author

liskin commented May 16, 2024

Okay, whatever works 🙂
Just please take the recent commit messages rather than the PR desc which I don't think is up to date

@liskin
Copy link
Contributor Author

liskin commented May 16, 2024

Actually, the PR desc is fine (sorry am on mobile, short attention span)

@Frenzie Frenzie merged commit e94550a into koreader:master May 18, 2024
3 checks passed
@liskin liskin deleted the pocketbook-open-link-browser branch May 18, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FR: Open links in PocketBook web browser
2 participants