Skip to content

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

Merged
mwoz123 merged 1 commit intomwoz123:masterfrom
houqp:pr/2592
Apr 3, 2017
Merged

optimzation: avoid downloading xml content to a temp file#3
mwoz123 merged 1 commit intomwoz123:masterfrom
houqp:pr/2592

Conversation

@houqp
Copy link
Copy Markdown

@houqp houqp commented Apr 3, 2017 •
edited
Loading

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 mwoz123 merged commit 6dcde21 into mwoz123:master Apr 3, 2017
mwoz123
mwoz123 reviewed Apr 3, 2017
View reviewed changes
plugins/newsdownloader.koplugin/main.lua
@@ -20,6 +20,35 @@ local FILE_EXTENSION = ".html"
local FEED_SOURCE_SUFFIX = "_rss_tmp.xml"
Copy link
Copy Markdown
Owner

@mwoz123 mwoz123 Apr 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FEED_SOURCE_SUFFIX seems to be unused now.

Copy link
Copy Markdown
Author

@houqp houqp Apr 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch

mwoz123
mwoz123 reviewed Apr 3, 2017
View reviewed changes
plugins/newsdownloader.koplugin/main.lua
if f then
local xmltext = f:read("*a")
f:close()
return deserializeXMLString(xmltext)
Copy link
Copy Markdown
Owner

@mwoz123 mwoz123 Apr 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Author

@houqp houqp Apr 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Copy Markdown
Owner

@mwoz123 mwoz123 Apr 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

@houqp houqp Apr 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mwoz123
Copy link
Copy Markdown
Owner

mwoz123 commented Apr 3, 2017

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

mwoz123
mwoz123 reviewed Apr 3, 2017
View reviewed changes
plugins/newsdownloader.koplugin/main.lua
require("lib/handler")

--Instantiate the object the states the XML file as a Lua table
local xmlhandler = simpleTreeHandler()
Copy link
Copy Markdown
Owner

@mwoz123 mwoz123 Apr 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Author

@houqp houqp Apr 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I mentioned that in your PR ;)

@mwoz123
Copy link
Copy Markdown
Owner

mwoz123 commented Apr 3, 2017

@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? :/

@mwoz123 mwoz123 mentioned this pull request Apr 6, 2017
Merged
@Frenzie
Copy link
Copy Markdown

Frenzie commented Apr 6, 2017

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
Copy link
Copy Markdown
Author

houqp commented Apr 7, 2017

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.

mwoz123 pushed a commit that referenced this pull request Mar 25, 2020
@Frenzie
[Android] kodev run android: show all errors (koreader#4569)
48af471 
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)
```
mwoz123 pushed a commit that referenced this pull request Mar 29, 2020
@Frenzie @mwoz123
[Android] kodev run android: show all errors (koreader#4569)
1ff479f 
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)
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mwoz123 mwoz123 mwoz123 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@houqp @mwoz123 @Frenzie