Implemented Save Page #1134

ShaopengLin · 2024-06-20T02:31:30Z

Fix #77
Changes:

Re-enabled file menu item and shortcut Ctrl + Shift + S for save page.
Saves the HTML of the page on the current tab to PDF.
Saves non-HTML pages directly as files.
Remembers the last directory saved when opening file selection dialoge.

ShaopengLin · 2024-06-20T03:04:49Z

src/webview.cpp

+        {
+            QString fileName = QFileDialog::getSaveFileName(
+                app->getMainWindow(), gt("save-file-as-window-title"),
+                suggestedPath, gt("pdf-files-filter") + " (*.pdf)");


Is the gt for the filter text needed? I didn't see one for our .zim filters but I added it just in case.

To me it seems mandatory, otherwise how would you get a translated message?! Or I get it wrong?! @veloman-yunkan

ShaopengLin · 2024-06-21T05:34:04Z

Actually, we should hold off until #1132 is merged as we have to test how that works with this save page. I am also seeing some places of improvement for this PR.

kelson42 · 2024-06-27T04:39:47Z

@ShaopengLin I will review this PR, can you please rebase and fix the conflict?

ShaopengLin · 2024-06-27T04:55:32Z

@kelson42 done

kelson42 · 2024-07-13T11:21:39Z

Why CTRL + shift + S? usual (Web Browser) shortcut is CTRL + S, at least on linux. (otherwise it works)
Saving a mp4 video directly does not work, it proposes to save a PDF file, not directly the .mp4 file. You can check https://tmp.kiwix.org/ci/tests_fr_reg-videos_2024-05.zim
If I want to save the article "AC/DC" then it tries to create DC.pdf, please create a function which sanitise properly the filename and propose something like AC_DC.pdf, so replacing all unallowed charatecer on the filesystem by an _ character. Actually I would recommend to implement a slugify() function.

ShaopengLin · 2024-07-14T00:24:43Z

If I want to save the article "AC/DC" then it tries to create DC.pdf

Edit: Okay I believe I misunderstood the proposed change. You meant if the article name IS AC/DC, we would like to sanitize it to AC_DC.
@kelson42 I may be wrong I believe your case happens like this: The native/Qt window manager would treat it as if AC is a directory and DC.pdf is the name. The path will be path/AC/DC.pdf which simply fails to save. For this case, there is no way for us to know if the user intended for it to be a path element or a file name when the results get back to us. I don't think we have a choice here but just report an error.

In most cases, native Linux window manager simply doesn't allow this to happen and reports the error while keeping the save dialog open. I don't believe windows' window manager will allow saves to invalid file names either.

ShaopengLin · 2024-07-15T05:20:26Z

@kelson42 Done.

Saving a mp4 video directly does not work, it proposes to save a PDF file, not directly the .mp4 file.

The video/contents that can be saved are these ones opened directly as a video file in the tab:

For the ones embedded inside an HTML web page, we would not be able to identify those as they are indeed web pages:

These videos can just be downloaded using the provided download button:

kelson42 · 2024-07-15T06:50:20Z

@ShaopengLin Please update me on each point I have given. In general this is very important to be very clear about what is done and why. The reviewer has a limited amount of time and therefore the goal is to implement PR so the reviewer has just to approve it without having to ask questions and obviously ask for changes.

Here I have no clue if you have fixed the "bad" shortcut, why it was bad in a first place

ShaopengLin · 2024-07-15T07:09:54Z

@kelson42

The shortcut for save is now Ctrl + S. Qt's "Save As" shortcut is Ctrl + Shift + S as it assumes a desktop environment instead of a browser one.
The issue is explained in my previous comment.
We now do a simple sanitization on the appearances of reserved characters of the OS in the file name by replacing them with '_'. This prevents our code from parsing a file name "AC/DC" as file "DC" in the directory "AC". All we care about here is preventing any side effects from using the title text as file name, as it should just be a placeholder name. Determining the file name validity is the job of the OS file window manager.

kelson42 · 2024-07-19T05:56:39Z

I have rebased as we are approaching the time to make the code review and the PR is already quite old.

kelson42 · 2024-07-19T05:58:38Z

@veloman-yunkan @ShaopengLin I would recommend to move the string slugification method to libkiwix stringTools.cpp. What do you think?

kelson42 · 2024-07-19T06:10:04Z

@ShaopengLin Looks good to me finally. Could you please add as well the the contextual menu like on a Web Browser?
@veloman-yunkan Ready to code review.

kelson42

LGTM

ShaopengLin · 2024-07-22T00:19:14Z

I would be fine with moving slugification to libkiwix. Note this doesn't guarantee the file name is 100% valid in Windows. They have a lot of rules extra rules.

ShaopengLin · 2024-07-27T17:22:23Z

@kelson42 Do you want me to just PR from fork to libkiwix, or create a branch like we do here? If the latter I will need to be a collaborator.

kelson42 · 2024-07-27T17:50:04Z

@kelson42 Do you want me to just PR from fork to libkiwix, or create a branch like we do here? If the latter I will need to be a collaborator.

Sorry, you should just have received an invitation to be member of the team and have write permission.

kelson42 · 2024-07-30T19:52:44Z

Blocked by kiwix/libkiwix#1105

ShaopengLin · 2024-08-15T03:48:33Z

@kelson42 @veloman-yunkan Ready for review. Updated change from kiwix/libkiwix#1105.

kelson42 · 2024-08-19T11:09:20Z

@ShaopengLin I want to test it, but would be very nice if you could rebase it (once again).

ShaopengLin · 2024-08-19T14:54:29Z

@kelson42 Done.

resources/i18n/en.json

src/kiwixapp.cpp

veloman-yunkan · 2024-08-26T09:36:43Z

src/kprofile.cpp

    QString extension =
        isSavePageDownload ? ".pdf" : "." + defaultFileName.section('.', -1);


Can't the extension now be derived only from defaultFileName (due to page()->save(suggestedFileName + ".pdf"); in this commit? If yes, you can also get rid of the isSavePageDownload variable and put download->isSavePageDownload() in the only remaining point of usage directly.

src/library.cpp

veloman-yunkan

If it were not for the pagr typo I might have approved. But since we are going to go through another iteration I chose not to silence the perfectionist side of me :)

