Add DownloadToEpub.koplugin #9943
Conversation
|
Using another working link: Guess I need that other PR of yours :) but enough playing for today :) |
|
Ok, a bit more playing. |
|
He's playing with it! How nice (: You'll want the latest commits (incoming...), which should fix the errors you'd been having. |
roygbyte
force-pushed
the
add_download_to_epub
branch
from
41678cc
to
273f869
Compare
|
Is there anything I need to change for this PR to be integrated? Also, well, do we want this PR integrated? |
|
OK, tried it again ;) Major issue is that a next download did overwrite the previous: all were saved to a single Also, the default download directory was outside my HOME directory (dunno if it's some remnants from my previous test), and I had to go hunt for it :) And it doesn't use the directory I select ! It keeps saving EPUBs in the original place.
So, a few usability issues to fix, but the main purpose works.
Dunno if you were supposed to download and tweak the CSS. But I guess we can all be fine without any CSS (which, on the web, are wilder than what crengine can support). Dunno if you could/should clean them out from the resulting HTML.
Anyway, code/PR wise, same comments as before about gazette: it's 35 files, with funky and variable coding style :) All this makes me not want to go look at the code :/ Also, can you explain in a few words how this works, from the moment you get the main HTML data to when you decide what to be downloaded (images ? css ?), and how you transform that main HTML data ? Why xml2lua ? Just to extract things, or do you make a DOM, and serialize it back to HTML so we're sure to get correct and balanced XHTML (that I guess, crengine appreciates :). (Personally, I would have made it as a single Lua script, doing the download + HTML as a string (no DOM) + some regexp to extract images and download/rework their URI like for Wikipedia.) Are you using this plugin ? How often ? How happy ? (I wish some other people would comment and test, because I have no real interest in the feature :/ This was just my curiosity and sympathy making me test and write all this :) |
|
I have zero interest in the plugin from a user POV, and my past experience with reviewing a from-scratch massively modular PR was... not a fun one (and made actually grasping how it all fit together basically impossible without resorting to a local IDE), so I have yet to find the block of time + will necessary to review it ;). |
|
Ahhhh I am haunted by the same issues my other work! Alas :'( I'm starting to see what you mean by the modularity being undesirable. There definitely can be some ways to reduce the file count by 50 percent at least. (On a side note: no longer taking Java so it can't get any worse, right?) Anyways ... it sounds like I should trash this and the other thing because they're so far gone and ... tbh .... I'm not using my ereader much more anyways :') |
|
I'm in the same boat btw. I'd certainly like to take a better look, but the code structure is less inviting to me. I can't grok it very well just looking at the GH diff or Reviewable due to the unnecessarily complex inheritance. If you compare the recent vocabbuilder plugin for example that was very clear for me to read. I'm not really sure how to turn that into useful/actionable feedback, which is why I hadn't responded yet. But for example, I don't really understand why "request" and "requestfactory" are separate files. I know the factory pattern is a popular thing in Java but my understanding of it is a bit different (but possibly wrong or outdated). I'd envision the pattern more like you have the request class and then you have several different factories depending on what type of request you're doing. I'm not sure how much sense that makes here regardless but we don't even have a postrequestfactory vs a getrequestfactory or something. :-) A similar example is the htmldocument stuff. To me there's no clear reason to split it up into 7 (?) files. I think I'd basically just have one or two files, the htmldocument and the template. That's mainly because at this point those are all one or two functions in an entirely new file/object. (Also see the ResourceIterator in webpageadapter.) In short, it comes across to me as using a pattern for the sake of using a pattern (or perhaps: to learn the pattern?) rather than because it's necessarily useful here. Anyway, as in writing good text, writing good code also often comes down to rewriting. That's not trashing it. ;-) |
Yeah, I was building it with a mind towards the future, where there would need to be many different factories for other subscription services. Probably that's why it's so bloated--those other services never appeared.
Actually, ha, yeah, the epub builder stuff twined with me learning about these types of patterns and then going whole hog on them. Oooopsies :)
True true true...! That would make it 10x easier to work with, too, since I'm not switching buffers so much to make a little change. Hmmm! |
Plugin adds a "Download To EPUB" button to external links.
This change is