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

From: Neel Patel <[email-masked]>
Date: Thu, 30 Jun 2016 15:12:31 +0530

Hi,

Please find attached patch file for initial version of download file in
runtime application.

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.

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

Please review it and let us know comments.

Thanks,
Neel Patel

-- 
Sent via pgadmin-hackers mailing list ([email-masked])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

---

From: Dave Page <[email-masked]>
Date: Thu, 30 Jun 2016 15:01:07 +0100

Hi

> Please find attached patch file for initial version of download file in

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

> Feature 1 :- Normal "Download file" from runtime application
>

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.

> Feature 2 :-   "download" attribute support for 'a' tag for client side
> download
>
> 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.
>

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

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.

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.

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.

Thanks.

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

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

---

From: Neel Patel <[email-masked]>
Date: Fri, 1 Jul 2016 10:13:42 +0530

Hi Dave,

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

> "urlLinkClicked"

I will fix.

> 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: Dave Page <[email-masked]>
Date: Fri, 1 Jul 2016 10:09:11 +0100

Thanks.

> From the QurlQuery class we can query "filename" and "data:" and accordingly
> save the data to filename provided.
>
> Please suggest.

Sounds good.

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

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

Cool, thanks.

---

From: Neel Patel <[email-masked]>
Date: Wed, 6 Jul 2016 13:42:43 +0530

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

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

> >> > As per our knowledge, webkit has not implemented the download
> attribute
> >> > at
> >> > 'a' tag.
> >> > from sqleditor. Once we found that tag then we are decoding the csv
> data
> >> > and
> >> > writing to file.
> >> >

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.

> >> the name. See attachment.

Fixed.

> file (and rename it). We should also remember the last save location

Fixed.

> >> entire data blob) is shown on the progress dialogue.

Fixed.

---

From: Dave Page <[email-masked]>
Date: Thu, 7 Jul 2016 09:43:01 +0100

Hi

On Wed, Jul 6, 2016 at 9:12 AM, Neel Patel <[email-masked]> wrote:
> Find my comments inline. Can you please review and let us know your feedback
> ?

That's definitely getting there;

- Please make sure you follow the code style requirements, e.g.
//<space>Comment text

- In your comments, typically you should refer to the user as "the
user" not just "user". e.g.
  // Check that *the* user has given *a* valid file name or not

  The same applies to other cases where you miss the article
(https://en.wikipedia.org/wiki/Article_(grammar)):
  // Check that *the* request contains the data download at client side

NOTE: This isn't a criticism of you in particular - most of the team
do this, I assume because it's more like the way you'd phrase things
in Hindi. I just find myself correcting such mistakes regularly, and
it's good for us all to continue to improve in general.

- I was able to reproduce the crash again:
  1) Open a tab, and go to the PostgreSQL download page on
enterprisedb.com (linked from the pg.org site)
  2) Start to download the 9.6b2 Win64 installer
  3) Cancel the download
  4) Click the link to download if your download didn't automatically start
  5) Overwrite the existing file

This results in:
  a) The progress bar flashes up and down weirdly on the second download
  b) The app crashes when the download completes:

The program has unexpectedly finished.
/Users/dpage/git/pgadmin4/build-pgAdmin4-Desktop_Qt_5_5_1_clang_64bit2-Debug/pgAdmin4.app/Contents/MacOS/pgAdmin4
crashed

See the attached backtrace.

Thanks!

>> >> > implement this feature in runtime application, we added one
>> >> > workaround
>> >> > to

---

From: Neel Patel <[email-masked]>
Date: Fri, 8 Jul 2016 11:47:23 +0530

Hi Dave,

Please find attached patch file for the fix of crash and comment text.
Downloading cancel request was not handled properly and due to that
application was getting crashed.

Do review it and let us know for comments.

Thanks,
Neel Patel

On Thu, Jul 7, 2016 at 2:13 PM, Dave Page <[email-masked]> wrote:

> feedback
> >> On Fri, Jul 1, 2016 at 5:43 AM, Neel Patel <[email-masked]
> >
> >> wrote:
> >> > Hi Dave,
> >> >
> >> >> > Please find attached patch file for initial version of download
> file
> >> >> > in
> >> >> > runtime application.
> >> >>
> >> >> > We did not found any signal in Qt for download attribute feature
> but
> >> >> > to
> >> > As i know, Qt provides QUrlQuery class which will be useful to find
> the
> >> > key
> >> > I think we should not ask for "Save as" dialog. If there is no key
> found
> >> > of

---

From: Dave Page <[email-masked]>
Date: Wed, 13 Jul 2016 12:26:41 +0100

Thanks - applied.

On Fri, Jul 8, 2016 at 7:17 AM, Neel Patel <[email-masked]> wrote:
>> >> On Fri, Jul 1, 2016 at 5:43 AM, Neel Patel
>> >> > "filename" in encodedURI then we should create the file
>> >> > "download.csv"
>> >> > in