The new atom changes look good to me 👍 @mwoz123 I should be able to help finish up the setting part this week, which will be my last nitpick before merging the plugin. Thank you for being patient with my slow review process ;P

Can we merge the first PR (#2592) as first version, and keep working here?

I merged from master, resolved conflicts.
Currently on master there's a bug that disallows to use upper menu, so I was not able to test it after merge.

Btw don't know why the list of commits includes also commits from previous branch.
If you know how to fix it please let me know otherwise we'll have to deal with it, but at least diff looks fine..

Should be fine when squashed, but I personally tend to prefer git pull -r to rebase on top of master.

@mwoz123 the easiest way to fix this is to do a clean checkout of the current koreader master, then cherry-pick your last atom commit: a50c8d7. Then do a force push to your news-atom-support branch.

To be clear, how I'd do that is something along these lines. (If I'm doing something inefficiently or whatever, please tell me. ;-) )

git checkout master
# I've got this set up with a remote called upstream, see http://koreader.rocks/doc/topics/Collaborating.html
# git pull git@github.com:koreader/koreader.git will also work otherwise
git pull upstream master
# make a new, clean branch based on most recent upstream
git checkout -b new_atom_stuff
# now cherry pick the commit into this branch
git cherry-pick a50c8d7
# up to now it was all harmless, but this one's dangerous!
git push -f origin new_atom_stuff:news-atom-support

PS I'd say that usually it's much easier to do git pull -r upstream master instead. This is kind of a special case.

This can be simplified to:

git fetch upstream
git checkout -b upstream/master new_atom_stuff
git cherry-pick a50c8d7
git push -f origin new_atom_stuff:news-atom-support

If your local master branch is not clean, doing a git pull upstream master might cause extra headaches ;)

That's not the same thing. Imo you're wrong even though you're also right. ;p I want to make sure master is up to date anyway.

In the extremely unlikely event that you don't hav3 a clean master you just reset hard or abort rebase (depending), but you always keep master clean. That's what branches are for. In my opinion anyway. :p

nowadays, I try to avoid developing on master branch completely ;) all my local branches are temporary branches that can be deleted anytime. I used to do this houqp-master thing to avoid tainting my local master branch. And later I figured if I only use local master as a clean starting point for other branches, then it's faster for me to just also use upstream/master. That way, i don't need to worry about synchronization with upstream master at all.

That's a very interesting point. I'll have to give it some thought with regard to Git in general vs GitHub-specific Git workflow.

I've decided I don't like git checkout upstream/master -b some_branch even within a GitHub workflow. The reason?

# whoops, first push
$ git push
fatal: The upstream branch of your current branch does not match
the name of your current branch.  To push to the upstream branch
on the remote, use

    git push upstream HEAD:master

To push to the branch of the same name on the remote, use

    git push upstream luacheck

# careful, can't just copy the helpful message like you could if you branched from master!
$ git push origin luacheck

I actually forgot, but that's the reason why I like to stick with where things are going rather than where they're coming from.

Guys there're so many problems with that merge conflicts that I thought maybe we could now use specialized external library for RSS/Atom?

Previously, before creating first PR, I tried to used them but I wasn't able to get my code working.
Maybe I'm too less familiar with Lua:(

RSS/Atom libraries I found:

• feedparser description, download v0.7 link, dependency
• lua-feedparser git page (it's probably the same as above, but newer version)

If I remember right, previously with one of them I wasn't able to import lua/so file, and other one I wasn't able to implement in lua.

@mwoz123 it's less than 50 lines of changes, it should not be that hard to resolve the conflicts. What issues are you running into right now? Have you tried the cherry-pick commands we suggested earlier? It's a good chance for you to learn how to resolve conflicts in git. Once you understand the concept, it would be really easy going forward.

If your code already works, there is no point introducing another heavy dependency for couple lines of code. Unless yours is missing some major features.

Yes I did cherry pick on master & force push. But got confused on some of conflicts in your implementation.

As rss is a standard that is less formal (although I used most frequent rss 2.0 format) it might happen that a Web site will have unsupported by us rss format variation.
I don't know how real is that scenario.

I thought that maybe specialized lib would handle that..

What you think about it?

Hopefully I fixed all review change requests. If I overlooked something please notify me.

Question:
local news_download_dir_name = G_reader_settings:readSetting("news_download_dir_name")
Would that be the right way to make this variable editable on Android?
(as only settings.reader.lua is on writeable android location)

Wouldn't you already have used something like LuaSettings:open(DataStorage:getSettingsDir().."/newsdownloader_settings.lua") previously?

It's probably better if each plugin keeps its settings contained without "polluting" the base program.

Yes, we had all news downloader variables in external setting (lua) file but there was a request to merge it to one main.lua

What should I do then? How you want that to be done?

• Re-externalize all variables or just that one to NewsDownloader config lua?
• put that to config file to settings.reader.lua?
• or other way?

That's doing what I said, not using global settings. :-)

I've found a cool solution to allow editable news folder path.
Please check this commit 611879a

Full solution is ready. Please review.

Not sure this file handling/acquiring function should have its download dir setting in the "reader" defaults.

The settings module can be used by any plugin: http://koreader.rocks/doc/modules/luasettings.html

Like I said above, you can call it using something like LuaSettings:open(DataStorage:getSettingsDir().."/newsdownloader_settings.lua")

So instead of

local custom_news_path = G_reader_settings:readSetting(custom_news_path_var_name)

You do something like

local newsreader_settings = LuaSettings:open(
        DataStorage:getSettingsDir().."/newsdownloader_settings.lua")
local custom_news_path = newsreader_settings:readSetting(custom_news_path_var_name)

That's basically all the global reader settings do, except when something doesn't need to be global it typically shouldn't be for usability, maintenance, as well as performance reasons.

My other main remark is that instead of displaying an info message it would be more user-friendly to call a graphical folder picker. See an example here:

It would be too big change.
"Own download folder" doesn't have to be part of PR Atom support.

I thought it could be simple, extra change, but it turns out it cannot.
So it was removed from this PR (reverted commit).
It will be done in next PR if I found time.

Please re-review.

It basically looks good to me. However, I don't know exactly what @houqp was referring to when he said the following in #2736 (comment):

I should be able to help finish up the setting part this week, which will be my last nitpick before merging the plugin.

Was that already taken care of in the previous PR?

I don't know what was on his mind.
@houqp ?

Sorry if this is a silly question but what does this merge add? (I'm not familiar enough with the code base to follow the commits). As far as I can tell, rss/news feature already esists?

Atom is another (imo mostly better) syndication format.

Sorry for the late reply, I have been really busy in the last couple weeks.

I should be able to help finish up the setting part this week, which will be my last nitpick before merging the plugin.

When I made that comment, the other rss PR hasn't been merged yet. I have since sent a PR to that branch for the setting support. So it's already done in the PR.

Feel free to merge this after my last review is resolved.

Done.

Thanks!