Skip to content
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

Send2Ebook (receiver) plugin #3681

Merged
merged 27 commits into from Feb 22, 2018

Conversation

Projects
None yet
3 participants
@mwoz123
Copy link
Contributor

mwoz123 commented Feb 15, 2018

Decription

Send2Ebook lets you send articles found on PC/Android phone to your Ebook reader.

How it works

Open your faviourite Android web browser, then Share via, choose Send2Ebook.
On your Ebook reader download articles

links

https://github.com/koreader/android-send2ebook
core lib (console UI):
https://github.com/mwoz123/send2ebook

@Frenzie
Copy link
Member

Frenzie left a comment

It's almost midnight so I won't look at this today, so for now just a few quick minor stylistic comments that jumped out at me while skimming.

Also looks good while skimming overall. :-)

local url = string.gsub(address, "://", replace)
return url
end

This comment has been minimized.

@Frenzie

Frenzie Feb 15, 2018

Member

Stray newline


--hint: host needs to start with 'ftp://' prefix otherwise won't work

host='ftp://server.com',

This comment has been minimized.

@Frenzie
local default_download_dir_name = "Send2Ebook"
local download_dir_path

local function stringEnds(String,End)

This comment has been minimized.

@Frenzie

Frenzie Feb 15, 2018

Member

variables should be lowercase

local download_dir_path

local function stringEnds(String,End)
return End=='' or string.sub(String,-string.len(End))==End

This comment has been minimized.

@Frenzie

Frenzie Feb 15, 2018

Member

Please use ""

return End=='' or string.sub(String,-string.len(End))==End
end

function Send2Ebook:downloadFileAndRemove(connectionUrl, remotePath, localDownloadPath)

This comment has been minimized.

@Frenzie

Frenzie Feb 15, 2018

Member

connection_url, remote_path and local_download_path

end

function Send2Ebook:process()
local info = InfoMessage:new{ text = _("Connecting ...") }

This comment has been minimized.

@Frenzie

Frenzie Feb 15, 2018

Member

Connecting…

@mwoz123

This comment has been minimized.

Copy link
Contributor Author

mwoz123 commented Feb 16, 2018

Thanks. Will fix them later.

One question :
Is there a method to check if ebook has been opened? Is it :DocSettings:getSidecarFile(file_abs_path)?

As I used this plugin for a while and I think it makes more sense to "delete only those which were opened" instead 'delete all'.
Many times I had plenty of unreaded z (and plenty of readed) and would like to clean the mess before downloading new

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 16, 2018

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented on plugins/send2ebook.koplugin/main.lua in 6ecd191 Feb 16, 2018

Why not simply os.remove?

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented on plugins/send2ebook.koplugin/main.lua in 6ecd191 Feb 16, 2018

Does this need to be an anonymous function of would it work as self.removeReadArticles?

This comment has been minimized.

Copy link
Contributor

mwoz123 replied Feb 17, 2018

without this wrap function, it's started right after top menu is opened. You don't have to even choose "Remove read articles", so that's wrong.

This comment has been minimized.

Copy link
Member

Frenzie replied Feb 18, 2018

I'm not saying self.blabla() (execution) but self.blabla (reference).

mwoz123 added some commits Feb 16, 2018

@mwoz123

This comment has been minimized.

Copy link
Contributor Author

mwoz123 commented Feb 16, 2018

Why not simply os.remove?

I refactored it a bit. and os.remove doesn't work for me.

Is there a simply way to create editable configuration UI for entering:

  • host, username, password, folder

or it has to be tons of code like in cloudstorage?

Maybe if it not too complicated I'll be able to replace the manual edit ?

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 16, 2018

What tons of code are you referring to? It looks pretty minimal to me.

function Ftp:config(item, callback)
local text_info = "FTP address must be in the format ftp://example.domian.com\n"..
"Also supported is format with IP e.g: ftp://10.10.10.1\n"..
"Username and password are optional."
local hint_name = _("Your FTP name")
local text_name = ""
local hint_address = _("FTP address eg ftp://example.com")
local text_address = ""
local hint_username = _("FTP username")
local text_username = ""
local hint_password = _("FTP password")
local text_password = ""
local title
local text_button_right = _("Add")
if item then
title = _("Edit FTP account")
text_button_right = _("Apply")
text_name = item.text
text_address = item.address
text_username = item.username
text_password = item.password
else
title = _("Add FTP account")
end
self.settings_dialog = MultiInputDialog:new {
title = title,
fields = {
{
text = text_name,
input_type = "string",
hint = hint_name ,
},
{
text = text_address,
input_type = "string",
hint = hint_address ,
},
{
text = text_username,
input_type = "string",
hint = hint_username,
},
{
text = text_password,
input_type = "string",
hint = hint_password,
},
},
buttons = {
{
{
text = _("Cancel"),
callback = function()
self.settings_dialog:onClose()
UIManager:close(self.settings_dialog)
end
},
{
text = _("Info"),
callback = function()
UIManager:show(InfoMessage:new{ text = text_info })
end
},
{
text = text_button_right,
callback = function()
local fields = MultiInputDialog:getFields()
if fields[1] ~= "" and fields[2] ~= "" then
if item then
-- edit
callback(item, fields)
else
-- add new
callback(fields)
end
self.settings_dialog:onClose()
UIManager:close(self.settings_dialog)
else
UIManager:show(InfoMessage:new{
text = _("Please fill in all fields.")
})
end
end
},
},
},
width = Screen:getWidth() * 0.95,
height = Screen:getHeight() * 0.2,
input_type = "text",
}
self.settings_dialog:onShowKeyboard()
UIManager:show(self.settings_dialog)
end

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 17, 2018

