Support for HTTP file retrieval in DC Plugin

[Fredrik Ullner]

It should be possible to download a file through HTTP with a DC Plugin.

[poy (poy)]

hey,

i'm not certain the DC plugin API should provide such features; but if the download is shown among other transfers, color me convinced. :)

the impl seems nice. i especially like the idea of going with the "data accessor" name - do you have future goals in mind for that interface?

my remarks:

1) we use camelCase naming for functions throughout the code, so if you could stick to it...

2) this is going to explode on large downloads. i think you want a custom output stream that will produce "HOOK_DATAACESSOR_HTTP_FILE_NOTIFICATION" events when it is written to. another possibility would be to react to HttpConnectionListener::Data events, if you have access to them. (i prefer the output stream way.)

3) when doing #2, you might notice the HTTP manager already knows how to handle downloads to files; so it would be better if it did (instead of writing the file at the end).

4) why is the "url" parameter of "HOOK_DATAACESSOR_HTTP_FILE_NOTIFICATION" a data array, and not just a string?

5) document the fact that the "localPath" argument of "get_http_file" can be null, and what it implies on the caller (having to listen for events to handle the output by itself).

[Fredrik Ullner (ullner)]

The download does indeed show up under the normal transfers. However, there is nowhere that indicates that the download was initiated by a plugin, but I'm not sure if that's useful or not.

Re "data accessor", yes, I had in mind other functionality, such as data (file) access to various setting files (favorites.xml for instance) in addition to the specific HTTP layer. The idea with the "data accessor" isn't specific to HTTP, it was simply that iceman discussed that it would be nice. I am envisioning an update plugin that can automatically update DC++ (including automatic updates for plugins themselves). Feel free to suggest other functionality that may be interesting.

1) Fair enough.

2) You mean like how the current HTTP manager sends ::Updated?

3) Ah, totally missed that. Agreed.

4) After some more consideration, there's no particular reason. It's fine either way.

5) Yep.

Will update and post a patch later on.

By the way, I could not find another way for a plugin to request to download a file synchronously without introducing busy looping or similar functionality. In reality for small files (if the requester knows it's a small file), it might be nicer to have such functionality. Also, because there is a callback, every plugin will get a notification of each file, which might be an information leak.

[poy (poy)]

the main thing to avoid regarding #2 is storing all the data in memory. the HTTP manager defaults to a StringOutputStream, which does precisely that. it's fine for small downloads, but not for a general purpose interface such as this one.
this is made even worse by the final "getString()" call and the copy to a DataArray.

this method can be available as an option, but the safe default would imo be to not handle any data aggregation in the interface implementation, but simply send "got data" events and let the plugin handle that data as it pleases.

i agree with only sending data events to the plugin that requested the download.

[Fredrik Ullner (ullner)]

File that addresses said comments

[Fredrik Ullner (ullner)]

File that addresses said comments

[poy (poy)]

looks great!

so you went with more params to the "updated" event. that's nice; much less convoluted than the stream-that-produces-events i was suggesting.

one point still bothering me: there still is a "getString" call on the "complete" event which nullifies all your efforts. i'm surprised it doesn't even crash when downloading to a file (as the stream would not be a string-stream in that case).
with this new hook style (which i like :)), there is no need to send the whole data once the download is finished: when downloading to a file, the plugin can simply open the file; when downloading via stream, the caller knows where it is storing the data.
so - remove the "data" param of HOOK_DATAACESSOR_HTTP_FILE_NOTIFICATION and this should be perfect. :)

[dat copyright year change that sneaked in tho]

[Fredrik Ullner (ullner)]

I've pushed this now.

poy: If you have any objection, feel free to modify the code yourself.

[poy (poy)]

hey,
sorry for the late reply.

this is looking very good! i have minor points but nothing blocking, hopefully:

1) could you test 2 downloads in parallel and confirm nothing gets mixed up?

2) are plugins able to determine whether a download has succeeded or failed in their "done" notification?

3) the 2 getDataArrayPtr functions are nearly identical - maybe have one call the other?

4) the "HTTP file" term just jumped to me, especially now that we don't necessarily download to (or from) files. i think a better term is "resource" <https://en.wikipedia.org/wiki/Web_resource>.

[Fredrik Ullner (ullner)]

I've updated the code.

1) I tested this back when I was working on the interface, so this should work now as well.

2) Added an additional notification for "Failed".

3) Refactored.

4) Changed.

[poy (poy)]

great!

[poy (poy)]

Fixed in DC++ 0.860.