optimzation: avoid downloading xml content to a temp file #3

houqp · 2017-04-03T06:17:28Z

Sorry for the delay. There is one last TODO item in the source that I would like to cleanup. I will take a look into it before Thursday.

mwoz123 · 2017-04-03T11:02:30Z

plugins/newsdownloader.koplugin/main.lua

@@ -20,6 +20,35 @@ local FILE_EXTENSION = ".html"
 local FEED_SOURCE_SUFFIX = "_rss_tmp.xml"


FEED_SOURCE_SUFFIX seems to be unused now.

mwoz123 · 2017-04-03T20:05:27Z

plugins/newsdownloader.koplugin/main.lua

+    if f then
+        local xmltext = f:read("*a")
+        f:close()
+        return deserializeXMLString(xmltext)


@houqp Did you check if this code works?

I did 'git pull' and got this exception all the time (no matter if it's creating init configuration or already has it):

04/03/17-22:00:45 DEBUG NewsDownloader: File to deserialize: ./news/feeds.xml
./luajit: plugins/newsdownloader.koplugin/main.lua:30: attempt to call global 'deserializeXMLString' (a nil value)
stack traceback:
plugins/newsdownloader.koplugin/main.lua:30: in function 'deserializeXML'
plugins/newsdownloader.koplugin/main.lua:130: in function 'loadConfigAndProcessFeeds'
plugins/newsdownloader.koplugin/main.lua:77: in function 'callback'
frontend/ui/widget/touchmenu.lua:576: in function 'action'
frontend/ui/uimanager.lua:464: in function '_checkTasks'
frontend/ui/uimanager.lua:613: in function 'handleInput'
frontend/ui/uimanager.lua:707: in function 'run'
./reader.lua:188: in main chunk
[C]: at 0x00404750

This is on PC, not tried yet on Android.

My bad, I did not know Lua requires strict ordering for local function definitions!

mwoz123 · 2017-04-03T20:12:36Z

plugins/newsdownloader.koplugin/main.lua

@@ -20,6 +20,35 @@ local FILE_EXTENSION = ".html"
 local FEED_SOURCE_SUFFIX = "_rss_tmp.xml"
 local NEWS_DL_DIR, FEED_CONFIG_PATH



I think it would be good to have here "/news/" extracted (from code below) to a variable
NEWS_DL_DIR = DataStorage:getDataDir() .. "/news/"
so I could be easly found where to change the default dir (e.g. on Android), and not have to search throughout all implementation

e.g.
local DEFAULT_NEWS_DIR_NAME = "/news/"
and
NEWS_DL_DIR = DataStorage:getDataDir() .. DEFAULT_NEWS_DIR_NAME

updated, i will make this a user configurable option later so they can change it through a setting.

mwoz123 · 2017-04-03T20:23:09Z

@houqp - I wrote some comments to your PR please check it.

mwoz123 · 2017-04-03T20:32:09Z

plugins/newsdownloader.koplugin/main.lua

+    require("lib/handler")
+
+    --Instantiate the object the states the XML file as a Lua table
+    local xmlhandler = simpleTreeHandler()


As you'd removed the -- luacheck: ignore from this and 'local xmlparser = xmlParser(xmlhandler)' line

Travis now fails:

plugins/newsdownloader.koplugin/main.lua:151:24: accessing undefined variable 'simpleTreeHandler'
plugins/newsdownloader.koplugin/main.lua:154:23: accessing undefined variable 'xmlParser'

I'm not sure if you're aware of that?

Yes, I mentioned that in your PR ;)

mwoz123 · 2017-04-03T20:36:23Z

@houqp - one more thing - after previous changes this branch is in conflict stage (with master):

Conflicting files
frontend/ui/elements/filemanager_menu_order.lua
frontend/ui/elements/reader_menu_order.lua

Question here: why does the News Downloader plugin must be the last one in the plugins list? :/

Frenzie · 2017-04-06T08:36:09Z

Laziness mostly. Newer plugins tend to get added to the bottom. But with statistics being merged into system statistics and also I'll make a PR for some other minor reorganization (cf. koreader#2562 (comment)) it won't matter in the sense that there won't be a second page anymore. However, it would be good to try to get some logical organization into the tools menu as well.

