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

WebDav CloudStorage #4272

Merged
merged 12 commits into from
Oct 17, 2018
Merged

WebDav CloudStorage #4272

merged 12 commits into from
Oct 17, 2018

Conversation

y-muller
Copy link
Contributor

Addition of WebDav to the CloudStorage. The functionality is the same as with Dropbox and FTP, it is possible to browse the files on the server and download a copy.

Tested with:

  • NextCloud
  • HubZilla

Addition of WebDav to the CloudStorage. The functionality is the same as with Dropbox and FTP, it is possible to browse the files on the server and download a copy.

Tested with:
NextCloud
HubZilla
Copy link
Member

@Frenzie Frenzie left a comment

Choose a reason for hiding this comment

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

No real comments, basically lgtm. 👍

@@ -88,6 +89,15 @@ function CloudStorage:selectCloudType()
end,
},
},
{
{
text = _("WebDav"),
Copy link
Member

Choose a reason for hiding this comment

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

WebDav or WebDAV?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to WebDAV

Copy link
Member

@Frenzie Frenzie Oct 17, 2018

Choose a reason for hiding this comment

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

I think you may have changed them all except this one? ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot to push that one.

end)
UIManager:close(self.download_dialog)
UIManager:show(InfoMessage:new{
text = _("Downloading may take several minutes…"),
Copy link
Member

Choose a reason for hiding this comment

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

Should we make this a local variable a little further up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe text is a member of the Button.

Copy link
Member

Choose a reason for hiding this comment

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

I mean the string that's copied 10 times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes. Might take the opportunity to make it something nicer, "several minutes" seems a bit pessimistic for the type of files useful on an ebook reader.

Please wait while your file is downloading.
Download in progress.
?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, although at least for my ereader and file use it hardly seems pessimistic. (I tend to go for USB most of the time, and SSH otherwise.) My H2O doesn't come anywhere near the vicinity of the speed of my internet/wifi, regardless whether it's SD card speed/interface bound, wifi-chip bound or CPU bound. But my phone would indeed download a small 50 MB file in under 10 seconds.

I still think a little "don't panic" message might not be a bad idea though.

Downloading. This might take a moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it.

The WiFi on the Clara is not amazing but too bad really. It didn't time exactly but a 5MB PDF took around 5s. I'm charging the old Kobo Mini to see if it much worse.

frontend/apps/cloudstorage/webdav.lua Outdated Show resolved Hide resolved
frontend/apps/cloudstorage/webdav.lua Outdated Show resolved Hide resolved
frontend/apps/cloudstorage/webdav.lua Show resolved Hide resolved
local hint_folder = _("Start folder")
local text_folder = ""
local title
local text_button_right = _("Add")
Copy link
Member

Choose a reason for hiding this comment

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

I think it's a bit weird how all the button text has a "semantic" name and this one just _right. Maybe something like text_button_ok by analogy with ConfirmBox?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. Copied that from ftp.lua and forgot to change it. I didn't like the name either.

frontend/apps/cloudstorage/webdavapi.lua Outdated Show resolved Hide resolved
frontend/apps/cloudstorage/webdavapi.lua Outdated Show resolved Hide resolved
frontend/apps/cloudstorage/webdavapi.lua Outdated Show resolved Hide resolved
frontend/apps/cloudstorage/webdavapi.lua Outdated Show resolved Hide resolved
@Frenzie Frenzie merged commit a3a17db into koreader:master Oct 17, 2018
@Frenzie
Copy link
Member

Frenzie commented Oct 17, 2018

Thanks!

@Frenzie
Copy link
Member

Frenzie commented Oct 17, 2018

#4272 (comment)

The WiFi on the Clara is not amazing but too bad really. It didn't time exactly but a 5MB PDF took around 5s. I'm charging the old Kobo Mini to see if it much worse.

Well, on my laptop that'd go at something like 150 Mbps ("250", it claims in connection speed info), limited by the slightly above 50 Mbps from my ISP if it's coming from the Internet. Even in the worst 50 Mbps scenario, that'd be 1 second for 5 MB. I don't know the precise H2O wifi speed, but something like 5-10 seconds probably sounds about right. But anyway, most books I look at are much bigger than 5 MB. :-)

@y-muller y-muller deleted the webdav branch October 17, 2018 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants