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

Wallabag plugin #4271

Merged
merged 12 commits into from Oct 16, 2018

Conversation

Projects
None yet
4 participants
@y-muller
Contributor

y-muller commented Oct 14, 2018

A plugin to download articles from a Wallabag server, an open source read-it-later service (https://wallabag.org).

Issues:

  • (solved) The settings menu changes the menu item text for the download directory and the tag filter. It makes it easy to see the current configuration. But I have not found how to refresh the value after a change. I could remove the feature but I'd rather have a hint on how to fix it.

  • To skip files that already exist locally, I want to check the dates of the local and remote versions. The only date parser is in the newsdownloader plugin. I have added an ugly hack to use it if available. Thoughts?

Yann Muller and others added some commits Oct 12, 2018

Yann Muller
Wallabag plugin: initial commit.
Archiving on server non-functional.
Missing screen for local settings.
Removed archiving support.
Setting archived status on server doesn't work. Might be a server issue.
Switch to branch 'wallabag-archiving' to work on this feature.
Refresh filemanager after downloads
Password is obscured.
Ugly hack to get parse the time of article.
@poire-z

This comment has been minimized.

Contributor

poire-z commented Oct 14, 2018

About your 1st issue I have not found how to refresh the value after a change :

the callback gets provided the touchmenu_instance as its first argument, so you can do something with it inside the callback depending on what happened (most often: close it, or update/refresh it). See some examples in #4189, eg: https://github.com/koreader/koreader/pull/4189/files#diff-01418c46265dd5757e5be1103a3a224e).
In your case, you want to pass it to Wallabag:setDownloadDirectory(touchmenu_instance), which can call just after self:saveSettings() : touchmenu_instance:updateItems()

@y-muller

This comment has been minimized.

Contributor

y-muller commented Oct 14, 2018

Can I run the CI checks on my own branch?

@y-muller

This comment has been minimized.

Contributor

y-muller commented Oct 14, 2018

the callback gets provided the touchmenu_instance as its first argument, so you can do something with it

Thanks, that worked nicely.

@Frenzie

This comment has been minimized.

Member

Frenzie commented Oct 15, 2018

You can enable CircleCI on your own repo, yes. Let me know if you need a pointer!

@Frenzie

Nice work!

local _ = require("gettext")
local T = FFIUtil.template
local Screen = require("device").screen
local DataStorage = require("datastorage")

This comment has been minimized.

@Frenzie

Frenzie Oct 15, 2018

Member

Please note the order in our coding standards.

This comment has been minimized.

@y-muller

y-muller Oct 15, 2018

Contributor

Done.

end,
},
{
text = _("Remotely delete Finished articles"),

This comment has been minimized.

@Frenzie

Frenzie Oct 15, 2018

Member

Capitalization.

Show resolved Hide resolved plugins/wallabag.koplugin/main.lua
keep_menu_open = true,
callback = function()
UIManager:show(InfoMessage:new{
text = _('Download directory: use a directory that is exclusively used by the Wallabag plugin. ' ..

This comment has been minimized.

@Frenzie

Frenzie Oct 15, 2018

Member

Please use a multiline comment. Our translation string extraction regex will fail on this.

This comment has been minimized.

@y-muller

y-muller Oct 15, 2018

Contributor

Is a max of 79 chars OK or should I split them further?

keep_menu_open = true,
callback = function()
UIManager:show(InfoMessage:new{
text = T(_('Wallabag is an open source Read-it-later service. This plugin synchronises with a Wallabag server.\n\n' ..

This comment has been minimized.

@Frenzie

Frenzie Oct 15, 2018

Member

See above

-- Check if the configuration is complete
local function isempty(s)
return s == nil or s == ''

This comment has been minimized.

@Frenzie

Frenzie Oct 15, 2018

Member

Nitpick, but could you use double quotes?

function Wallabag:getBearerToken()
-- workaround for dateparser, only once
-- the parser is in newsdownloader.koplugin, check if it is available

This comment has been minimized.

@Frenzie

Frenzie Oct 15, 2018

Member

Should it be moved?

This comment has been minimized.

@y-muller

y-muller Oct 15, 2018

Contributor

I moved it to init().

The date parser is really useful. Ideally it would be moved to a place where all plugins can access it.

end
function Wallabag:editServerSettings()
local text_info = "Enter the details of your Wallabag server and account.\n"..

This comment has been minimized.

@Frenzie

Frenzie Oct 15, 2018

Member

Multiline

This comment has been minimized.

@y-muller

y-muller Oct 15, 2018

Contributor

Shortened strings a bit.

This comment has been minimized.

@Frenzie

Frenzie Oct 15, 2018

Member

It's not about length but concat won't work right.

Use either just a single line with \n or [[multiline]]

This comment has been minimized.

@Frenzie

Frenzie Oct 15, 2018

Member

It's not really a defect in the regex imo (or I would've changed it) because [[]] is much nicer anyway.

This comment has been minimized.

@y-muller

y-muller Oct 15, 2018

Contributor

I get it now. I have changed it to multiline.

y-muller added some commits Oct 15, 2018

@Frenzie Frenzie added the enhancement label Oct 15, 2018

end
local num_deleted = self:processLocalFiles( "manual" )
UIManager:show(InfoMessage:new{
text = T(_('Articles processed.\nDeleted: %1'), num_deleted)

This comment has been minimized.

@Frenzie

Frenzie Oct 15, 2018

Member

Could you also make these double quotes for consistency?

else
path = filemanagerutil.abbreviate(self.directory)
end
return _("Set download directory") .. " (" .. path .. ")"

This comment has been minimized.

@Frenzie

Frenzie Oct 15, 2018

Member

Even for stuff like this it's generally preferable to use T(). The thing is, you don't really know if some localization might like to make it (path) Set download directory or Download directory (path) set or many other possibilities. :-)

keep_menu_open = true,
callback = function()
UIManager:show(InfoMessage:new{
text = T(_([[Wallabag is an open source Read-it-later service. This plugin synchronises with a Wallabag server.

This comment has been minimized.

@Frenzie

Frenzie Oct 15, 2018

Member

read-it-later should be lowercase

if isempty(self.server_url) or isempty(self.username) or isempty(self.password) or isempty(self.client_id) or isempty(self.client_secret) or isempty(self.directory) then
UIManager:show(InfoMessage:new{
text = _('Please configure the server and local settings.')

This comment has been minimized.

@Frenzie

Frenzie Oct 15, 2018

Member

quotes

logger.dbg("mode:", dir_mode)
if dir_mode ~= "directory" then
UIManager:show(InfoMessage:new{
text = _('The download directory is not valid.\nPlease configure it in the settings.')

This comment has been minimized.

@Frenzie

Frenzie Oct 15, 2018

Member

quotes

})
return false
end
if string.sub( self.directory, -1 ) ~= '/' then

This comment has been minimized.

@Frenzie

Frenzie Oct 15, 2018

Member

more quotes ;-)

end
local login_url = "/oauth/v2/token"
local body = "{ \"grant_type\": \"password\", \"client_id\": \"" .. self.client_id .. "\", \"client_secret\": \"" .. self.client_secret .. "\", \"username\": \"" .. self.username .. "\", \"password\": \"" .. self.password .. "\"}"

This comment has been minimized.

@Frenzie

Frenzie Oct 15, 2018

Member

Yet perhaps here there's a good reason to deviate and use single quotes? ;-)

This comment has been minimized.

@Frenzie

Frenzie Oct 15, 2018

Member

PS string.format could potentially be more legible.

This comment has been minimized.

@y-muller

y-muller Oct 15, 2018

Contributor

Good shout. A string.format and a [[multiline]] made it a lot neater.

["Accept"] = "application/json, */*",
["Content-Length"] = tostring(#body),
}
local result = self:callAPI( 'POST', login_url, headers, body, "" )

This comment has been minimized.

@Frenzie

Frenzie Oct 15, 2018

Member

I have to wonder actually, is there a specific rationale behind where you used single and where double quotes?

This comment has been minimized.

@y-muller

y-muller Oct 15, 2018

Contributor

I'm afraid there was no specific logic in my use of quotes.
I starting by grabbing a few bits of interesting code in other plugins and after a while the inconsistencies spread a bit.

All changed to double quotes and a few multilines.

function Wallabag:getArticleList()
local articles_url = "/api/entries.json?archive=0&tags=" .. self.filter_tag
return self:callAPI( 'GET', articles_url, nil, "", "" )

This comment has been minimized.

@Frenzie

Frenzie Oct 15, 2018

Member

quotes

function Wallabag:callAPI( method, apiurl, headers, body, filepath )
local request, sink = {}, {}
local parsed = url.parse(apiurl)
request['url'] = self.server_url .. apiurl

This comment has been minimized.

@Frenzie

Frenzie Oct 15, 2018

Member

more quotes

logger.dbg("method ", method)
http.TIMEOUT, https.TIMEOUT = 10, 10
local httpRequest = parsed.scheme == 'http' and http.request or https.request

This comment has been minimized.

@Frenzie

Frenzie Oct 15, 2018

Member

quotes

@Frenzie

These nits should be the last. Earlier I left a couple of quick comments on my phone for code that "smells" at a glance; these were nitpicks on my laptop. ;-)

logger.dbg("deleting article ", path )
local id = self:getArticleID( path )
if id then
self:callAPI( 'DELETE', "/api/entries/" .. id .. ".json", nil, "", "" )

This comment has been minimized.

@Frenzie

Frenzie Oct 15, 2018

Member

quotes

local text_info = [[Enter the details of your Wallabag server and account.
Client ID and client secret are long strings so you might prefer to save the empty settings and edit the config file directly:
.adds/koreader/settings/wallabag.lua

This comment has been minimized.

@Frenzie

Frenzie Oct 15, 2018

Member

This is Kobo-specific (and even then mainly for KSM), plus I suspect you typod settings? You should populate this with DataStorage:getSettingsDir().

function DataStorage:getSettingsDir()

else
filter = self.filter_tag
end
return _("Filter articles by tag") .. " (" .. filter .. ")"

This comment has been minimized.

@Frenzie

Frenzie Oct 15, 2018

Member

Copying my other comment.

Even for stuff like this it's generally preferable to use T().

https://github.com/koreader/koreader/pull/4271/files#r225139189

Review comments.
Double quotes. T(). Multilines.
@Frenzie

This comment has been minimized.

Member

Frenzie commented Oct 15, 2018

Alright, I'll let it sit a couple more days so feel free to ping me then if I haven't looked on GH yet.

@poire-z or just merge it if you think it's ready. ;-)

@poire-z

This comment has been minimized.

Contributor

poire-z commented Oct 16, 2018

Looking fine and clean, so merging.

@poire-z poire-z merged commit 846f8bf into koreader:master Oct 16, 2018

1 check passed

ci/circleci: build Your tests passed on CircleCI!
Details

@y-muller y-muller deleted the y-muller:wallabag branch Oct 17, 2018

@poire-z

This comment has been minimized.

Contributor

poire-z commented Oct 17, 2018

There might be a little something if one has not yet configured this plugin.
Not yet tried on my kobo, but on the emulator:

10/17/18-22:30:49 INFO  FM loaded plugin terminal at plugins/terminal.koplugin
10/17/18-22:30:49 INFO  FM loaded plugin texteditor at plugins/texteditor.koplugin
10/17/18-22:30:49 ERROR Failed to initialize wallabag plugin:  plugins/wallabag.koplugin/main.lua:44: attempt to index field 'wallabag' (a nil value)

The menu is still registered and usable thus.

self.ui.menu:registerToMainMenu(self)
self.wb_settings = self.readSettings()
self.server_url = self.wb_settings.data.wallabag.server_url
self.client_id = self.wb_settings.data.wallabag.client_id

Looks like when there is no settings yet, there is no self.wb_settings.data.wallabag table.
Might just need to be set to ...wallabag={} in Wallabag:readSettings() if it's not there.

@y-muller

This comment has been minimized.

Contributor

y-muller commented Oct 18, 2018

There might be a little something if one has not yet configured this plugin.

Yes, I've reproduced it after deleting the config file. PR to fix it coming up.
Sorry about that.

@bobberb

This comment has been minimized.

bobberb commented Nov 17, 2018

The wallabag plugin provides ALMOST 1:1 parity to the pocket integration on the Kobo. This is amazing. Currently, the functionality is focused on server-side deletion post-read.

To accommodate maybe more use cases, maybe, there should be an option that only favorited/unread articled be allowed. Triggers activate when the un-stared article is finished to make the "mark as read" api call. On next sync the article will be removed from the device but kept on the server. At that point, the device could also accept permanently retained articles via a "device" tag.

@y-muller

This comment has been minimized.

Contributor

y-muller commented Nov 17, 2018

Thanks @bobberb. I have a couple of bugs to fix on the plugin first. Then I'll have a look at your suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment