-
Notifications
You must be signed in to change notification settings - Fork 4
App features #894
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
App features #894
Conversation
…gle notifications
- Added summary item - Added HTML description
- Improve app feature HTML files - Add const action getters in the abstract settings manager - The user must give explicit consent after reading app feature description
…features are enabled
…earning center pages when the corresponding app features are enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request introduces a comprehensive app features system to manage optional downloadable content in ManiVault Studio. The changes provide users with granular control over which dynamic features they want to enable, including tutorials, videos, projects, and error logging.
Key Changes:
- Introduces a new
AppFeatureActionbase class and specific feature actions (Videos, Tutorials, Projects, ErrorLogging) to manage optional app features - Refactors error logging settings into the new app features framework
- Renames toggle action variables throughout the codebase to include "Section" suffix for improved clarity and consistency
Reviewed changes
Copilot reviewed 50 out of 50 changed files in this pull request and generated 36 comments.
Show a summary per file
| File | Description |
|---|---|
| ManiVault/src/actions/AppFeatureAction.h/cpp | New base class for managing app features with consent dialogs and enable/disable functionality |
| ManiVault/src/actions/*AppFeatureAction.h/cpp | Individual app feature implementations for Videos, Tutorials, Projects, and ErrorLogging |
| ManiVault/src/AppFeaturesSettingsAction.h/cpp | New settings action that groups all app features together |
| ManiVault/src/private/SettingsManager.h/cpp | Adds app features settings and removes deprecated error logging settings |
| ManiVault/src/AbstractSettingsManager.h | Updates interface with const overloads and app features support |
| ManiVault/src/private/StartPageContentWidget.h/cpp | Renames toggle actions with "Section" suffix and integrates tutorials app feature visibility |
| ManiVault/src/private/StartPageGetStartedWidget.cpp | Adds conditional tutorials widget visibility based on app feature setting |
| ManiVault/src/private/LearningPageContentWidget.h/cpp | Renames actions and integrates videos/tutorials app feature visibility controls |
| ManiVault/src/private/HelpMenu.cpp | Conditionally adds videos and tutorials menus based on app feature settings |
| ManiVault/src/private/HelpManager.cpp | Updates to support videos DSN synchronization and app feature-based visibility |
| ManiVault/src/models/LearningCenterVideosModel.h/cpp | Adds DSN synchronization capability and app feature integration |
| ManiVault/src/models/LearningCenterTutorialsModel.cpp | Integrates tutorials app feature check in DSN synchronization |
| ManiVault/src/actions/StringAction.h/cpp | Adds new TextBrowser widget type for HTML content display |
| ManiVault/src/actions/GroupAction.h/cpp | Removes unnecessary virtual keywords and improves code formatting |
| ManiVault/src/private/ErrorManager.cpp | Refactors error logging integration (work in progress with TODOs) |
| ManiVault/src/private/AppFeaturesDialog.h/cpp | Renamed from ErrorLoggingConsentDialog to support all app features |
| ManiVault/res/html/AppFeature*.html | New HTML resources describing each app feature for consent dialogs |
| ManiVault/src/PluginFactory.h/cpp | Adds videos DSN action for plugin-provided video content |
| ManiVault/src/CoreInterface.h | Adds constSettings() convenience function for const access |
| ManiVault/cmake/*.cmake | Updates CMake configuration to include new app feature files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| addMenu(mv::help().getVideosMenu()); | ||
|
|
||
| if (tutorialsAppFeatureEnabledAction.isChecked()) | ||
| addMenu(mv::help().getTutorialsMenu()); |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent whitespace detected. This line uses a tab character for indentation while the surrounding code uses spaces. Please use consistent spacing throughout the file.
| //TODO | ||
| //public: // Action getters | ||
| // | ||
| // const ToggleAction& getTutorialsAction() const { return _tutorialsAction; } | ||
| // const ToggleAction& getProjectsAction() const { return _projectsAction; } | ||
| // | ||
| //private: | ||
| // ToggleAction _tutorialsAction; /** Toggle tutorials dynamic content */ | ||
| // ToggleAction _projectsAction; /** Toggle projects dynamic content */ |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incomplete TODO comments. These comments in the file indicate work in progress but provide no context about what needs to be done. Please either complete the implementation or add details about what's needed.
| gui::AppearanceSettingsAction& getAppearanceSettingsAction() override { return _appearanceSettingsAction; }; | ||
| gui::TemporaryDirectoriesSettingsAction& getTemporaryDirectoriesSettingsAction() override { return _temporaryDirectoriesSettingsAction; }; | ||
| gui::ErrorLoggingSettingsAction& getErrorLoggingSettingsAction() override { return _errorLoggingSettingsAction; }; | ||
| //gui::AppFeaturesSettingsAction& getAppFeaturesSettingsAction() override { return _appFeaturesSettingsAction; } |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented-out code. The line contains a commented-out method declaration. If this is no longer needed, it should be removed. If it's still needed, please add a comment explaining why it's temporarily disabled.
| connect(&tutorialsAppFeatureEnabledAction, &ToggleAction::toggled, this, [this](bool toggled) -> void { | ||
| if (toggled) | ||
| _toggleTutorialsSectionAction.setChecked(true); | ||
| }); |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent whitespace detected. This line uses a tab character for indentation while the surrounding code uses spaces. Please use consistent spacing throughout the file.
| } | ||
| }); | ||
| } | ||
|
|
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent whitespace detected. This line uses a tab character for indentation while the surrounding code uses spaces. Please use consistent spacing throughout the file.
| connect(&tutorialsAppFeatureEnabledAction, &ToggleAction::toggled, this, [this](bool toggled) -> void { | ||
| if (toggled) | ||
| _toggleTutorialsSectionAction.setChecked(true); | ||
| }); |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential dangling reference issue. The lambda captures tutorialsAppFeatureEnabledAction by reference and is connected to a signal. If this lambda outlives the scope where tutorialsAppFeatureEnabledAction is defined, the reference will be invalid. Consider capturing by value or ensuring the lambda's lifetime is properly managed.
| const auto toggleVisibility = [tutorialsMenu, &tutorialsAppFeatureEnabledAction]() -> void { | ||
| tutorialsMenu->setEnabled(tutorialsAppFeatureEnabledAction.isChecked()); |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential dangling reference issue. The lambda captures tutorialsAppFeatureEnabledAction by reference and is connected to a signal. If this lambda outlives the scope where tutorialsAppFeatureEnabledAction is defined, the reference will be invalid. Consider capturing by value or ensuring the lambda's lifetime is properly managed.
| // TODO | ||
| //errorLoggingSettingsAction.addAction(&getLoggingAskConsentDialogAction()); | ||
| //errorLoggingSettingsAction.addAction(&getLoggingEnabledAction()); | ||
| //errorLoggingSettingsAction.addAction(&getLoggingDsnAction()); | ||
| //errorLoggingSettingsAction.addAction(&getLoggingShowCrashReportDialogAction()); |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incomplete TODO comments. These comments indicate work in progress but provide no context about what needs to be done. Please either complete the implementation or add details about what's needed.
| // TODO | ||
| //public: // Action getters | ||
| // | ||
| // const ToggleAction& getTutorialsAction() const { return _tutorialsAction; } | ||
| // const ToggleAction& getProjectsAction() const { return _projectsAction; } | ||
| // | ||
| //private: | ||
| // ToggleAction _tutorialsAction; /** Toggle tutorials dynamic content */ | ||
| // ToggleAction _projectsAction; /** Toggle projects dynamic content */ |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incomplete TODO comments. These comments in the file indicate work in progress but provide no context about what needs to be done. Please either complete the implementation or add details about what's needed.
| } | ||
|
|
||
| updateActionsReadOnly(); | ||
| //mv::help().addNotification("App Feature", QString("%1 app feature has been %2").arg(text(), toggled ? "enabled" : "disabled"), StyledIcon(toggled ? "toggle-on" : "toggle-off")); |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented-out notification code. This line contains a commented-out call to add a notification. If this feature is not yet ready for production, please add a comment explaining when/why it should be enabled. Otherwise, if it's no longer needed, it should be removed.
| //mv::help().addNotification("App Feature", QString("%1 app feature has been %2").arg(text(), toggled ? "enabled" : "disabled"), StyledIcon(toggled ? "toggle-on" : "toggle-off")); |
No description provided.