src/library.cpp

src/library.h

resources/i18n/qqq.json

src/kprofile.cpp

veloman-yunkan

~~The commit that dropped category translations was lost. Was that a mistake?~~

Sorry, I confused this PR with #1175 :)

src/urlschemehandler.cpp

Retrieval function now includes parsing of path. Prepares future change.

Page saved as either pdf or file based on content

Document folder is default.

Convert reserved chars to '_' before parsing

ShaopengLin force-pushed the Issue#77-implement-save-page branch from aff6cfe to 1f9c641 Compare June 20, 2024 03:03

ShaopengLin commented Jun 20, 2024

View reviewed changes

ShaopengLin requested review from veloman-yunkan and kelson42 June 20, 2024 03:06

ShaopengLin force-pushed the Issue#77-implement-save-page branch from 1f9c641 to 8b94432 Compare June 21, 2024 00:20

ShaopengLin force-pushed the Issue#77-implement-save-page branch from 8b94432 to c23025d Compare June 22, 2024 05:24

ShaopengLin force-pushed the Issue#77-implement-save-page branch from c23025d to 60c5e0e Compare June 27, 2024 04:49

ShaopengLin force-pushed the Issue#77-implement-save-page branch 2 times, most recently from e3762e7 to 76d5477 Compare July 15, 2024 05:09

ShaopengLin force-pushed the Issue#77-implement-save-page branch from 76d5477 to dbec9ce Compare July 17, 2024 06:59

kelson42 force-pushed the Issue#77-implement-save-page branch from dbec9ce to 8862b10 Compare July 19, 2024 05:56

kelson42 reviewed Jul 19, 2024

View reviewed changes

ShaopengLin force-pushed the Issue#77-implement-save-page branch from 886c7ae to a504ee7 Compare July 22, 2024 00:45

ShaopengLin mentioned this pull request Jul 27, 2024

Add String Slugification for Generating File Name kiwix/libkiwix#1105

Merged

ShaopengLin force-pushed the Issue#77-implement-save-page branch from a504ee7 to 97de27c Compare August 15, 2024 03:25

ShaopengLin force-pushed the Issue#77-implement-save-page branch from 97de27c to 81f1dd5 Compare August 15, 2024 03:47

ShaopengLin force-pushed the Issue#77-implement-save-page branch from 81f1dd5 to 1d3deb4 Compare August 19, 2024 14:47

kelson42 force-pushed the Issue#77-implement-save-page branch 2 times, most recently from 1a6f621 to 3392531 Compare August 23, 2024 14:15

veloman-yunkan requested changes Aug 26, 2024

View reviewed changes

ShaopengLin force-pushed the Issue#77-implement-save-page branch from 3392531 to 27ee036 Compare August 29, 2024 15:44

ShaopengLin requested a review from veloman-yunkan August 29, 2024 15:45

veloman-yunkan requested changes Aug 29, 2024

View reviewed changes

src/library.cpp Outdated Show resolved Hide resolved

src/library.h Outdated Show resolved Hide resolved

resources/i18n/qqq.json Outdated Show resolved Hide resolved

src/kprofile.cpp Outdated Show resolved Hide resolved

ShaopengLin force-pushed the Issue#77-implement-save-page branch from 27ee036 to 0465442 Compare August 30, 2024 07:19

ShaopengLin requested a review from veloman-yunkan August 30, 2024 07:20

