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

Hi Dave,

I have tried to fix most of the review comments. I have modified the patch
on top of your changes. Please find attached updated patch file.
Find my comments inline. Can you please review and let us know your
feedback ?

Thanks,
Neel Patel

On Fri, Jul 1, 2016 at 2:39 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

> On Fri, Jul 1, 2016 at 5:43 AM, Neel Patel <neel(dot)patel(at)enterprisedb(dot)com>
> wrote:
> > Hi Dave,
> >
> > On Thu, Jun 30, 2016 at 7:31 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
> >>
> >> Hi
> >>
> >> On Thu, Jun 30, 2016 at 10:42 AM, Neel Patel
> >> <neel(dot)patel(at)enterprisedb(dot)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.
>

I have tried to reproduce the crash but no luck. I have tried on Linux and
Mac.

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

I found that it is due to different Qt version. You might be using Qt 5.5.
In Qt 5.5, we are getting "download" signal and for Qt < 5.5 we are getting
"urlLinkClicked" signal for client side data download.
We have fixed the issue for all Qt version. Let me know if you can still
able to reproduce the issue.

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

Fixed.

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

Fixed.

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

Fixed.

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