LNReader / lnreader-plugins

Repository to host source related issues, and requests for LNReader
MIT License
101 stars 51 forks source link

Created basic Re:Library support #936

Closed Maix0 closed 1 month ago

Maix0 commented 1 month ago

Simple Extension that adds support for Re:Library (https://re-library.com)

Please do give feedback on poor code choice since it is my first plugin, and is probably terrible haha

it does seems to work enough for my use case (reading a single light novel).

Batorian commented 1 month ago

Only thing I'd ask is why you chose version 0.0.0 for your plugin. Shouldn't it be 1.0.0?

Maix0 commented 1 month ago

Welp it was when I was prototyping it, but yes it should be 1.0.0 (or 0.x.0) let me fix it

Maix0 commented 1 month ago

Also this would close #933, but I didn't see the issue before doing the plugin haha

Maix0 commented 1 month ago

I fixed the genre in the ParseNovel function, and implemented a crude search function, but the way it is implement make it not very useful.

To be consise, I can't get all the novel that match X search term in a single page using the website seatch because it is also spammed with unwanted (by us) items. this makes it so a website search page might have nothing that is interesting to us, and thus the app thinks it reached the last page.

So for example searching Hero doesn't show every result possible (for example Hero king as I am writing this) because the second page has zero interesting item, so it returns an empty array and voila...

nyagami commented 1 month ago

Path must be relative (without domain prefix). eg: /translations/i-reincarnated-and-became-the-daughter-of-a-dragon/. Novel status would not be presented in app if it is none of values defined by enum NovelStatus for now. image

Maix0 commented 1 month ago

I fixed the links being absolute and not shortened, tho I didn't do that treatment to the covers links since I feel they should be the full link (for example if hosted somewhere else ?).

also replaced "old style" string concatenation with the new template string everywhere, so it feels more readable. The summary is now only set when I'm able to extract it from the page, otherwise it should be undefined.

The fetchPopular has few safeguards to avoid spamming request (only fetch if page == 1 since there is a single page, and also returns directly if asked to get latests, since it isn't feasable from my understanding)

I also wonder if I should skip chapters where I don't find the link, but it would also allow the user to see that there is an issue (and report it). Your call

nyagami commented 1 month ago

Yeah, cover url have to be absolute, only novel/chapter path must be relative -> if site domain get changed, app can still update novel. you can leave invalid chapter paths as long as it is unique for that novel.

Maix0 commented 1 month ago

you can leave invalid chapter paths as long as it is unique for that novel.

Currently the invalid chapter path are just an empty string... I should probably name them something along invalid-url-${chapterIdx}

Such that it is easy to see which chapter is broken

Maix0 commented 1 month ago

I'm wondering about something:

Currently the only way to find a novel is either through the search (which is unrealiable!), or through the most popular, which is limited in number.

Should the most popular be adapted to work as is, but also append every other novels by scraping a another (single) page (which house link to every novel) ?

Basically it would mean that it scrap the most popular page, and also add the rest from the "all" novels, but in a single "most popular page".

This would allow the user to see through every novel the website offers, even though it is subobtimal in terms of user experience (no covers provided for the other novels)

nyagami commented 1 month ago

Spamming request could lead users to be banned by the site. I think you can keep popular page for popular novels and use all page for search novels.

Maix0 commented 1 month ago

Spamming request could lead users to be banned by the site. I think you can keep popular page for popular novels and use all page for search novels.

I don't quite understand this.

You are suggesting that I use the all page in the SearchNovels functions ? This could work (tho I would need to reimplement the "search" part in the plugin, which can be done)

My original suggestion was to fetch the popular page, take every novel from there, and also fetch the all page to add the missing ones after (probably in alphabetical order, to stay consitent).

This would mean the PopularNovels function would perform two fetches when called, but only if the pageNo is 1 and you aren't asking for the newest/latest.

I can see the issue with the app making two back to back fetches tho...

You might have more insight on what is the best to do, but in the meantime i'm going to implement the app-side search since it will probably be better then the current. The PopularNovels approch might also be something that will be implemented along side this app-side search so it can be added later (and it is pretty easy to add)

Maix0 commented 1 month ago

I made few changes:

Happy code review haha (the code is pretty easy to easy to read IMO)

nyagami commented 1 month ago

If novel cant be parsed, ignore or throw an error instead. But in this case, you can just use the novelPath from parseNovel function parameters image

Maix0 commented 1 month ago

If novel cant be parsed, ignore or throw an error instead. But in this case, you can just use the novelPath from parseNovel function parameters image

I did some general fixes, basically throwing errors or skipping iteration where it wouldn't make any sense to include them (for example in chapters, skipping chapter that doesn't have a name/path). I also found some == that were converted to === because they should...

Any issues about the scraping should be fixed (hopefully).

Thanks again for the code review :D

Edit: Forgot to mentions that dates are set to null for chapter since we knowingly don't have them, which is a personal preference I guess