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

Thanks - applied.

On Fri, Jul 8, 2016 at 7:17 AM, Neel Patel <neel.patel@enterprisedb.com> wrote:
> 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 <dpage@pgadmin.org> wrote:
>>
>> 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
>
>



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

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