5924 nd force skip imasges dl by mwoz123 · Pull Request #6003 · koreader/koreader

mwoz123 · 2020-03-25T20:50:01Z

Solves: #5924

This change is

Frenzie · 2020-03-25T20:51:23Z

plugins/newsdownloader.koplugin/main.lua

 local file_extension = ".epub"
 local news_download_dir_name = "news"
 local news_download_dir_path, feed_config_path
+local global_exlude_images_config_name = 'global_exlude_images_config_name';


Suggested change

local global_exlude_images_config_name = 'global_exlude_images_config_name';

local global_exlude_images_config_name = "global_exlude_images_config_name"

Frenzie · 2020-03-25T20:52:42Z

plugins/newsdownloader.koplugin/main.lua

                callback = function() self:removeNewsButKeepFeedConfig() end,
            },
+            {
+                text = _("Global skip images download"),


If you reverse the logic you could just call it:

Suggested change

text = _("Global skip images download"),

text = _("Download images"),

Or maybe:

Suggested change

text = _("Global skip images download"),

text = _("Download images (default)"),

Frenzie · 2020-03-25T20:53:53Z

plugins/newsdownloader.koplugin/main.lua

 local news_download_dir_name = "news"
 local news_download_dir_path, feed_config_path
-local global_exlude_images_config_name = 'global_exlude_images_config_name';
+local global_skip_images_download_config_name = 'global_skip_images_download_config_name';


Suggested change

local global_skip_images_download_config_name = 'global_skip_images_download_config_name';

local global_skip_images_download_config_name = "global_skip_images_download_config_name"

Frenzie · 2020-03-25T20:54:51Z

plugins/newsdownloader.koplugin/main.lua

                callback = function()
+                    local news_downloader_settings = LuaSettings:open(("%s/%s"):format(DataStorage:getSettingsDir(), news_downloader_config_file))
                    local skip = news_downloader_settings:readSetting(global_skip_images_download_config_name) or false;
+                    logger.warn('NewsDownloader: global_skip_images_download', skip)


Suggested change

logger.warn('NewsDownloader: global_skip_images_download', skip)

logger.debug("NewsDownloader: global_skip_images_download", skip)

Frenzie · 2020-03-25T20:55:11Z

plugins/newsdownloader.koplugin/main.lua

 local news_download_dir_name = "news"
 local news_download_dir_path, feed_config_path
-local global_skip_images_download_config_name = 'global_skip_images_download_config_name';
+local force_skip_images_download_config_name = 'force_skip_images_download_config_name';


Suggested change

local force_skip_images_download_config_name = 'force_skip_images_download_config_name';

local force_skip_images_download_config_name = "force_skip_images_download_config_name"

Frenzie · 2020-03-25T20:55:52Z

plugins/newsdownloader.koplugin/main.lua

            },
            {
-                text = _("Force skip images download toggle"),
+                text = _("Toogle: force skip images download"),


Typo, but I'm not entirely sure what the text means. I suspect that "toggle" shouldn't be there since it'd already be visually in the checkbox.

Suggested change

text = _("Toogle: force skip images download"),

text = _("Toggle: force skip images download"),

Frenzie

almost there ^_^

Frenzie · 2020-03-25T21:47:55Z

plugins/newsdownloader.koplugin/main.lua

+                callback = function()
+                    local news_downloader_settings = LuaSettings:open(("%s/%s"):format(DataStorage:getSettingsDir(), news_downloader_config_file))
+                    local skip = news_downloader_settings:readSetting(force_skip_images_download_config_name) or false;
+                    logger.warn('NewsDownloader: previous force_skip_images_download: ', skip)


Suggested change

logger.warn('NewsDownloader: previous force_skip_images_download: ', skip)

logger.info("NewsDownloader: previous force_skip_images_download: ", skip)

But that whole never/don't stuff isn't necessary and harder to parse, better yet is simply reversed logic:

The hole idea was to make it a quick way to sometimes disable images (e.g. slow wifi, no time for downloading all data, use smaller amount of wifi in case of being currently connected to limited transfer wifi etc).

I didn't want to make it a replacement for" include_images " which can be set for each rss/atom source independly.

And the above screenshot looks this way for me that 's why I wanted to mark it as "skip" / "temporally" etc..

Btw I like the idea with checkbox (and would like to have it) but remembered that moving "lua Settings open" to lazy load was because there was a need/request to speed up menu.
That's why I didn't use it..

Maybe you're thinking of something wifi-related? It already says self:lazyInitialization() in addToMainMenu so regardless how slow that may or may not be, the cost is incurred.

koreader/plugins/newsdownloader.koplugin/main.lua

Lines 72 to 73 in 0b89862

function NewsDownloader:addToMainMenu(menu_items)

self:lazyInitialization()

Ok, how about checkbox + name that indicates enabling won't cause automatically downloading images from all sources (as it still has to be enabled in feed_config.lua)?

I have no problem with Never download images.

Ok checkbox + "never download images " is much safer will do so.

BTW I'm not familiar with recent git changes.
Although I haven't touch the majority of the PR request changes they are marked as 'outdated'.
Do I need to change them or you somehow patched it?

No, that just means I thought you submitted a PR with 4 distinct commits which I mistakenly reviewed independently.

Frenzie · 2020-03-25T21:49:16Z

plugins/newsdownloader.koplugin/main.lua

                callback = function() self:removeNewsButKeepFeedConfig() end,
            },
