Show Open/Save dialog on downloading files #1123

juuz0 · 2024-06-04T14:34:06Z

Superseeds #1070

kelson42 · 2024-06-04T14:38:39Z

@juuz0 Thx a lot for your PR. Il will test ot in the next hours. Just to be sure: you went through all the comments of the superseeded PR to ensure we are on rge right track?

juuz0 · 2024-06-04T14:39:30Z

@kelson42 Yes I believe so.

kelson42

@juuz0 Works almost exactly like it should. Just one comment: Can you please have a better choice of directory following this logic. Actually it could use the same settings (to keep directory path, would be the same as "import").

resources/i18n/en.json

src/kprofile.cpp

src/kprofile.h

veloman-yunkan · 2024-06-11T07:07:05Z

src/kprofile.cpp

+ QTemporaryFile *tempFile = new QTemporaryFile(QDir::tempPath() + "/XXXXXX." + QFileInfo(defaultFileName).suffix());
+ if (tempFile->open()) {
+ QString tempFilePath = tempFile->fileName();
+ tempFile->close();
+#if QT_VERSION < QT_VERSION_CHECK(5, 15, 0)
+ download->setPath(tempFilePath);
+#else
+ download->setDownloadFileName(tempFilePath);
+#endif
+ connect(download, &QWebEngineDownloadItem::finished, [tempFilePath]() {
+ if(!QDesktopServices::openUrl(QUrl::fromLocalFile(tempFilePath)))
+ showInfoBox(gt("error-title"), gt("error-opening-file-with"), KiwixApp::instance()->getMainWindow());
+ });
+ download->accept();
+ } else {


Memory leak - tempFile is not deleted. You can create the QTemporaryFile object on the stack (rather than on the heap) and disable auto-remove.

Its on the stack now but I havent disabled auto-remove, why do we need that?

Then the file will be deleted when tempFile goes out of scope. Since the file is opened asynchronously (after its download is finished) that operation may fail.

src/kprofile.cpp

juuz0 · 2024-06-13T11:47:41Z

Can you please have a better choice of directory following #1058 (comment). Actually it could use the same settings (to keep directory path, would be the same as "import").

@kelson42 done! please check.

And I also rebased it on main (CI seems to be failing because of that?)

juuz0 · 2024-06-13T14:51:11Z

Fixed an issue where the compilation was failing for QT6 (i was still at 5.15 so it slipped, upgraded now)

src/kprofile.cpp

src/kprofile.h

src/kprofile.cpp

veloman-yunkan

Please rebase and fixup (which should require some manual conflict resolution) for the next (hopefully, final) iteration.

src/kiwixmessagebox.cpp

veloman-yunkan · 2024-06-17T12:35:40Z

src/kiwixmessagebox.cpp

+KiwixMessageBox::Result showKiwixMessageBox(QString title, QString text, QWidget *parent, QString leftTitle, QString rightTitle)
+{
+ KiwixMessageBox *dialog = new KiwixMessageBox(title, text, false, parent, leftTitle, rightTitle);
+ dialog->setAttribute(Qt::WA_DeleteOnClose);


This is an interesting finding. Can't we use the same trick in showInfoBox()? Or maybe even move it into the constructor of KiwixMessageBox?

Moving into the constructor is a nice idea, I did it in the new fixup.
showConfirmBox() also uses execDialog() now - It looks more natural to me that a confirmBox() is high prio and should be handled before interacting with other windows (hence the exec() call)

Now that I looked into the documentation of QDialog::exec() and Qt::WA_DeleteOnClose, I have some doubts. When QDialog::exec() returns the dialog should have closed and hence deleted. Therefore KiwixMessageBox::execDialog() will execute its last few instructions on a deleted object. Am I wrong?

Also, I am also concerned with QDialog::exec() being a blocking function. I wonder how download updater code will react to the the main thread being blocked.

Now that I looked into the documentation of QDialog::exec() and Qt::WA_DeleteOnClose ..........

Sorry for the delay, life happened!
You're right, I looked into qdialog's source code and when deleteOnClose is enabled it deletes it instantly. I reverted it to the old way of ours - a simple deleteLater().

Also, I am also concerned with QDialog::exec() being a blocking function.....

I don't really get this, we aren't starting the download until open/save is clicked (unless you mean the Download Books functionality, which I agree could've been an issue and already reverted...)

veloman-yunkan

My review for the new changes came in the form of two more comments (1, 2) on a previous thread. I am posting this response to the most recent review request to make it explicit that it has not been ignored.

kelson42 · 2024-06-26T19:58:18Z

@juuz0 Works almost exactly like it should. Just one comment: Can you please have a better choice of directory following this logic. Actually it could use the same settings (to keep directory path, would be the same as "import").

@juuz0 It seems this part is not implemented... it seems to always propose to save in the current path which is really not correct.

juuz0 · 2024-07-03T10:32:05Z

@kelson42 It works for me, can you please recheck? (unless I misunderstood what was needed)

In the attached video, original location is common, I save the file, then start another save file session - save it in tmp and in the following saves, it remembers tmp

Screencast.from.2024-07-03.15-57-18.webm

veloman-yunkan · 2024-07-04T14:46:15Z

I will review the PR again after @kelson42 approves it from user's perspective.

kelson42 · 2024-07-13T12:09:43Z

@juuz0 Retested and indeed it work fine but I have a last topic: what happens if there is no known app to open the file? I though I would have a choice of app to open the file? But instead I get a filepicker, this is not correct.

If the app to open the file is not known, then we should display the list of app and the user should choose which app he wants to use to open the file. If this is not possible, then we should probably just disabled the button in the dialog box so the user can only save to a file.

kelson42 · 2024-07-19T05:29:55Z

@juuz0 Beside answering to my last comment, can you please also give feedback to all discussion points open and close them if necessary? Rebasing on main to resolve red CI

It started as a simple yes/no box but is now being used as an InfoBox, ConfirmBox and a customized MessageBox (in further commits) - this sounds better!

Added a function to show custom choices instead of yes/no. The chosen option is returned A closeButton is also added to the dialog.

The last downloaded file's location is remembered and the "Save File" dialog opens in the same directory.

kelson42 · 2024-07-27T16:37:16Z

@juuz0 ?

juuz0 · 2024-07-27T16:40:09Z

@kelson42 sooon! (im still trying to figure out how to get the open-with selection).
QDesktopServices calls xdg-open which thinks the file manager/picker is the correct application for that specific file. I'll try finding a workaround.

kelson42 · 2024-08-02T06:59:32Z

@juuz0 Any update? Would really like to merge this PR which is slowly getting old.

kelson42 · 2024-08-11T17:45:52Z

@juuz0 Any update? We need this PR for 2.4.0!

kelson42 · 2024-09-01T13:13:59Z

@veloman-yunkan Can younplease rebase an complete this PR so we can complete 2.4.0 milestone?

juuz0 · 2024-09-09T15:41:25Z

Apologies @kelson42, I'm equipped with several things IRL - cant get the time, and yeah if someone is willing to take over it, please do! :>

My 2 cents:
The issue is xdg-open (called by QDesktopServices) which thinks the file manager/picker is the correct application for that specific file (it works ok for pdf but epub files open the manager).

One workaround is using xdg-mime. For mime-types which dont have a specific application set, xdg-mime returns nothing so if we can disable the Open button when xdg-mime query is empty, that could help.
BUT remember xdg-mime is not available on windows obviously - its possible QDesktopServices opens the application-picker-menu on windows though...up to testing!

kelson42 · 2024-09-28T12:57:13Z

@veloman-yunkan AFAIK this is the last piece of work which is assigned to you in the 2.4.0 milestone.

kelson42 mentioned this pull request Jun 4, 2024

Fix #697 : Clicking on a PDF link opens directly in a PDF reader #1070

Closed

juuz0 requested review from veloman-yunkan and kelson42 June 4, 2024 14:36

kelson42 approved these changes Jun 10, 2024

View reviewed changes

veloman-yunkan requested changes Jun 11, 2024

View reviewed changes

juuz0 force-pushed the pdfSmth branch from a9eaa9d to 3b8927b Compare June 13, 2024 11:43

juuz0 requested a review from veloman-yunkan June 13, 2024 11:47

juuz0 force-pushed the pdfSmth branch from 3b8927b to c030476 Compare June 13, 2024 14:43

veloman-yunkan requested changes Jun 14, 2024

View reviewed changes

src/kprofile.cpp Outdated Show resolved Hide resolved

src/kprofile.h Outdated Show resolved Hide resolved

veloman-yunkan requested changes Jun 14, 2024

View reviewed changes

src/kprofile.cpp Show resolved Hide resolved

juuz0 force-pushed the pdfSmth branch 2 times, most recently from 241f310 to 46f2513 Compare June 14, 2024 14:46

juuz0 requested a review from veloman-yunkan June 14, 2024 14:47

veloman-yunkan requested changes Jun 14, 2024

View reviewed changes

src/kprofile.cpp Outdated Show resolved Hide resolved

src/kprofile.cpp Outdated Show resolved Hide resolved

src/kprofile.cpp Show resolved Hide resolved

juuz0 requested a review from veloman-yunkan June 15, 2024 08:04

veloman-yunkan reviewed Jun 16, 2024

View reviewed changes

src/kiwixmessagebox.cpp Show resolved Hide resolved

juuz0 force-pushed the pdfSmth branch 2 times, most recently from 6b9f9c8 to bc0c97b Compare June 16, 2024 16:57

juuz0 requested a review from veloman-yunkan June 16, 2024 16:58

veloman-yunkan reviewed Jun 17, 2024

View reviewed changes

juuz0 force-pushed the pdfSmth branch from df7c392 to 07ed96e Compare June 17, 2024 15:25

juuz0 requested a review from veloman-yunkan June 17, 2024 15:28

veloman-yunkan reviewed Jun 25, 2024

View reviewed changes

juuz0 force-pushed the pdfSmth branch 2 times, most recently from cdf3935 to 27895d5 Compare July 3, 2024 10:03

juuz0 requested a review from veloman-yunkan July 3, 2024 10:09

juuz0 added 5 commits July 19, 2024 07:30

KiwixConfirmBox is now KiwixMessageBox

405bbd7

It started as a simple yes/no box but is now being used as an InfoBox, ConfirmBox and a customized MessageBox (in further commits) - this sounds better!

Introduce showKiwixMessageBox()

2efd924

Added a function to show custom choices instead of yes/no. The chosen option is returned A closeButton is also added to the dialog.

Open/Save functionality for downloaded files

9aefb8f

Remember previous download location

3ce2423

The last downloaded file's location is remembered and the "Save File" dialog opens in the same directory.

fixup! Introduce showKiwixMessageBox()

f046af2

kelson42 force-pushed the pdfSmth branch from 27895d5 to f046af2 Compare July 19, 2024 05:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show Open/Save dialog on downloading files #1123

Show Open/Save dialog on downloading files #1123

juuz0 commented Jun 4, 2024 •

edited by kelson42

Loading

kelson42 commented Jun 4, 2024

juuz0 commented Jun 4, 2024

kelson42 left a comment

veloman-yunkan Jun 11, 2024

juuz0 Jun 13, 2024

veloman-yunkan Jun 14, 2024

juuz0 commented Jun 13, 2024

juuz0 commented Jun 13, 2024

veloman-yunkan left a comment

veloman-yunkan Jun 17, 2024

juuz0 Jun 17, 2024

veloman-yunkan Jun 17, 2024

veloman-yunkan Jun 17, 2024

juuz0 Jul 3, 2024

veloman-yunkan left a comment

kelson42 commented Jun 26, 2024

juuz0 commented Jul 3, 2024

veloman-yunkan commented Jul 4, 2024

kelson42 commented Jul 13, 2024 •

edited

Loading

kelson42 commented Jul 19, 2024 •

edited

Loading

kelson42 commented Jul 27, 2024

juuz0 commented Jul 27, 2024

kelson42 commented Aug 2, 2024

kelson42 commented Aug 11, 2024

kelson42 commented Sep 1, 2024

juuz0 commented Sep 9, 2024

kelson42 commented Sep 28, 2024

Show Open/Save dialog on downloading files #1123

Are you sure you want to change the base?

Show Open/Save dialog on downloading files #1123

Conversation

juuz0 commented Jun 4, 2024 • edited by kelson42 Loading

kelson42 commented Jun 4, 2024

juuz0 commented Jun 4, 2024

kelson42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juuz0 commented Jun 13, 2024

juuz0 commented Jun 13, 2024

veloman-yunkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veloman-yunkan left a comment

Choose a reason for hiding this comment

kelson42 commented Jun 26, 2024

juuz0 commented Jul 3, 2024

veloman-yunkan commented Jul 4, 2024

kelson42 commented Jul 13, 2024 • edited Loading

kelson42 commented Jul 19, 2024 • edited Loading

kelson42 commented Jul 27, 2024

juuz0 commented Jul 27, 2024

kelson42 commented Aug 2, 2024

kelson42 commented Aug 11, 2024

kelson42 commented Sep 1, 2024

juuz0 commented Sep 9, 2024

kelson42 commented Sep 28, 2024

juuz0 commented Jun 4, 2024 •

edited by kelson42

Loading

kelson42 commented Jul 13, 2024 •

edited

Loading

kelson42 commented Jul 19, 2024 •

edited

Loading