rNeomy / reader-view

Access Firefox's built in reader view from right click context menu
https://webextension.org/listing/chrome-reader-view.html
Mozilla Public License 2.0
464 stars 73 forks source link

Next/Previous chapter navigation and OS theme sync #177

Closed Shrulik closed 10 months ago

Shrulik commented 10 months ago

Hey rNeomy,

I have implemented one extra main feature and a few minor features. I hope you would find this useful.

The main feature is that the code tries to detect links to the next page/chapter and allows opening the reader on that page using the press of a button without leaving the reader view, clicking something and reopening the reader, in sites like RoyalRoad or other web novel sites for example.

Looks like this: image

Keyboard shortcuts are supported. Ctrl + PageDown/Up by default. The shortcuts work even without starting the reader, so are useful even if the page itself isn't reader compatible, but has an idea of next/previous.

I have squashed the changes into features to make things easier, but the initial-dev branch has all the dirty details. The detection works often, but not always, since I often have to guess. I plan to expand this part in the future to support things like:

Another feature is OS theme sync. image Only tested on macOS.

I also added virtual white space because I found it confusing when nearing the end of the chapter and pressing page down would jump an unknown amount and I would find myself looking for where I was. This might just me, so I put this in a separate commit.

Regards, Shrulik

rNeomy commented 10 months ago

Thanks! I've added your bug fix and OS theme parts for now.

The main feature is pretty complex to review. Can you just request a pull merge for wrapper section that detects previous and next chapters for now? After we are done with this section, we can find a way to have the interface buttons and also the shortcuts.

Please do not add unnecessary files (e.g., v3/data/inject/next-chap/fast-levenshtein/package.json, v3/data/inject/next-chap/test/setup-jest.js, ....)

Also please do not change the styles in your commit:

const { width } = document.getElementById('toolbar').getBoundingClientRect();
const {width} = document.getElementById('toolbar').getBoundingClientRect();
Shrulik commented 10 months ago

a. I removed the files you mentioned, the jest file was an oversight. I believe the license file is legally required. Wasn't sure about the README.MD, do you want to replace it with a link like for Readability? Say if there are other files you consider redundant.

b. Sorry about the styling. Do you happen to have a format file I can apply automatically on the project to fit your preferences ?

c. I separated the link finding functionality as you asked. The rest is on the pull-next-with-ui branch.

rNeomy commented 10 months ago

I was hopping of a separate minimal commit that adds nextLink and prevLink to the article object for now. So that I can debug it. Basically I want to try the base function first (without UI and shortcut modifications).

In the current commit, I need to revert many things. For instance take a look at the committed v3/manifest.json https://github.com/Shrulik/reader-view/commit/b4405de5b18cfa0a323a4aa110b3d762220a12e9

Shrulik commented 10 months ago

You are right, moved more parts to the UI section including the manifest change.

By the way, the reason the manifest had so many changes was because it had CRLF for line ending unlike most of the code which ends with LF, so the commit changed it to LF as well.

rNeomy commented 10 months ago

Nice! A few notes:

  1. Please use scope on "NextChap". Then expose "self.extractChapLinks" so that the script can be reinjected.
  2. This function should not return the chapters when it is not confident enough. For instance on "https://en.wikipedia.org/wiki/Book", it returns "https://en.wikipedia.org/wiki/Book#cite_ref-1" as the next chapter
  3. I wasn't sure why you've used "chrome.storage.session" for this part. so I removed it for now.

My linter configs: eslint.zip

Shrulik commented 10 months ago
  1. You mean like it is done in : fastest-levenshtein/mod.js ?

  2. It makes sense not to display the UI if there is a low confidence result, I will change it. Need to be more careful on how I calculate confidence then. I think I will add a minimum confidence threshold that can be passed to the function, and nothing is returned if the best candidate doesn't pass it.

What happens in this example is that it is quite common that the main page of a site will be of the form www.example.com/ and the next page will be www.example.com/something-1 so I have to guess, but it is a pretty low confidence guess since an extra element with the number 1 is pretty common as we saw in the wiki example.

If there was a way for the user to communicate that a next page/chapter actually exists for the current page, then the chances of success would be higher. Maybe a different shortcut to open the reader in "paged mode" ?

  1. There are some scenarios where the nextChap was being reinserted which was throwing an error because a const like NavType were redefined so I tried caching the fact the library was already loaded. It helped, but it still happened occasionally. Had no effect besides an error in the console. The scoping you suggested will probably make this redundant.
rNeomy commented 10 months ago

You mean like it is done in : fastest-levenshtein/mod.js ?

Yes! Just place all the code in {} scope.

I think I will add a minimum confidence threshold that can be passed to the function, and nothing is returned if the best candidate doesn't pass it.

Perfect

If there was a way for the user to communicate that a next page/chapter actually exists for the current page, then the chances of success would be higher. Maybe a different shortcut to open the reader in "paged mode" ?

We will work on the interface later. For now, I think having a confidence parameter helps. For example, we can display the addresses of the next and previous chapters at the bottom of each view.