+            {
+                text = _("Force skip images download"),


@NiLuJe Do you have any thoughts? I'm not overly fond of this string. (Setting aside that Force skip image download would be the more idiomatic form of it.)

Never download images?

Probably applies to a bunch of similar strings up the chain, then ;).

(and/or variable names if someone wants to go the extra mile and it makes sense re: the rest of the code).

Yes, my thoughts exactly (also see here because I mistakenly thought the commits were distinct units). :-)

Suggested change

text = _("Force skip images download"),

text = _("Never download images"),

And yes, the variable name would also be much clearer as never_download_images

Guys IMHO "never" means something that will burn down function (or even hardware? (if I would have been non-tech) :p) responsible for downloading images.
I would NEVER click that (if I didn't write that function):p
I'm serious.
"never" for me means "forever"

This is "temporarily" quick toggle disable you can quickly reverse it and its gets back to normal downloading images (if set so in config file) not forever!

Please rethink once again you request

How about " temporary skip images"?

That's because I overlooked that you made it scary, and it also explains the weird popup. ;-)

Should be like this instead:

diff --git a/plugins/newsdownloader.koplugin/main.lua b/plugins/newsdownloader.koplugin/main.lua index 5f3921b1..debbaa64 100644 --- a/plugins/newsdownloader.koplugin/main.lua +++ b/plugins/newsdownloader.koplugin/main.lua @@ -107,18 +107,20 @@ function NewsDownloader:addToMainMenu(menu_items) callback = function() self:removeNewsButKeepFeedConfig() end, }, { - text = _("Force skip images download"), + text = _("Never download images"), keep_menu_open = true, + checked_func = function() + -- Not sure this should all be quite this local all over the file ;-) + local news_downloader_settings = LuaSettings:open(("%s/%s"):format(DataStorage:getSettingsDir(), news_downloader_config_file)) + local skip = news_downloader_settings:readSetting(force_skip_images_download_config_name) + return skip + end, callback = function() local news_downloader_settings = LuaSettings:open(("%s/%s"):format(DataStorage:getSettingsDir(), news_downloader_config_file)) local skip = news_downloader_settings:readSetting(force_skip_images_download_config_name) or false; logger.warn('NewsDownloader: previous force_skip_images_download: ', skip) news_downloader_settings:saveSetting(force_skip_images_download_config_name, not skip) news_downloader_settings:flush() - UIManager:show(InfoMessage:new{ - text = T(_("Force skip images download set to %1"), - string.format("%s", not skip)) - }) end, }, {

Frenzie · 2020-03-26T07:59:59Z

plugins/newsdownloader.koplugin/main.lua

+                keep_menu_open = true,
+                callback = function()
+                    local news_downloader_settings = LuaSettings:open(("%s/%s"):format(DataStorage:getSettingsDir(), news_downloader_config_file))
+                    local skip = news_downloader_settings:readSetting(force_skip_images_download_config_name) or false;


Suggested change

local skip = news_downloader_settings:readSetting(force_skip_images_download_config_name) or false;

local skip = news_downloader_settings:readSetting(force_skip_images_download_config_name) or false

mwoz123 · 2020-03-26T20:32:36Z

changes done.

Frenzie · 2020-03-26T20:42:39Z

plugins/newsdownloader.koplugin/main.lua

+                text = _("Never download images"),
+                keep_menu_open = true,
+                checked_func = function()
+                    local news_downloader_settings = LuaSettings:open(("%s/%s"):format(DataStorage:getSettingsDir(), news_downloader_config_file))


To be clear, I meant that the cost wouldn't be incurred if you didn't keep the settings excessively local. In this case there is an extra cost — not one that would be noticeable of course, but surely it should be more measurable than whatever profit you could possibly get out of that never_download_images_config_name thing.

I mean something like this:

diff --git a/plugins/newsdownloader.koplugin/main.lua b/plugins/newsdownloader.koplugin/main.lua index 0a141c15..7695fcd3 100644 --- a/plugins/newsdownloader.koplugin/main.lua +++ b/plugins/newsdownloader.koplugin/main.lua @@ -25,6 +25,7 @@ local initialized = false local wifi_enabled_before_action = true local feed_config_file_name = "feed_config.lua" local news_downloader_config_file = "news_downloader_settings.lua" +local news_downloader_settings local config_key_custom_dl_dir = "custom_dl_dir" local file_extension = ".epub" local news_download_dir_name = "news" @@ -110,11 +111,9 @@ function NewsDownloader:addToMainMenu(menu_items) text = _("Never download images"), keep_menu_open = true, checked_func = function() - local news_downloader_settings = LuaSettings:open(("%s/%s"):format(DataStorage:getSettingsDir(), news_downloader_config_file)) return news_downloader_settings:readSetting(never_download_images_config_name) end, callback = function() - local news_downloader_settings = LuaSettings:open(("%s/%s"):format(DataStorage:getSettingsDir(), news_downloader_config_file)) local never_download_images = news_downloader_settings:readSetting(never_download_images_config_name) or false logger.info("NewsDownloader: previous never_download_images: ", never_download_images) news_downloader_settings:saveSetting(never_download_images_config_name, not never_download_images) @@ -153,7 +152,7 @@ end function NewsDownloader:lazyInitialization() if not initialized then logger.dbg("NewsDownloader: obtaining news folder") - local news_downloader_settings = LuaSettings:open(("%s/%s"):format(DataStorage:getSettingsDir(), news_downloader_config_file)) + news_downloader_settings = LuaSettings:open(("%s/%s"):format(DataStorage:getSettingsDir(), news_downloader_config_file)) if news_downloader_settings:has(config_key_custom_dl_dir) then news_download_dir_path = news_downloader_settings:readSetting(config_key_custom_dl_dir) else @@ -191,7 +190,6 @@ function NewsDownloader:loadConfigAndProcessFeeds() return end - local news_downloader_settings = LuaSettings:open(("%s/%s"):format(DataStorage:getSettingsDir(), news_downloader_config_file)) local never_download_images = news_downloader_settings:readSetting(never_download_images_config_name) or false local unsupported_feeds_urls = {} @@ -409,7 +407,6 @@ function NewsDownloader:setCustomDownloadDirectory() require("ui/downloadmgr"):new{ onConfirm = function(path) logger.dbg("NewsDownloader: set download directory to: ", path) - local news_downloader_settings = LuaSettings:open(("%s/%s"):format(DataStorage:getSettingsDir(), news_downloader_config_file)) news_downloader_settings:saveSetting(config_key_custom_dl_dir, ("%s/"):format(path)) news_downloader_settings:flush()

More for ease of reading than performance really, that local news_downloader_settings = LuaSettings:open(("%s/%s"):format(DataStorage:getSettingsDir(), news_downloader_config_file)) is quite distracting.

Frenzie · 2020-03-26T20:47:13Z

plugins/newsdownloader.koplugin/main.lua

 local file_extension = ".epub"
 local news_download_dir_name = "news"
 local news_download_dir_path, feed_config_path
+local never_download_images_config_name = "never_download_images_config_name"


I find this unnecessarily complex. I don't follow the reasoning behind not to just using readSetting("never_download_images"), but setting that aside:

Suggested change

local never_download_images_config_name = "never_download_images_config_name"

local never_download_images_config_name = "never_download_images"

mwoz123 · 2020-03-27T12:21:04Z

done

Frenzie

Thanks!

Closes koreader#5924.

mwoz123 added 4 commits March 25, 2020 20:58

feat(5924): nd skip download images

4dad0cc

feat(5924): rename var

2bd8312

fix(5924): ND error in saving images toggle

8036131

fix(5924): error at obtaining ND config

2480467

Frenzie added the Plugin label Mar 25, 2020

Frenzie reviewed Mar 25, 2020

View reviewed changes

Frenzie added this to the 2020.04 milestone Mar 25, 2020

Code review changes

2f49644

Frenzie reviewed Mar 25, 2020

View reviewed changes

Frenzie reviewed Mar 26, 2020

View reviewed changes

mwoz123 and others added 2 commits March 26, 2020 20:12

Chore: code review changes

8f0872f

chore: nd update var names

1d0b933

Frenzie reviewed Mar 26, 2020

View reviewed changes

mwoz123 added 2 commits March 27, 2020 12:52

refactor: code review changes

e0004ad

replace variable with strings (code review changes)

39e47d6

Frenzie approved these changes Mar 27, 2020

View reviewed changes

Frenzie merged commit 904a610 into koreader:master Mar 27, 2020

mwoz123 added a commit to mwoz123/koreader that referenced this pull request Mar 29, 2020

plugins/NewsDownloader: add never download images (koreader#6003)

3fd18e3

Closes koreader#5924.

	local global_exlude_images_config_name = 'global_exlude_images_config_name';
	local global_exlude_images_config_name = "global_exlude_images_config_name"

	text = _("Global skip images download"),
	text = _("Download images"),

	text = _("Global skip images download"),
	text = _("Download images (default)"),

	local global_skip_images_download_config_name = 'global_skip_images_download_config_name';
	local global_skip_images_download_config_name = "global_skip_images_download_config_name"

	logger.warn('NewsDownloader: global_skip_images_download', skip)
	logger.debug("NewsDownloader: global_skip_images_download", skip)

	local force_skip_images_download_config_name = 'force_skip_images_download_config_name';
	local force_skip_images_download_config_name = "force_skip_images_download_config_name"

	text = _("Toogle: force skip images download"),
	text = _("Toggle: force skip images download"),

	logger.warn('NewsDownloader: previous force_skip_images_download: ', skip)
	logger.info("NewsDownloader: previous force_skip_images_download: ", skip)

	function NewsDownloader:addToMainMenu(menu_items)
	self:lazyInitialization()

	text = _("Force skip images download"),
	text = _("Never download images"),

	local skip = news_downloader_settings:readSetting(force_skip_images_download_config_name) or false;
	local skip = news_downloader_settings:readSetting(force_skip_images_download_config_name) or false

	local never_download_images_config_name = "never_download_images_config_name"
	local never_download_images_config_name = "never_download_images"

Uh oh!

Conversation

mwoz123 commented Mar 25, 2020 • edited by Frenzie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Frenzie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwoz123 Mar 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwoz123 Mar 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwoz123 commented Mar 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwoz123 commented Mar 27, 2020

Uh oh!

Frenzie left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mwoz123 commented Mar 25, 2020 •

edited by Frenzie

Loading

mwoz123 Mar 26, 2020 •

edited

Loading

mwoz123 Mar 26, 2020 •

edited

Loading