Re: [pgAdmin4][runtime]: Download feature in runtime - Mailing list pgadmin-hackers

Hi

On Wed, Jul 6, 2016 at 9:12 AM, Neel Patel <neel.patel@enterprisedb.com> wrote:
> 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
> ?

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!

> On Fri, Jul 1, 2016 at 2:39 PM, Dave Page <dpage@pgadmin.org> wrote:
>>
>> 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.
>
>
> 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
>
>



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

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