Re: [pgAdmin4][runtime]: Download feature in runtime

On Fri, Jul 1, 2016 at 5:43 AM, Neel Patel <neel.patel@enterprisedb.com> wrote:
> Hi Dave,
>
> On Thu, Jun 30, 2016 at 7:31 PM, Dave Page <dpage@pgadmin.org> wrote:
>>
>> Hi
>>
>> On Thu, Jun 30, 2016 at 10:42 AM, Neel Patel
>> <neel.patel@enterprisedb.com> wrote:
>> > Hi,
>> >
>> > Please find attached patch file for initial version of download file in
>> > runtime application.
>>
>> I've attached an update with some improved messages, and setting the
>> progress dialogue to be modal (seeing as we cannot have multiple
>> downloads, and it's easy to lose the dialogue).
>>
>> > With this patch, we have implemented two features.
>> >
>> > Feature 1 :- Normal "Download file" from runtime application
>> >
>> > Previously "Download file" was not implemented in runtime application.
>> > With this patch file, we have handled Qt signal for download file
>> > properly.
>>
>> This seems to work fine. I did get one crash (after I cancelled a
>> download, then tried it again), but I couldn't reproduce that.
>
>
> Okay. I will try to reproduce the issue and also i will try to review the
> code again if i can find something regrading crash.

Thanks.

>
>>
>> > Feature 2 :-   "download" attribute support for 'a' tag for client side
>> > download
>> >
>> > As per our knowledge, webkit has not implemented the download attribute
>> > at
>> > 'a' tag.
>> > Currently it shows under development from below link.
>> >
>> > https://bugreports.qt.io/browse/QTBUG-47732
>> >
>> > We did not found any signal in Qt for download attribute feature but to
>> > implement this feature in runtime application, we added one workaround
>> > to
>> > make it work with download CSV file.
>> >
>> > When we click on download buttons, we are getting Qt signal
>> > "urlLinkClicked"
>> > and in that url we are finding "data:text/csv" from encoded URL
>> > generated
>> > from sqleditor. Once we found that tag then we are decoding the csv data
>> > and
>> > writing to file.
>> >
>> > Is that right approach ? Should we add our own custom mime-type to
>> > header ?
>> > Let us know your thoughts on this feature.
>>
>> This doesn't work so well, for a number of reasons:
>>
>> 1) QT Creator is complaining that your regexp contains an invalid
>> escape sequence (line 546).
>
>
> I will fix.
>>
>>
>> 2) The default file name seems to be the entire data blob. I would
>> suggest making the file name "download.csv" if we don't know anything
>> better. The "csv" part should be taken from the mime type (see below)
>>
>> 3) Should we handle all "data:" downloads in this way? Taking the file
>> type and default extension from the mimetype offered.
>
>
> We can handle all "data:" download. We will extract the filename and
> extension from mime type.
> As i know, Qt provides QUrlQuery class which will be useful to find the key
> value pair. I will test and let you know.
>
> e.g. If we have header as below
>
> "data:text/csv;charset=utf-8;Content-disposition:attachment;filename=download.csv;"
>
> From the QurlQuery class we can query "filename" and "data:" and accordingly
> save the data to filename provided.
>
> Please suggest.

Sounds good.

>> 4) When I change the filename the data is properly saved, but then I
>> get a confirmation message that still has the full data blob as the
>> filename.

>>
>> 5) It somehow seems to have let me save files with forward slashes in
>> the name. See attachment.

>
>
> I think we should not ask for "Save as" dialog. If there is no key found of
> "filename" in encodedURI then we should create the file "download.csv" in
> user's download directory and save the csv data.

Well we can get the extension from the mimetype in that instance, but
otherwise I agree with the naming. I do think we need a Save As
dialogue, as the user should be able to choose the location for the
file (and rename it). We should also remember the last save location
for convenience.

>> 6) I get all sorts of weird redrawing on the screen when downloading a
>> data blob. I suspect it's because the filename (which is still the
>> entire data blob) is shown on the progress dialogue.
>>

>
> I will try to fix as per above comments and submit the patch again.
> Let us know for any misunderstanding.

Cool, thanks.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company