Gazette plugin to replace Newsdownloader #9637
Conversation
There was a problem hiding this comment.
Reviewed 72 of 72 files at r1, all commit messages.
Reviewable status: all files reviewed, 50 unresolved discussions (waiting on @roygbyte)
plugins/gazette.koplugin/gazettemessages.lua line 4 at r1 (raw file):
local GazetteMessages = {
Put everything inside it at declaration time, this'll save the interpreter a bunch of useless reallocs to grow the table.
Code quote:
local GazetteMessages = {
}plugins/gazette.koplugin/main.lua line 17 at r1 (raw file):
local ViewResults = require("composers/view_results") local Gazette = WidgetContainer:new{
extend
plugins/gazette.koplugin/composers/configure_subscription.lua line 16 at r1 (raw file):
local ConfigureSubscription = { subscription = nil, }
This appears to be storing a reference to dynamic objects inside a module-local, beware of pinned references and/or object lifecycle.
(The layers of indirection do not help having any idea of what this object will actually look like and how critical that might be).
Code quote:
local ConfigureSubscription = {
subscription = nil,
}plugins/gazette.koplugin/composers/configure_subscription.lua line 48 at r1 (raw file):
Trapper:info(GazetteMessages.CONFIGURE_SUBSCRIPTION_TEST_FEED_BEGIN) local test_subscription, err = ConfigureSubscription:createFeedFromDialog(dialog)
self:?
plugins/gazette.koplugin/composers/view_results.lua line 25 at r1 (raw file):
local subscription = FeedSubscription:new({ id = subscription_results.subscription_id })
Our usual style for object instantiation is to omit the parens, e.g., Class:new{constructor}
(c.f., https://www.lua.org/manual/5.1/manual.html#2.5.8)
Code quote:
local subscription = FeedSubscription:new({
id = subscription_results.subscription_id
})plugins/gazette.koplugin/composers/view_results.lua line 58 at r1 (raw file):
end self.view = KeyValuePage:new{
Same caveats about pinning a ref to a module-local as usual ;).
plugins/gazette.koplugin/composers/view_subscriptions.lua line 31 at r1 (raw file):
end self.view = KeyValuePage:new{
Ditto.
(This is doubly problematic in a plugin, because this is cached in package.loaded, so it won't even magically go away once the Plugin instance gets dropped on view change).
plugins/gazette.koplugin/feed/feederror.lua line 5 at r1 (raw file):
local T = require("ffi/util").template local FeedError = HttpError:new{
extend
plugins/gazette.koplugin/feed/feederror.lua line 12 at r1 (raw file):
FeedError.FEED_HAS_NO_CONTENT = _("The feed didn't return any content.") FeedError.RESPONSE_NOT_XML = _("Feed is not an XML document.") FeedError.FEED_NOT_SUPPORTED_SYNDICATION_FORMAT = _("URL is not a supported syndication format.")
Move into class object declaration.
Code quote:
FeedError.FEED_NONSPECIFIC_ERROR = _("There was an error. That's all I know.")
FeedError.FEED_HAS_NO_CONTENT = _("The feed didn't return any content.")
FeedError.RESPONSE_NOT_XML = _("Feed is not an XML document.")
FeedError.FEED_NOT_SUPPORTED_SYNDICATION_FORMAT = _("URL is not a supported syndication format.")plugins/gazette.koplugin/feed/feederror.lua line 39 at r1 (raw file):
end return getmetatable(self).__index:provideFromResponse(response)
This is probably not doing what you think it's doing ;). self == self.__index == FeedError, which makes it also == getmetatable(self) == getmetatable(self).__index
Plus, you know your base class is HttpError, so just call HttpError.provideFromResponse(self, response).
plugins/gazette.koplugin/feed/feedresponse.lua line 3 at r1 (raw file):
local Response = require("libs/http/response") local FeedResponse = Response:new {
extend
plugins/gazette.koplugin/feed/atom/atomentry.lua line 3 at r1 (raw file):
local Entry = require("feed/entry") local AtomEntry = Entry:new {
extend
(You may not currently actually have a distinction between extend and new for some of these classes, I'd recommend a simple extend == new declaration for those that do not use an init function, just to honor the semantics (c.f., #9586)).
plugins/gazette.koplugin/feed/atom/atomfeed.lua line 6 at r1 (raw file):
local util = require("util") local AtomFeed = Feed:new {
Definitely extend, because that one has an init ;).
plugins/gazette.koplugin/feed/atom/atomfeed.lua line 15 at r1 (raw file):
email = nil, uri = nil, },
Move initialization of that to the init fn to avoid inheritance shenanigans.
Code quote:
author = {
name = nil,
email = nil,
uri = nil,
},plugins/gazette.koplugin/feed/atom/atomfeed.lua line 22 at r1 (raw file):
logo = nil, rights = nil, entries = {},
Ditto
plugins/gazette.koplugin/feed/atom/atomfeed.lua line 44 at r1 (raw file):
self.title = util.htmlEntitiesToUtf8(channel.title) else self.title = T(GazetteMessages.UNTITLED_FEED, self.link)
What's T?
plugins/gazette.koplugin/feed/rdf/rdffeed.lua line 1 at r1 (raw file):
local RdfFeed = Feed:new {
extend
plugins/gazette.koplugin/feed/rss/rssentry.lua line 3 at r1 (raw file):
local Entry = require("feed/entry") local RssEntry = Entry:new {
extend
plugins/gazette.koplugin/feed/rss/rssfeed.lua line 7 at r1 (raw file):
local util = require("util") local RssFeed = Feed:new {
extend
plugins/gazette.koplugin/feed/rss/rssfeed.lua line 22 at r1 (raw file):
title = nil, link = nil, },
Move to init (you can keep a nil placeholder like the rest, it helps the interpreter know how large the table will be).
Code quote:
image = {
url = nil,
title = nil,
link = nil,
},plugins/gazette.koplugin/feed/rss/rssfeed.lua line 25 at r1 (raw file):
skipHours = nil, skipDays = nil, entries = {}
Ditto
plugins/gazette.koplugin/libs/gazette/epub32writer.lua line 5 at r1 (raw file):
local xml2lua = require("libs/xml2lua/xml2lua") local Epub32Writer = ZipWriter:new {
extend
plugins/gazette.koplugin/libs/gazette/epubbuilddirector.lua line 6 at r1 (raw file):
writer = nil, epub = nil, result = nil,
Same comments about pinned refs as the other module-locals ;).
Code quote:
writer = nil,
epub = nil,
result = nil,plugins/gazette.koplugin/libs/gazette/epuberror.lua line 21 at r1 (raw file):
EpubError.MANIFEST_ITEM_ALREADY_EXISTS = _("Item already exists in manifest") EpubError.MANIFEST_ITEM_NIL = _("Can't add a nil item to the manifest.") EpubError.SPINE_BUILD_ERROR = _("Could not build spine part for item.")
Move inside table declaration
Code quote:
EpubError.EPUB_INVALID_CONTENTS = _("Contents invalid")
EpubError.EPUBWRITER_INVALID_PATH = _("The path couldn't be opened.")
EpubError.ITEMFACTORY_UNSUPPORTED_TYPE = _("Item type is not supported.")
EpubError.ITEMFACTORY_NONEXISTENT_CONSTRUCTOR = _("Item type is supported but ItemFactory doesn't have a constructor for it.")
EpubError.RESOURCE_WEBPAGE_INVALID_URL = _("")
EpubError.ITEM_MISSNG_ID = _("Item missing id")
EpubError.ITEM_MISSING_MEDIA_TYPE = _("Item missing media type")
EpubError.ITEM_MISSING_PATH = _("Item missing path")
EpubError.ITEM_NONSPECIFIC_ERROR = _("Something's wrong with your item. That's all I know")
EpubError.IMAGE_UNSUPPORTED_FORMAT = _("Image format is not supported.")
EpubError.MANIFEST_BUILD_ERROR = _("Could not build manifest part for item.")
EpubError.MANIFEST_ITEM_ALREADY_EXISTS = _("Item already exists in manifest")
EpubError.MANIFEST_ITEM_NIL = _("Can't add a nil item to the manifest.")
EpubError.SPINE_BUILD_ERROR = _("Could not build spine part for item.")plugins/gazette.koplugin/libs/gazette/epub/epub.lua line 3 at r1 (raw file):
local Package = require("libs/gazette/epub/package") local Epub = Package:new{
extend
plugins/gazette.koplugin/libs/gazette/epub/epub.lua line 8 at r1 (raw file):
function Epub:new(o) o = Package:new()
That's... strange?
Either don't take an o as input, or make it work via proper class inheritance?
plugins/gazette.koplugin/libs/gazette/epub/package/item.lua line 28 at r1 (raw file):
function Item:generateId() self.id = "a" .. md5(self.path) -- IDs can't start with number
Not sure what this is used for, but we have a thingy that generates UUID v4 somewhere, in case it helps ;).
plugins/gazette.koplugin/libs/gazette/epub/package/item/image.lua line 5 at r1 (raw file):
local util = require("util") local Image = Item:new {
extend
plugins/gazette.koplugin/libs/gazette/epub/package/item/image.lua line 26 at r1 (raw file):
then return false, EpubError.ITEM_MISSING_PATH end
That's... mildly unusual, but okay. It'll always fail when passing an empty constructor, though, so, make it clear that not supported by moving the check earlier?
(Note that this is the sort of stuff we'd put in a dedicated init function, to keep new clean (applies to a few classes before, too)).
Code quote:
if not o.path
then
return false, EpubError.ITEM_MISSING_PATH
endplugins/gazette.koplugin/libs/gazette/epub/package/item/nav.lua line 5 at r1 (raw file):
local xml2lua = require("libs/xml2lua/xml2lua") local Nav = Item:new{
extend
plugins/gazette.koplugin/libs/gazette/epub/package/item/xhtmlitem.lua line 5 at r1 (raw file):
local util = require("util") local XHtmlItem = Item:new {
extend
plugins/gazette.koplugin/libs/gazette/resources/htmldocument.lua line 5 at r1 (raw file):
local util = require("util") local HtmlDocument = Resource:new{
extend
plugins/gazette.koplugin/libs/gazette/resources/image.lua line 4 at r1 (raw file):
local Resource = require("libs/gazette/resources/resource") local Image = Resource:new{
extend
plugins/gazette.koplugin/libs/gazette/resources/webpage.lua line 9 at r1 (raw file):
local socket_url = require("socket.url") local WebPage = Resource:new {
extend
plugins/gazette.koplugin/libs/gazette/resources/webpage.lua line 79 at r1 (raw file):
local item, err = ItemFactory:makeItemFromResource(resource) if err then
nit: Haven't noticed if there were any of these earlier, but we usually keep the then on the same line (unless the conditional is multi-line or something).
Code quote:
if err
thenplugins/gazette.koplugin/libs/gazette/resources/webpageadapter.lua line 14 at r1 (raw file):
return webpage.items[i] end end
I'd have to see how this is used, but I'm fairly certain there's a less clunky way of writing that ;o).
Code quote:
local i = 0
local item_count = #webpage.items
return function()
i = i + 1
if i <= item_count
then
return webpage.items[i]
end
endplugins/gazette.koplugin/libs/http/httperror.lua line 12 at r1 (raw file):
HttpError.REQUEST_INCOMPLETE = _("Request couldn't complete. Code %1.") HttpError.REQUEST_PAGE_NOT_FOUND = _("Page not found.") HttpError.RESPONSE_HAS_NO_CONTENT = _("No content found in response.")
Move to table declaration
Code quote:
HttpError.RESPONSE_NONSPECIFIC_ERROR = _("There was an error. That's all I know.")
HttpError.REQUEST_UNSUPPORTED_SCHEME = _("Scheme not supported.")
HttpError.REQUEST_INCOMPLETE = _("Request couldn't complete. Code %1.")
HttpError.REQUEST_PAGE_NOT_FOUND = _("Page not found.")
HttpError.RESPONSE_HAS_NO_CONTENT = _("No content found in response.")plugins/gazette.koplugin/libs/http/request.lua line 19 at r1 (raw file):
timeout = DEFAULT_TIMEOUT, redirects = DEFAULT_REDIRECTS, sink = {},
Move to object instantiation (right now, you're sharing a sink across all instances, and keeping it pinned).
plugins/gazette.koplugin/subscription/state.lua line 11 at r1 (raw file):
State.STATE_FILE = "gazette_subscription_config.lua" State.ID_PREFIX = "subscription_" State.DATA_STORAGE_DIR = DataStorage:getSettingsDir()
Move to table declaration
Code quote:
State.STATE_FILE = "gazette_subscription_config.lua"
State.ID_PREFIX = "subscription_"
State.DATA_STORAGE_DIR = DataStorage:getSettingsDir()plugins/gazette.koplugin/subscription/subscription.lua line 3 at r1 (raw file):
local State = require("subscription/state") local Subscription = State:new{
extend
plugins/gazette.koplugin/subscription/subscription.lua line 26 at r1 (raw file):
self.last_updated = o.last_updated self.last_fetch = o.last_fetch self.subscription_type = o.subscription_type
Not entirely sure this makes sense, given that self is o?
Code quote:
self.id = o.id
self.last_updated = o.last_updated
self.last_fetch = o.last_fetch
self.subscription_type = o.subscription_typeplugins/gazette.koplugin/subscription/subscriptionfactory.lua line 10 at r1 (raw file):
SubscriptionFactory.SUBSCRIPTION_TYPES = { feed = "feed" }
Move to table declaration
Code quote:
SubscriptionFactory.SUBSCRIPTION_TYPES = {
feed = "feed"
}plugins/gazette.koplugin/subscription/subscriptions.lua line 9 at r1 (raw file):
local ResultsFactory = require("subscription/result/resultsfactory") local Subscriptions = State:new{
extend
plugins/gazette.koplugin/subscription/result/results.lua line 8 at r1 (raw file):
local ResultsFactory = require("subscription/result/resultsfactory") local Results = State:new{
extend
plugins/gazette.koplugin/subscription/result/subscription_sync_result.lua line 6 at r1 (raw file):
local ResultFactory = require("subscription/result/resultfactory") local SubscriptionSyncResult = State:new{
extend
plugins/gazette.koplugin/subscription/result/subscription_sync_result.lua line 29 at r1 (raw file):
self.id = o.id self.subscription_id = o.subscription_id self.timestamp = o.timestamp
Ditto, self is o?
Code quote:
self.results = o.results
self.id = o.id
self.subscription_id = o.subscription_id
self.timestamp = o.timestampplugins/gazette.koplugin/subscription/type/feed.lua line 10 at r1 (raw file):
local Results = require("subscription/result/results") Feed = Subscription:new{
extend
plugins/gazette.koplugin/subscription/type/feed.lua line 25 at r1 (raw file):
CONTENT = "content", WEBPAGE = "webpage", }
Move to table declaration
Code quote:
Feed.CONTENT_SOURCE = {
SUMMARY = "summary",
CONTENT = "content",
WEBPAGE = "webpage",
}plugins/gazette.koplugin/subscription/type/feed.lua line 50 at r1 (raw file):
self.include_images = o.enabled_filter -- not implemented self.filter_element = o.filter_element -- not implemented self.content_source = o.content_source
Same as earlier? self is o.
(And self is aFeed, so inheritance should take care of subscription_type; unless Feed itself can be subclassed to something that sets this field to something else).
Code quote:
self.url = o.url
self.limit = o.limit
self.download_full_article = o.download_full_article -- not implemented
self.download_directory = o.download_directory
self.include_images = o.enabled_filter -- not implemented
self.filter_element = o.filter_element -- not implemented
self.content_source = o.content_sourceplugins/gazette.koplugin/views/subscription_edit_dialog.lua line 14 at r1 (raw file):
EditDialog.DOWNLOAD_DIRECTORY = 2 EditDialog.LIMIT = 3 EditDialog.CONTENT_SOURCE = 4
Move to table declaration
Code quote:
EditDialog.URL = 1
EditDialog.DOWNLOAD_DIRECTORY = 2
EditDialog.LIMIT = 3
EditDialog.CONTENT_SOURCE = 4
|
(A lot of comments are similar in spirit, and this was a quick skim, so I may have missed similar issues ;)). (Haven't looked at the tests). |
|
Is there a special way I should integrate your changes/suggestions? You're using reviewable, which I don't know much about. Should I just make the changes locally and push to my PR? |
|
However you see fit, it's just that reviewing anything more than a screenful in GitHub is a massively painful experience. |
roygbyte
force-pushed
the
add_gazette_plugin
branch
from
ddcad25
to
4dea025
Compare
|
Shit, I think I need help with this. I'm reading back through your comments @NiLuJe, and there's a lot of stuff I don't understand. For instance, you mention "caveats about pinning a ref to a module-local as usual" ;) end
self.view = KeyValuePage:new{But I'm not sure how else I'm supposed to track the view so I can close it later from a separate function. I hope it's not too onerous, but I'd really appreciate some help polishing up this plugin ^_^ For me, I've had no problems using it. But vis a vis your comments, I don't know how to fix it to avoid whatever pitfalls you foresee. |
|
I'm sad, because you have been c̵̯̃o̷̮̊r̸̜͊ṙ̸͍ȕ̷̱p̷̯̐tê̵͜d̶͍̏ :( :) You started well months/years ago, with your crossword plugin, and asking questions and getting our answers. You went with the "one file per class" Java gimmick (or obligation, not sure), so you have >70 files, more than all other plugins combined. This does not really help with reviewing: lots of small files, and lots of boilerplate code. And some Java-like naming, like stuffFactory. Also, to make out for the small files, I see you were unconfortable and you have been cheating by making everything takes more lines :) like many multi-blank-lines, and your multilines which is not really what we are used to everywhere else. And your variable indentation (3 or 4 chars). And this gazettemessages.lua, being the holder of all the strings (like I see it done in Java): it moves the english wording out of the code for no real reason, making the code harder to review: the presence of english strings in the code often serves as documentation/comments about what is happening there. Just thought I had to mention that. (You can take the above as just my verbose way of apologizing for not getting into reviewing this - sorry :) |
|
@poire-z Thank you sooooo much for the feedback! It is true, they are trying to corrupt me! Perhaps they've succeeded? (No---I won't let them! I shall find that little light of Lua, our darling language, and make it shine brighter!) I think your criticisms are well founded, and I would be happy to address/revise. I think compacting the classes, as you say, would be a good start.
As to the other madness you have observed... specifically indentations... I've struggled to get automatic indentations to conform to the KOReader style. I'll have to do it manually. And, yes, remove that weird quirk with the Please believe KOReader is still in my DNA! Don't give up on this one :') :') _, _ |
|
Apologies if this doesn't make too much sense, I'm running on very little sleep tonight, I'll do an editing pass on this answer tomorrow ;). But, to answer your query: remember this answer from yesterday for context, because some of it will be relevant ;). And, to set some more context: using When D is, say, a widget class, for all intents and purposes, you can assume that D is This ensures that whatever If you were to, instead of instantiating a new D object, simply use the empty D shell, shit happens: there's only one D, for starters, so if two different things use it it's ghostbusters time because shit doesn't make sense anymore as you don't know which is which anymore; but, and that pertains to my original point: whatever you put in there stays there. Forever. Soooo, back to your own stuff ;o). Having "utility" 'classes' that are basically just there to store a bunch of functions is perfectly fine (e.g., see both our In your case, either make sure everything is a real class that gets instantiated and doesn't randomly put stuff in the class object itself (e.g., my crappy "const empy shell" analogy), or do some gymnastics to make sure all these references only ever make it to something that is instantiated (e.g., the main plugin instance). I'm not necessarily a fan of this as it probably involves another layer of indirection, possibly via one or more pairs of getters/setters, and would just look like cheaply faked proper classes anyway. (e.g., having a D.createObject return the object but not storing it in D/self, and instead leaving that responsibility to the caller or something). Besides what we talked about on Gitter, your extreme approach to modularity is most probably hurting you in that respect ;). |
|
As a slightly relevant but not a really good example because it's still terrible (also, a lot more things happened around it so it might not be readily visible in the diff), see what happened to It was originally badly implemented and used via the class object itself, instead of using a newly instantiated instance of it every time. |
roygbyte
force-pushed
the
add_gazette_plugin
branch
from
3790e6f
to
170c27b
Compare
|
When "Set download directory": Probably due to some recent change: |
Get rid of things that are not used, don't make sense.
roygbyte
force-pushed
the
add_gazette_plugin
branch
from
1719bf0
to
dd54912
Compare
|
@poire-z If you choose to try this again, the add subscription dialog requires that you "test" the feed before you choose its download directory. Is this a feature or a bug? Neither: it's a question whose answer I haven't yet found. 👍 I would like to make the add subscription flow be nicer, but haven't bothered to set it up with a nice flow. I figure it should be something like:
Probably these would each be their own dialog box, or some fancy full-screen view thing. Do you have any strong opinions or suggestions for what it should be like, or how best to build it? A custom view is enticing: it could be pretty! But there's no reason for it to be a looker... it's just for entering some data once and probably not touching it ever again. |
|
No strong opinion, and neither weak thoughts :/ Again, I'm not your customer for this, just did some test to make this PR alive :) But now that your 2 plugins (gazette and download2epub) are standalone and don't need hack to koreader core, and don't overwrite newsdownloader which will still work, you could just provide some zip files, and ping all the people that reported issues about newsdownloader for them to test and participate in making the UI/UX. |
Yes, it's per feed. Technically, the set directory button is choosing the parent directory, for once the directory is selected a directory is automatically created using the feed's name. This is why the feed needs to be tested first, to get that info. I suppose, I could just omit the feed's name if it hasn't been tested yet.
Gosh, I can't remember where I left off with this feature. I think the library I made could do it? Wait... no... the library could add many articles to one EPUB, but hasn't yet been tested on opening up existing EPUBs and adding to them. But it shouldn't be too hard to do that.
Yeah, did this a few months back when the plugin was in more of a broken state XD S'pose I should do it again. Thx for trying it out! I appreciate it. Nice to have some commentary on what's a good idea or not. Otherwise--watch out--I might revert to styling my code with |
Bug reported by sith-on-mars
Still in progress. But figured I should get this up so people can start to think about what they might like changed.
As of writing, some patches are required to make this run. More info on that in the FR issue..
Unit tests are also written. They're back in my repo.
This change is