veloman-yunkan requested changes Aug 30, 2024

View reviewed changes

src/urlschemehandler.cpp Outdated Show resolved Hide resolved

ShaopengLin force-pushed the Issue#77-implement-save-page branch from 0465442 to cf71bcb Compare August 30, 2024 09:35

ShaopengLin requested a review from veloman-yunkan August 30, 2024 09:37

veloman-yunkan approved these changes Aug 30, 2024

View reviewed changes

ShaopengLin added 5 commits August 30, 2024 10:41

Enhance urlschemehandler archive entry retrieval

8877214

Retrieval function now includes parsing of path. Prepares future change.

Add support to save page

6ff4dff

Page saved as either pdf or file based on content

Save As remember previous save path

3988067

Document folder is default.

Sanitizes file name suggestion

fc029c7

Convert reserved chars to '_' before parsing

Added Save Page to context menu

7fcd6b5

kelson42 force-pushed the Issue#77-implement-save-page branch from cf71bcb to 7fcd6b5 Compare August 30, 2024 10:41

kelson42 merged commit 0c7b60c into main Aug 30, 2024
5 checks passed

kelson42 deleted the Issue#77-implement-save-page branch August 30, 2024 11:02

veloman-yunkan mentioned this pull request Nov 5, 2024

Show Open/Save dialog on downloading files #1123

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented Save Page #1134

Implemented Save Page #1134

ShaopengLin commented Jun 20, 2024

ShaopengLin Jun 20, 2024

kelson42 Jul 19, 2024

ShaopengLin commented Jun 21, 2024 •

edited

Loading

kelson42 commented Jun 27, 2024

ShaopengLin commented Jun 27, 2024

kelson42 commented Jul 13, 2024 •

edited

Loading

ShaopengLin commented Jul 14, 2024 •

edited

Loading

ShaopengLin commented Jul 15, 2024 •

edited

Loading

kelson42 commented Jul 15, 2024 •

edited

Loading

ShaopengLin commented Jul 15, 2024 •

edited

Loading

kelson42 commented Jul 19, 2024

kelson42 commented Jul 19, 2024

kelson42 commented Jul 19, 2024

kelson42 left a comment

ShaopengLin commented Jul 22, 2024 •

edited

Loading

ShaopengLin commented Jul 27, 2024

kelson42 commented Jul 27, 2024

kelson42 commented Jul 30, 2024

ShaopengLin commented Aug 15, 2024

kelson42 commented Aug 19, 2024

ShaopengLin commented Aug 19, 2024

veloman-yunkan Aug 26, 2024

veloman-yunkan left a comment

veloman-yunkan left a comment •

edited

Loading

		QString extension =
		isSavePageDownload ? ".pdf" : "." + defaultFileName.section('.', -1);

Implemented Save Page #1134

Implemented Save Page #1134

Conversation

ShaopengLin commented Jun 20, 2024

ShaopengLin Jun 20, 2024

Choose a reason for hiding this comment

kelson42 Jul 19, 2024

Choose a reason for hiding this comment

ShaopengLin commented Jun 21, 2024 • edited Loading

kelson42 commented Jun 27, 2024

ShaopengLin commented Jun 27, 2024

kelson42 commented Jul 13, 2024 • edited Loading

ShaopengLin commented Jul 14, 2024 • edited Loading

ShaopengLin commented Jul 15, 2024 • edited Loading

kelson42 commented Jul 15, 2024 • edited Loading

ShaopengLin commented Jul 15, 2024 • edited Loading

kelson42 commented Jul 19, 2024

kelson42 commented Jul 19, 2024

kelson42 commented Jul 19, 2024

kelson42 left a comment

Choose a reason for hiding this comment

ShaopengLin commented Jul 22, 2024 • edited Loading

ShaopengLin commented Jul 27, 2024

kelson42 commented Jul 27, 2024

kelson42 commented Jul 30, 2024

ShaopengLin commented Aug 15, 2024

kelson42 commented Aug 19, 2024

ShaopengLin commented Aug 19, 2024

veloman-yunkan Aug 26, 2024

Choose a reason for hiding this comment

veloman-yunkan left a comment

Choose a reason for hiding this comment

veloman-yunkan left a comment • edited Loading

Choose a reason for hiding this comment

ShaopengLin commented Jun 21, 2024 •

edited

Loading

kelson42 commented Jul 13, 2024 •

edited

Loading

ShaopengLin commented Jul 14, 2024 •

edited

Loading

ShaopengLin commented Jul 15, 2024 •

edited

Loading

kelson42 commented Jul 15, 2024 •

edited

Loading

ShaopengLin commented Jul 15, 2024 •

edited

Loading

ShaopengLin commented Jul 22, 2024 •

edited

Loading

veloman-yunkan left a comment •

edited

Loading