Codecov Report

Merging #3681 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3681      +/-   ##
==========================================
- Coverage    52.4%   52.36%   -0.05%     
==========================================
  Files         206      206              
  Lines       27425    27409      -16     
==========================================
- Hits        14373    14352      -21     
- Misses      13052    13057       +5
Impacted Files Coverage Δ
frontend/optmath.lua 48.27% <0%> (-51.73%) ⬇️
frontend/ui/widget/inputtext.lua 58.82% <0%> (+1.83%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 720fd5d...12f6cff. Read the comment docs.

mwoz123 added some commits Feb 17, 2018

@mwoz123

This comment has been minimized.

Copy link
Contributor Author

mwoz123 commented Feb 17, 2018

What tons of code are you referring to? It looks pretty minimal to me.

code looked complicated. But I manage to code it.

mwoz123 and others added some commits Feb 17, 2018

mwoz123
mwoz123
@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 18, 2018

code looked complicated.

UI is always complicated, although I think QML is more elegant than most attempts at making it easier to work with. But I don't think it's really any better or worse than Gtk, Tcl/Tk, Java's Swing and its successor JavaFX, etc. :-)

for entry in lfs.dir(download_dir_path) do
if entry ~= "." and entry ~= ".." then
local entry_path = download_dir_path .. entry
if DocSettings:hasSidecarFile(entry_path) then

This comment has been minimized.

@mwoz123

mwoz123 Feb 18, 2018

Author Contributor

Does this pr https://github.com/koreader/koreader/pull/3675/files
Make any influence to above check? Cause after merge master it doesn't seem to work correctly (on Inkbook), which it did on emulator (and old master)

This comment has been minimized.

@mwoz123

mwoz123 Feb 18, 2018

Author Contributor

Maybe I should use something else here?

This comment has been minimized.

@Frenzie

Frenzie Feb 22, 2018

Member

No, it's the same way files are displayed read or unread in the filemanager. What are the steps?

Edit: sorry, ignore this one.

This comment has been minimized.

@mwoz123

mwoz123 Feb 22, 2018

Author Contributor

It seems to be working fine on emulator, after todays master merge

@mwoz123

This comment has been minimized.

Copy link
Contributor Author

mwoz123 commented Feb 22, 2018

any comments? merge?

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 22, 2018

Oops, I had a pending comment on the question that I forgot to submit. But that seems to be no longer an issue.

No, it's the same way files are displayed read or unread in the filemanager. What are the steps?

I'll take a look. It should be ready or just one or two dotted is from it.

Frenzie added some commits Feb 22, 2018

@Frenzie
Copy link
Member

Frenzie left a comment

I think that just leaves a couple of naming conventions.

local fileTable = FtpApi:listFolder(connection_url .. FtpConnectionConfig.folder, FtpConnectionConfig.folder) --args looks strange but otherwise resonse with invalid paths

if not fileTable then
info = InfoMessage:new{ text = T(_("Could not get file list for server: %1, user: %2, folder: %3"), FtpConnectionConfig.host, FtpConnectionConfig.user, FtpConnectionConfig.folder) }

This comment has been minimized.

@Frenzie

Frenzie Feb 22, 2018

Member

indentation

for entry in lfs.dir(download_dir_path) do
if entry ~= "." and entry ~= ".." then
local entry_path = download_dir_path .. entry
if DocSettings:hasSidecarFile(entry_path) then

This comment has been minimized.

@Frenzie

Frenzie Feb 22, 2018

Member

No, it's the same way files are displayed read or unread in the filemanager. What are the steps?

Edit: sorry, ignore this one.

UIManager:close(info)

local count = 1
local ftp_config = send2ebook_settings:readSetting("ftp_config") or {address='ftp://Please_setup_ftp_settings_first;)', username="", password="", folder=""}

This comment has been minimized.

@Frenzie

Frenzie Feb 22, 2018

Member

Is ftp://Please_setup_ftp_settings_first;) a typo of some sort? (Particularly that ;))

Also double quotes would be nice. ;-)


local connection_url = FtpApi:generateUrl(ftp_config.address, ftp_config.username, ftp_config.password)

local fileTable = FtpApi:listFolder(connection_url .. ftp_config.folder, ftp_config.folder) --args looks strange but otherwise resonse with invalid paths

This comment has been minimized.

@Frenzie

Frenzie Feb 22, 2018

Member

It's not a function.

This comment has been minimized.

@mwoz123

mwoz123 Feb 22, 2018

Author Contributor

you mean it should be with underscore? file_table?

local total_entries = table.getn(fileTable)
logger.dbg("total_entries ", total_entries)
if total_entries > 1 then total_entries = total_entries -2 end --remove result "../" (upper folder) and "./" (current folder)
for idx, ftpFile in ipairs(fileTable) do

This comment has been minimized.

@Frenzie

Frenzie Feb 22, 2018

Member

ftp_file is also not a function

@Frenzie Frenzie merged commit 25a67a2 into koreader:master Feb 22, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 22, 2018

Thanks!

@mwoz123 mwoz123 deleted the mwoz123:send-to-koreader-receiver branch Jul 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.