Skip to content

Add options for downloading multimedia content - #1424

Open
vighnesh-sawant wants to merge 1 commit into
kiwix:mainfrom
vighnesh-sawant:context-menu-improvement
Open

Add options for downloading multimedia content #1424
vighnesh-sawant wants to merge 1 commit into
kiwix:mainfrom
vighnesh-sawant:context-menu-improvement

Conversation

@vighnesh-sawant

@vighnesh-sawant vighnesh-sawant commented Nov 26, 2025

Copy link
Copy Markdown
Contributor

Fixes #1274
This PR adds an option to save image when right clicked on an image, save video when right clicked on an video and save video when right clicked on an audio.
In rest of the cases it gives you an option to save page as html.
I think this a better fix then the last one

@vighnesh-sawant

Copy link
Copy Markdown
Contributor Author

Qt6 builds are failing, will look into it.

@vighnesh-sawant

Copy link
Copy Markdown
Contributor Author

Tested for qt6 also.
@kelson42 can you please have a look at this.

@kelson42

kelson42 commented Dec 2, 2025

Copy link
Copy Markdown
Collaborator

@vighnesh-sawant Thank you very much for this PR. I will test it this evening!

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the context menu implementation in the web view by replacing the custom-built context menu with Qt WebEngine's standard context menu functionality, while adding appropriate icons and removing media control options.

Key Changes:

  • Replaced custom back/forward actions with Qt WebEngine's standard context menu
  • Added version-specific handling for Qt 5 vs Qt 6 API differences
  • Set custom icons for back/forward actions and removed media control options

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/webview.cpp Outdated
Comment on lines +326 to +335
#if QT_VERSION >= QT_VERSION_CHECK(6, 0, 0)
QMenu* menu = QWebEngineView::createStandardContextMenu();
#else
QMenu* menu = page()->createStandardContextMenu();
#endif
page()->action(QWebEnginePage::Back)->setIcon(app->getAction(KiwixApp::HistoryBackAction)->icon());
page()->action(QWebEnginePage::Forward)->setIcon(app->getAction(KiwixApp::HistoryForwardAction)->icon());
if (page()->action(QWebEnginePage::ToggleMediaControls)) {
menu->removeAction(page()->action(QWebEnginePage::ToggleMediaControls));
}

Copilot AI Dec 2, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indentation style here is inconsistent with the rest of the file. In this file, preprocessor directives like #if are not indented (see lines 98, 104, 312, 317). Additionally, the code within the #if blocks uses 2-space indentation instead of the 4-space indentation used elsewhere.

Please adjust to match the file's style:

#if QT_VERSION >= QT_VERSION_CHECK(6, 0, 0)
    QMenu* menu = QWebEngineView::createStandardContextMenu();
#else
    QMenu* menu = page()->createStandardContextMenu();
#endif
    page()->action(QWebEnginePage::Back)->setIcon(app->getAction(KiwixApp::HistoryBackAction)->icon());
    page()->action(QWebEnginePage::Forward)->setIcon(app->getAction(KiwixApp::HistoryForwardAction)->icon());
    if (page()->action(QWebEnginePage::ToggleMediaControls)) {
        menu->removeAction(page()->action(QWebEnginePage::ToggleMediaControls));
    }

Copilot uses AI. Check for mistakes.
Comment thread src/webview.cpp Outdated
QMenu* menu = QWebEngineView::createStandardContextMenu();
#else
QMenu* menu = page()->createStandardContextMenu();
#endif

Copilot AI Dec 2, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The createStandardContextMenu() functions can return nullptr if there is no context menu available. You should add a null check after creating the menu to prevent potential crashes when accessing menu in subsequent lines.

#if QT_VERSION >= QT_VERSION_CHECK(6, 0, 0)
    QMenu* menu = QWebEngineView::createStandardContextMenu();
#else
    QMenu* menu = page()->createStandardContextMenu();
#endif
if (!menu) {
    menu = new QMenu(this);
}
Suggested change
#endif
#endif
if (!menu) {
menu = new QMenu(this);
}

Copilot uses AI. Check for mistakes.
Comment thread src/webview.cpp Outdated
if (page()->action(QWebEnginePage::ToggleMediaControls)) {
menu->removeAction(page()->action(QWebEnginePage::ToggleMediaControls));
}

Copilot AI Dec 2, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing whitespace detected. Please remove the trailing spaces at the end of this line.

