TYPE(FEAT): ToggleFullScreen Shortcut #6943
Conversation
These changes were made to implement a toggle Fullscreen button. Toggle Fullscreen Button.
Walkthrough
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ 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: 2
🧹 Nitpick comments (1)
src/mumble/GlobalShortcutTypes.h (1)
66-66: Consider adding a static_assert for the new enum value.The
ToggleFullScreenenum value is correctly added at the end of the enum, following the instruction to "ONLY EVER APPEND TO THE END OF THE ENUM!".However, to maintain consistency with the existing static_asserts (lines 71-75) that guard against accidental insertions, consider adding a new assertion:
static_assert(AdaptivePush == 49, "You may only append to the end of the enum!"); +static_assert(ToggleFullScreen == 50, "You may only append to the end of the enum!"); } // namespace GlobalShortcutTypeThis would make it easier to catch mistakes if someone accidentally modifies the enum in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/mumble/GlobalShortcutTypes.h(1 hunks)src/mumble/MainWindow.cpp(1 hunks)src/mumble/MainWindow.h(1 hunks)
| gsToggleFullScreen = new GlobalShortcut(this, GlobalShortcutType::ToggleFullScreen, | ||
| tr("ToggleFullScreen", "Global Shortcut")); | ||
| gsToggleFullScreen->setObjectName("gsToggleFullScreen"); | ||
| gsToggleFullScreen->qsWhatsThis = tr("This will toggle FullScreen on/off"); |
There was a problem hiding this comment.
🧩 Analysis chain
Fix display name formatting and connect the shortcut to its handler.
There are two issues with this initialization:
-
Inconsistent display name: The display name
"ToggleFullScreen"(line 459) doesn't follow the naming convention used by other shortcuts in this file. Compare with:- Line 426:
"Toggle positional audio" - Line 430:
"Move back" - Line 325:
"Toggle search dialog"
It should be
"Toggle fullscreen"or"Toggle full screen"with spaces and consistent capitalization. - Line 426:
-
Missing handler connection: Unlike other global shortcuts (e.g., line 533 shows
gsMinimalconnected toqaConfigMinimal), I don't see wheregsToggleFullScreenis connected to its handler. The shortcut needs to be connected to either:- An action that toggles fullscreen mode, or
- A slot like
on_gsToggleFullScreen_triggeredthat implements the toggle logic
Apply this diff to fix the display name:
gsToggleFullScreen = new GlobalShortcut(this, GlobalShortcutType::ToggleFullScreen,
- tr("ToggleFullScreen", "Global Shortcut"));
+ tr("Toggle fullscreen", "Global Shortcut"));
gsToggleFullScreen->setObjectName("gsToggleFullScreen");
- gsToggleFullScreen->qsWhatsThis = tr("This will toggle FullScreen on/off");
+ gsToggleFullScreen->qsWhatsThis = tr("This will toggle fullscreen on/off");Additionally, please verify where the handler connection is made, or add the appropriate connection and implementation. For reference, see how other shortcuts are connected around line 531-533 in setupGui().
Run the following script to verify if the handler connection exists:
🏁 Script executed:
#!/bin/bash
# Search for connections to gsToggleFullScreen
rg -n "gsToggleFullScreen.*connect" --type=cpp
rg -n "connect.*gsToggleFullScreen" --type=cppLength of output: 0
🏁 Script executed:
#!/bin/bash
rg -n "ToggleFullScreen" --type=cpp -C3 src/mumble/MainWindow.cpp
rg -n "GlobalShortcutType::ToggleFullScreen" --type=cpp -C3 src/mumble/MainWindow.cpp
rg -n "connect" --type=cpp -C3 src/mumble/MainWindow.cppLength of output: 20501
Fix display name, tooltip, and add missing gsToggleFullScreen handler connection
- Rename display name and tooltip at lines 459 and 461 in src/mumble/MainWindow.cpp:
@@ src/mumble/MainWindow.cpp:458
- tr("ToggleFullScreen", "Global Shortcut")
+ tr("Toggle full screen", "Global Shortcut")
@@ src/mumble/MainWindow.cpp:461
- qsWhatsThis = tr("This will toggle FullScreen on/off");
+ qsWhatsThis = tr("This will toggle full screen on/off");- In setupGui(), immediately after initializing gsToggleFullScreen, add:
connect(gsToggleFullScreen, SIGNAL(down(QVariant)), this, SLOT(on_gsToggleFullScreen_triggered(QVariant)));Implement on_gsToggleFullScreen_triggered(...) to toggle the window’s fullscreen state.
🤖 Prompt for AI Agents
In src/mumble/MainWindow.cpp around lines 458 to 461, the GlobalShortcut
gsToggleFullScreen needs its display name and tooltip corrected and a missing
signal connection plus handler implemented: rename the tr(...) display name at
line 459 to a clearer label (e.g., "Toggle Fullscreen" instead of
"ToggleFullScreen"), update qsWhatsThis at line 461 to a proper tooltip (e.g.,
"Toggle fullscreen mode"), then in setupGui() immediately after creating
gsToggleFullScreen add a connect call to bind its down(QVariant) signal to a new
slot on_gsToggleFullScreen_triggered(QVariant), and implement that slot to
toggle the main window's fullscreen state (enter fullscreen if not fullscreen,
exit if currently fullscreen) using the window's existing
showFullScreen/showNormal or appropriate Qt window state methods.
| GlobalShortcut *gsMoveBack; | ||
| GlobalShortcut *gsCycleListenerAttenuationMode, *gsListenerAttenuationUp, *gsListenerAttenuationDown; | ||
| GlobalShortcut *gsAdaptivePush; | ||
| GlobalShortcut *gsToggleFullScreen; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that the handler slot is implemented.
The declaration of gsToggleFullScreen follows the correct pattern. However, I don't see a corresponding handler slot declaration (e.g., on_gsToggleFullScreen_triggered) in this header file. Other global shortcuts typically have handler slots declared in the public slots: section (see examples like on_gsToggleSearch_triggered at line 335 or on_gsToggleTalkingUI_triggered at line 334).
Please verify that:
- A handler slot is declared for this shortcut
- The shortcut is properly connected to its handler in the implementation
- The handler implements the actual fullscreen toggle functionality
Run the following script to check if the handler is implemented:
🏁 Script executed:
#!/bin/bash
# Search for the handler slot declaration and implementation
rg -n "on_gsToggleFullScreen" --type=cpp --type=hLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for any references to gsToggleFullScreen across the repository
rg -n "gsToggleFullScreen" -C2Length of output: 919
🏁 Script executed:
#!/bin/bash
# Show context around gsToggleFullScreen instantiation and any connect calls
rg -n -C5 "gsToggleFullScreen" src/mumble/MainWindow.cppLength of output: 685
🏁 Script executed:
#!/bin/bash
rg -n "connectSlotsByName" src/mumble/MainWindow.cpp
rg -n -C3 "on_gsToggleSearch_triggered" src/mumble/MainWindow.hLength of output: 412
Add missing fullscreen toggle slot
No slot on_gsToggleFullScreen_triggered(bool, QVariant) exists to handle the gsToggleFullScreen shortcut. Declare it under public slots: in MainWindow.h and implement it in MainWindow.cpp (or explicitly connect the shortcut’s triggered signal to your handler).
🤖 Prompt for AI Agents
In src/mumble/MainWindow.h around line 118, the GlobalShortcut pointer
gsToggleFullScreen is declared but there is no slot to handle its triggered
signal; add a declaration for on_gsToggleFullScreen_triggered(bool, QVariant)
under the public slots: section of MainWindow.h and then implement the
corresponding method in src/mumble/MainWindow.cpp to toggle fullscreen (or
alternatively, in the constructor connect gsToggleFullScreen's triggered(bool,
QVariant) signal to an existing handler), making sure the signature and
QObject::connect types match and to include any necessary headers.
|
As the AI review already stated: this doesn't implement the underlying functionality for this festure |
2 participants
These changes were made to implement a toggle Fullscreen button.
Toggle Fullscreen Button.
Checks