FEAT(Client): Add Documentation dialog with official resource links#7144
FEAT(Client): Add Documentation dialog with official resource links#7144FNXDOOM wants to merge 1 commit intomumble-voip:masterfrom
Conversation
WalkthroughA documentation dialog feature has been added to the Mumble client. The change introduces a new 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mumble/Documentation.cpp`:
- Around line 20-34: The documentationView currently calls
documentationView->setOpenExternalLinks(true) which allows any URL scheme to be
opened; change this to documentationView->setOpenExternalLinks(false) and
connect documentationView's anchorClicked signal to a handler (e.g.,
onDocumentationAnchorClicked) that inspects the QUrl scheme and only calls
QDesktopServices::openUrl for "http" or "https" schemes (otherwise reject/log
the URL); apply the same pattern to the analogous code in About.cpp.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d2dc723f-897b-4c87-bc43-94318f534acc
⛔ Files ignored due to path filters (1)
mumble_env.x64-windows-static-md.b1fe4a4257.7zis excluded by!**/*.7z
📒 Files selected for processing (6)
src/mumble/CMakeLists.txtsrc/mumble/Documentation.cppsrc/mumble/Documentation.hsrc/mumble/MainWindow.cppsrc/mumble/MainWindow.hsrc/mumble/MainWindow.ui
| documentationView->setOpenExternalLinks(true); | ||
| documentationView->setAccessibleName(tr("Documentation links")); | ||
| documentationView->setHtml( | ||
| tr("<h2>Welcome to Mumble Documentation</h2>" | ||
| "<h3>Documentation</h3>" | ||
| "<p>You can find official Mumble documentation and support resources below.</p>" | ||
| "<p>For new users, start with the <a href=\"https://www.mumble.info/documentation/user/\">User Guide</a>.</p>" | ||
| "<p>Configure key bindings with <a href=\"https://www.mumble.info/documentation/user/global-shortcuts/\">Global Shortcuts</a>.</p>" | ||
| "<p>Tune your setup with the <a href=\"https://www.mumble.info/documentation/user/audio-settings/\">Audio Settings guide</a>.</p>" | ||
| "<p>Browse all topics in <a href=\"https://www.mumble.info/documentation/\">Main Documentation</a>.</p>" | ||
| "<h3>Help and Resources</h3>" | ||
| "<p>Download the latest release from <a href=\"https://www.mumble.info/downloads/\">Downloads</a>.</p>" | ||
| "<p>Report issues or get technical help via <a href=\"https://github.com/mumble-voip/mumble/issues\">GitHub Issues</a>.</p>" | ||
| "<p>Read project updates on the <a href=\"https://www.mumble.info/blog/\">Blog</a>.</p>" | ||
| "<p>Reach the team through the <a href=\"https://www.mumble.info/contact/\">Contact page</a>.</p>")); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify URL-opening patterns and whether scheme validation is applied consistently.
# Expected: this new dialog currently appears with setOpenExternalLinks(true), while
# other call sites show explicit scheme checks.
rg -n -C2 'setOpenExternalLinks\s*\(\s*true\s*\)' --type=cpp
rg -n -C3 'QDesktopServices::openUrl\s*\(' --type=cpp
rg -n -C3 'allowedSchemes|url\.scheme\(\)\s*(!=|==)|contains\s*\(\s*url\.scheme\(\)\s*\)' --type=cppRepository: mumble-voip/mumble
Length of output: 12666
Harden external-link handling with a scheme allowlist.
setOpenExternalLinks(true) on line 20 opens any URL scheme without validation, bypassing the stricter scheme checks applied elsewhere in the codebase (e.g., ConnectDialog, MainWindow, Log). Replace with setOpenExternalLinks(false) and connect to anchorClicked to validate only http/https schemes before opening.
Note: About.cpp has the same vulnerability in multiple locations and should be addressed similarly.
Proposed fix
`#include` "Documentation.h"
+#include <QtCore/QSet>
+#include <QtGui/QDesktopServices>
`#include` <QtWidgets/QPushButton>
`#include` <QtWidgets/QTextBrowser>
`#include` <QtWidgets/QVBoxLayout>
DocumentationDialog::DocumentationDialog(QWidget *parent) : QDialog(parent) {
@@
QTextBrowser *documentationView = new QTextBrowser(this);
documentationView->setReadOnly(true);
- documentationView->setOpenExternalLinks(true);
+ documentationView->setOpenExternalLinks(false);
+ connect(documentationView, &QTextBrowser::anchorClicked, this, [](const QUrl &url) {
+ static const QSet< QString > allowedSchemes = {
+ QStringLiteral("http"),
+ QStringLiteral("https"),
+ };
+
+ if (!url.isRelative() && allowedSchemes.contains(url.scheme())) {
+ QDesktopServices::openUrl(url);
+ }
+ });
documentationView->setAccessibleName(tr("Documentation links"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mumble/Documentation.cpp` around lines 20 - 34, The documentationView
currently calls documentationView->setOpenExternalLinks(true) which allows any
URL scheme to be opened; change this to
documentationView->setOpenExternalLinks(false) and connect documentationView's
anchorClicked signal to a handler (e.g., onDocumentationAnchorClicked) that
inspects the QUrl scheme and only calls QDesktopServices::openUrl for "http" or
"https" schemes (otherwise reject/log the URL); apply the same pattern to the
analogous code in About.cpp.
|
|
||
| DocumentationDialog::DocumentationDialog(QWidget *parent) : QDialog(parent) { | ||
| setWindowTitle(tr("Mumble Documentation")); | ||
| resize(640, 520); |
There was a problem hiding this comment.
Hard-coded sizes are (almost) never a good idea. They will almost certainly break with certain system configurations.
| "Documentation.cpp" | ||
| "Documentation.h" |
There was a problem hiding this comment.
The file names should have Dialog in their name and they should probably be located under widgets.
There was a problem hiding this comment.
What is the benefit of creating a dedicated dialog class for this rather than using a plain QDialog and setting the text accordingly?
There was a problem hiding this comment.
You're right, dedicated class isn't really necessary. I can replace it with a plain QDialog set up inline at the call site.
| "<p>Reach the team through the <a href=\"https://www.mumble.info/contact/\">Contact page</a>.</p>")); | ||
|
|
||
| QPushButton *okButton = new QPushButton(tr("OK"), this); | ||
| connect(okButton, SIGNAL(clicked()), this, SLOT(accept())); |
There was a problem hiding this comment.
This should use the type-safe overload that takes function pointers and doesn't require the SIGNAL and SLOT macro.
There was a problem hiding this comment.
This shouldn't be part of any commit
There was a problem hiding this comment.
Sorry about that, I'll remove it.
2 participants
Summary
This pull request adds a new "Documentation" dialog to the Help menu in the Mumble client. The dialog provides users with direct links to official Mumble resources, including:
Implementation Details
Notes
Checks