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 <dp...@pgadmin.org> wrote: > On Fri, Jul 1, 2016 at 5:43 AM, Neel Patel <neel.pa...@enterprisedb.com> > wrote: > > Hi Dave, > > > > On Thu, Jun 30, 2016 at 7:31 PM, Dave Page <dp...@pgadmin.org> wrote: > >> > >> Hi > >> > >> On Thu, Jun 30, 2016 at 10:42 AM, Neel Patel > >> <neel.pa...@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 >
download_runtime_v3.patch
Description: Binary data
-- Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgadmin-hackers
