Skip to content

added compat actions to navigation controller #150

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.

Sign up for GitHub

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

Merged
igorkorsukov merged 1 commit into from
Jul 14, 2026
Merged

added compat actions to navigation controller #150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions framework/ui/internal/navigationcontroller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
#define MYLOG() LOGN()
#endif

using namespace muse;
using namespace muse::ui;

static const muse::UriQuery DEV_SHOW_CONTROLS_URI("muse://devtools/keynav/controls?modal=false");
Expand Down Expand Up @@ -300,6 +301,33 @@ void NavigationController::init()

d->onRequest(this, TRIGGER_CONTROL_COMMAND, [this]() { doTriggerControl(); return muse::make_ok(); });

// compat
{
static const std::map<std::string, rcommand::Command> compatActionToCommand = {
{ "nav-next-section", NEXT_SECTION_COMMAND },
{ "nav-prev-section", PREV_SECTION_COMMAND },
{ "nav-next-panel", NEXT_PANEL_COMMAND },
{ "nav-prev-panel", PREV_PANEL_COMMAND },
{ "nav-next-tab", NEXT_PANEL_COMMAND },
{ "nav-prev-tab", PREV_PANEL_COMMAND },
{ "nav-trigger-control", TRIGGER_CONTROL_COMMAND },
{ "nav-right", RIGHT_COMMAND },
{ "nav-left", LEFT_COMMAND },
{ "nav-up", UP_COMMAND },
{ "nav-down", DOWN_COMMAND },
{ "nav-escape", ESCAPE_COMMAND },
{ "nav-first-control", FIRST_CONTROL_COMMAND },
{ "nav-last-control", LAST_CONTROL_COMMAND },
{ "nav-nextrow-control", NEXTROW_CONTROL_COMMAND },
{ "nav-prevrow-control", PREVROW_CONTROL_COMMAND },
};

auto ad = actionsDispatcher();
for (const auto& [action, command] : compatActionToCommand) {
ad->reg(this, action, [d, command]() { d->dispatch(command); });
}
}
Comment on lines +325 to +329

@coderabbitai coderabbitai Bot Jul 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Ensure Actionable::setDispatcher is called to guarantee cleanup.

The NavigationController inherits from actions::Actionable, which relies on its m_dispatcher member to automatically unregister actions in ~Actionable(). Since you are manually registering actions with ad->reg(...), you need to explicitly set the dispatcher so the base class can properly clean up these registrations when the controller is destroyed.

  • framework/ui/internal/navigationcontroller.cpp#L325-L329: Call setDispatcher(ad); right after retrieving the dispatcher to ensure the base class has the pointer for cleanup.
  • framework/ui/internal/navigationcontroller.h#L41-L45: NavigationController inherits actions::Actionable here, inherently taking on this lifecycle requirement.
🛠️ Proposed fix 
         auto ad = actionsDispatcher();
+        setDispatcher(ad);
         for (const auto& [action, command] : compatActionToCommand) {
             ad->reg(this, action, [d, command]() { d->dispatch(command); });
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
auto ad = actionsDispatcher();
for (const auto& [action, command] : compatActionToCommand) {
ad->reg(this, action, [d, command]() { d->dispatch(command); });
}
}
auto ad = actionsDispatcher();
setDispatcher(ad);
for (const auto& [action, command] : compatActionToCommand) {
ad->reg(this, action, [d, command]() { d->dispatch(command); });
}
}
🧰 Tools
🪛 Clang (14.0.6)

[warning] 325-325: variable name 'ad' is too short, expected at least 3 characters

(readability-identifier-length)

📍 Affects 2 files
  • framework/ui/internal/navigationcontroller.cpp#L325-L329 (this comment)
  • framework/ui/internal/navigationcontroller.h#L41-L45
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@framework/ui/internal/navigationcontroller.cpp` around lines 325 - 329, Set
the dispatcher immediately after obtaining it in the NavigationController
initialization flow, before manually registering actions, so Actionable can
unregister them during destruction. Specifically update
framework/ui/internal/navigationcontroller.cpp lines 325-329 around
actionsDispatcher() and reg(); framework/ui/internal/navigationcontroller.h
lines 41-45 only documents the Actionable inheritance and requires no direct
change.


qApp->installEventFilter(this);
}

Expand Down
5 changes: 4 additions & 1 deletion framework/ui/internal/navigationcontroller.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
#include "modularity/ioc.h"
#include "rcommand/commandable.h"
#include "rcommand/icommanddispatcher.h"
#include "actions/actionable.h"
#include "actions/iactionsdispatcher.h"
#include "async/asyncable.h"
#include "ui/imainwindow.h"
#include "interactive/iinteractive.h"
Expand All @@ -36,10 +38,11 @@

namespace muse::ui {
class NavigationController : public QObject, public INavigationController, public Contextable, public async::Asyncable,
public rcommand::Commandable
public rcommand::Commandable, public actions::Actionable
{
public:
ContextInject<rcommand::ICommandDispatcher> dispatcher = { this };
ContextInject<actions::IActionsDispatcher> actionsDispatcher = { this };
ContextInject<IInteractive> interactive = { this };
ContextInject<IMainWindow> mainWindow = { this };

Expand Down
1 change: 1 addition & 0 deletions framework/ui/tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ set(MODULE_TEST_SRC

set(MODULE_TEST_LINK
muse_rcommand
muse_actions
muse_ui
# Linking a non-qml module (or its tests) to its qml counterpart should
# generally be avoided, but for the ui module we have to make an exception.
Expand Down
4 changes: 3 additions & 1 deletion framework/ui/tests/environment.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,12 @@

#include "log.h"
#include "framework/rcommand/rcommandmodule.h"
#include "framework/actions/actionsmodule.h"

static muse::testing::SuiteEnvironment ui_senv(
{
new muse::rcommand::RCommandModule()
new muse::rcommand::RCommandModule(),
new muse::actions::ActionsModule()
},
nullptr,
[]() {
Expand Down
4 changes: 4 additions & 0 deletions framework/ui/tests/navigationcontroller_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

#include "ui/internal/navigationcontroller.h"
#include "rcommand/internal/commanddispatcher.h"
#include "actions/internal/actionsdispatcher.h"
#include "ui/navigationcommands.h"

#include "global/tests/mocks/applicationmock.h"
Expand All @@ -54,7 +55,9 @@ class Ui_NavigationControllerTests : public ::testing::Test
m_controller = std::make_shared<NavigationController>(nullptr);

m_dispatcher = std::make_shared<rcommand::CommandDispatcher>();
m_actionsDispatcher = std::make_shared<actions::ActionsDispatcher>(iocCtx);
m_controller->dispatcher.set(m_dispatcher);
m_controller->actionsDispatcher.set(m_actionsDispatcher);

m_mainWindow = std::make_shared<ui::MainWindowMock>();
ON_CALL(*m_mainWindow, qWindow()).WillByDefault(Return(&m_window));
Expand Down Expand Up @@ -294,6 +297,7 @@ class Ui_NavigationControllerTests : public ::testing::Test

std::shared_ptr<NavigationController> m_controller;
std::shared_ptr<rcommand::CommandDispatcher> m_dispatcher;
std::shared_ptr<actions::ActionsDispatcher> m_actionsDispatcher;
std::shared_ptr<MainWindowMock> m_mainWindow;
std::shared_ptr<muse::ApplicationMock> m_applicationMock;

Expand Down