houqp · 2017-04-07T06:07:21Z

Thanks for the comments, I have added more commits in a new PR. I will try get most of the remaining stuff done during the weekends. We can delay the merge conflict resolution to the end since it will require another git rebase, which will make you having to redo the current branch again (like what happened to my first PR to your repo).

Good news is you can now ship your own plugins without waiting for the official builds. See a new feature introduced through koreader#2693.

It will introduce some minor noise, but too much stuff is simply not seen otherwise, for example: *:E ``` 02-09 12:45:53.359 2580 2609 E IconLoader: Unexpected null component name or activity info: ComponentInfo{org.koreader.launcher/org.koreader.launcher.MainActivity}, null ``` *:F ``` --------- beginning of system --------- beginning of main --------- beginning of crash 02-09 12:47:31.275 4985 5003 F libc : /home/frans/src/kobo/koreader/base/thirdparty/filemq/build/i686-linux-android-debug/filemq-prefix/src/filemq/src/fmq_client.c:170: fmq_client_set_resync: assertion "self" failed 02-09 12:47:31.275 4985 5003 F libc : Fatal signal 6 (SIGABRT), code -6 (SI_TKILL) in tid 5003 (Thread-37), pid 4985 (reader.launcher) 02-09 12:47:31.512 5085 5085 F DEBUG : *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** 02-09 12:47:31.512 5085 5085 F DEBUG : Build fingerprint: 'Android/sdk_phone_x86_64/generic_x86_64:9/PSR1.180720.012/4923214:userdebug/test-keys' 02-09 12:47:31.512 5085 5085 F DEBUG : Revision: '0' 02-09 12:47:31.513 5085 5085 F DEBUG : ABI: 'x86' 02-09 12:47:31.513 5085 5085 F DEBUG : pid: 4985, tid: 5003, name: Thread-37 >>> org.koreader.launcher <<< 02-09 12:47:31.513 5085 5085 F DEBUG : signal 6 (SIGABRT), code -6 (SI_TKILL), fault addr -------- 02-09 12:47:31.513 5085 5085 F DEBUG : Abort message: '/home/frans/src/kobo/koreader/base/thirdparty/filemq/build/i686-linux-android-debug/filemq-prefix/src/filemq/src/fmq_client.c:170: fmq_client_set_resync: assertion "self" failed' 02-09 12:47:31.513 5085 5085 F DEBUG : eax 00000000 ebx 00001379 ecx 0000138b edx 00000006 02-09 12:47:31.513 5085 5085 F DEBUG : edi 00000000 esi 00000000 02-09 12:47:31.513 5085 5085 F DEBUG : ebp 33167a17 esp de3e9528 eip f70d8b59 02-09 12:47:31.515 5085 5085 F DEBUG : 02-09 12:47:31.515 5085 5085 F DEBUG : backtrace: 02-09 12:47:31.515 5085 5085 F DEBUG : #00 pc 00000b59 [vdso:f70d8000] (__kernel_vsyscall+9) 02-09 12:47:31.515 5085 5085 F DEBUG : #1 pc 0001fc08 /system/lib/libc.so (syscall+40) 02-09 12:47:31.515 5085 5085 F DEBUG : #2 pc 000321f3 /system/lib/libc.so (abort+115) 02-09 12:47:31.515 5085 5085 F DEBUG : #3 pc 00032681 /system/lib/libc.so (__assert2+49) 02-09 12:47:31.515 5085 5085 F DEBUG : #4 pc 000046b0 /data/data/org.koreader.launcher/files/libs/libfmq.so.1 02-09 12:47:31.515 5085 5085 F DEBUG : #5 pc 00008e54 /data/app/org.koreader.launcher-u3tYrWEK7wgvoJGvd1pTrA==/lib/x86/libluajit.so 02-09 12:47:31.515 5085 5085 F DEBUG : #6 pc 00056a61 /data/app/org.koreader.launcher-u3tYrWEK7wgvoJGvd1pTrA==/lib/x86/libluajit.so 02-09 12:47:31.515 5085 5085 F DEBUG : #7 pc 0006ed92 /data/app/org.koreader.launcher-u3tYrWEK7wgvoJGvd1pTrA==/lib/x86/libluajit.so 02-09 12:47:31.515 5085 5085 F DEBUG : #8 pc 00006fc9 /data/app/org.koreader.launcher-u3tYrWEK7wgvoJGvd1pTrA==/lib/x86/libluajit.so 02-09 12:47:31.515 5085 5085 F DEBUG : #9 pc 00061f73 /data/app/org.koreader.launcher-u3tYrWEK7wgvoJGvd1pTrA==/lib/x86/libluajit.so 02-09 12:47:31.515 5085 5085 F DEBUG : #10 pc 00006fc9 /data/app/org.koreader.launcher-u3tYrWEK7wgvoJGvd1pTrA==/lib/x86/libluajit.so 02-09 12:47:31.515 5085 5085 F DEBUG : #11 pc 00061f73 /data/app/org.koreader.launcher-u3tYrWEK7wgvoJGvd1pTrA==/lib/x86/libluajit.so 02-09 12:47:31.515 5085 5085 F DEBUG : #12 pc 00006fc9 /data/app/org.koreader.launcher-u3tYrWEK7wgvoJGvd1pTrA==/lib/x86/libluajit.so 02-09 12:47:31.515 5085 5085 F DEBUG : #13 pc 0001c470 /data/app/org.koreader.launcher-u3tYrWEK7wgvoJGvd1pTrA==/lib/x86/libluajit.so (lua_pcall+80) 02-09 12:47:31.515 5085 5085 F DEBUG : #14 pc 0000571a /data/app/org.koreader.launcher-u3tYrWEK7wgvoJGvd1pTrA==/lib/x86/libluajit.so (android_main+458) 02-09 12:47:31.515 5085 5085 F DEBUG : koreader#15 pc 00070a38 /data/app/org.koreader.launcher-u3tYrWEK7wgvoJGvd1pTrA==/lib/x86/libluajit.so 02-09 12:47:31.515 5085 5085 F DEBUG : koreader#16 pc 0009cce5 /system/lib/libc.so (__pthread_start(void*)+53) 02-09 12:47:31.515 5085 5085 F DEBUG : koreader#17 pc 00033c1b /system/lib/libc.so (__start_thread+75) ```

optimzation: avoid downloading xml content to a temp file

c187579

mwoz123 approved these changes Apr 3, 2017

View reviewed changes

mwoz123 merged commit 6dcde21 into mwoz123:master Apr 3, 2017

mwoz123 reviewed Apr 3, 2017

View reviewed changes

mwoz123 mentioned this pull request Apr 6, 2017

Simple News (RSS/Atom) downloader plugin koreader/koreader#2592

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

optimzation: avoid downloading xml content to a temp file #3

optimzation: avoid downloading xml content to a temp file #3

houqp commented Apr 3, 2017 •

edited

Loading

mwoz123 Apr 3, 2017

houqp Apr 7, 2017

mwoz123 Apr 3, 2017

houqp Apr 7, 2017

mwoz123 Apr 3, 2017

houqp Apr 7, 2017

mwoz123 commented Apr 3, 2017

mwoz123 Apr 3, 2017

houqp Apr 7, 2017

mwoz123 commented Apr 3, 2017

Frenzie commented Apr 6, 2017

houqp commented Apr 7, 2017

		@@ -20,6 +20,35 @@ local FILE_EXTENSION = ".html"
		local FEED_SOURCE_SUFFIX = "_rss_tmp.xml"

		@@ -20,6 +20,35 @@ local FILE_EXTENSION = ".html"
		local FEED_SOURCE_SUFFIX = "_rss_tmp.xml"
		local NEWS_DL_DIR, FEED_CONFIG_PATH

optimzation: avoid downloading xml content to a temp file #3

optimzation: avoid downloading xml content to a temp file #3

Conversation

houqp commented Apr 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwoz123 commented Apr 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwoz123 commented Apr 3, 2017

Frenzie commented Apr 6, 2017

houqp commented Apr 7, 2017

houqp commented Apr 3, 2017 •

edited

Loading