Suggested change

Copilot uses AI. Check for mistakes.
@kelson42
kelson42 force-pushed the context-menu-improvement branch from 454bcee to 48ffcac Compare December 3, 2025 18:28
@kelson42

kelson42 commented Dec 3, 2025

Copy link
Copy Markdown
Collaborator

@vighnesh-sawant I have a bit of a hard time to identify what your PR changed from a user perspective. Could you please clarify this with a list of differences and a small description in the PR description?

That been said, I have already a small feedback:

  • I believe the better approach for the "Save article as PDF" feature, is to move it in the top (toolbar) contextual menu
  • "Save as..." should work like in a browser and save the multimedia element below the pointer
  • I have a segfault crash scenario when I right click many times in a row (and also on the contextual menu)

@vighnesh-sawant
vighnesh-sawant force-pushed the context-menu-improvement branch from 48ffcac to f2b672c Compare December 7, 2025 07:13
@vighnesh-sawant vighnesh-sawant changed the title Add default context menu from qtwebengine and set appropriate icons. … Add options for downloading multimedia content Dec 7, 2025
@vighnesh-sawant

Copy link
Copy Markdown
Contributor Author

@kelson42 I have changed the PR a lot, I think this a better option that I did not figure out then.
The changes from the user perspective are mentioned in the PR description

@vighnesh-sawant
vighnesh-sawant marked this pull request as ready for review December 7, 2025 07:17
@kelson42
kelson42 force-pushed the context-menu-improvement branch from f2b672c to 2061d18 Compare December 10, 2025 06:58

@kelson42 kelson42 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vighnesh-sawant Thank you for this new iteration. Here my remarks:

  • Saving an image works, but one I have done it once, the item is not display anymore, see for example (actually I have achieved to save the thumbnail.webp once and then not anymore. Something is wrong here):
Image
  • The contextual menu should not allow anymore to save the article in PDF, please move this in the topbar contextual menu (I would suggest between "Random article" and "print"). I would recommend to do that first in a dedicated issue.

  • Saving a video does not work for me, see

Image
  • To me it seems the code should allow to download anything via the right-click contextual menu. Here I have an .epub behind the link... and I can not save it.
Image

@vighnesh-sawant
vighnesh-sawant force-pushed the context-menu-improvement branch from 2061d18 to 2393714 Compare December 11, 2025 08:28
@vighnesh-sawant

vighnesh-sawant commented Dec 11, 2025

Copy link
Copy Markdown
Contributor Author
  1. This is fixed now ( I was testing on an article, not on the front page)
  2. Save page as pdf as been kept in
image

Should I change it? (asking for confirmation)
The save page in the normal contextual menu saves as html.

  1. Can you please point me to where this video is ( I am not able to reproduce this)
  2. Fixed this, this is named as save link,if we need this to be named as something else please do tell.

@vighnesh-sawant
vighnesh-sawant force-pushed the context-menu-improvement branch 2 times, most recently from 6624f48 to 11a5771 Compare December 11, 2025 08:38
@vighnesh-sawant
vighnesh-sawant force-pushed the context-menu-improvement branch from 11a5771 to 6b2b317 Compare December 11, 2025 08:53
@vighnesh-sawant

Copy link
Copy Markdown
Contributor Author

@kelson42 forgot to tag you please look at this iteration!

@kelson42

kelson42 commented Dec 17, 2025

Copy link
Copy Markdown
Collaborator

@vighnesh-sawant

Here my remarks:

  • Please take the code to move the "Save article" out of the context menu in a dedicated branch. I have created a dedicated issue for that and this issue should be treated first.
  • Please answer the remarks of copilat
  • I see that you have code specific to Qt6... but it applies only if you have specific version 6.0.0
  • You can get the TED ZIM videos from https://library.kiwix.org/#lang=&category=ted

Once #1433 implemented an merged, we should rebase this one and restest everything... sorry, but for the moment this part is still pretty fuzzy at this stage.

@rgaudin

rgaudin commented Dec 17, 2025

Copy link
Copy Markdown
Member

On the video issue, from @kelson42's screenshot, I understand it's not a <video /> at this stage, but just a poster. @kelson42 can you click the play button and retry?

@kelson42

Copy link
Copy Markdown
Collaborator

We are working on #1434 for the moment, once merged we will rebased and finish the work here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Saving a video fails (and try to save a PDF)

4 participants