Skip to content
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

Jump to bottom

Added the ability to change properties of UiActions #24834

Merged
merged 1 commit into from
Sep 20, 2024
Merged

Added the ability to change properties of UiActions #24834

merged 1 commit into from
Sep 20, 2024

Conversation

igorkorsukov
Copy link
Contributor

No description provided.

saintmatthieu
saintmatthieu reviewed Sep 20, 2024
View reviewed changes
Copy link

@saintmatthieu saintmatthieu left a comment

Choose a reason for hiding this comment

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

I'm surprised that the method to modify the actions isn't part of the abstract interface.
Besides that LGTM.

src/framework/ui/iuiactionsregister.h Outdated
virtual const std::vector<UiAction> getActions() const = 0;
virtual UiActionState actionState(const muse::actions::ActionCode& code) const = 0;
virtual async::Channel<muse::actions::ActionCodeList> actionStateChanged() const = 0;
virtual const std::vector<UiAction> actionList() const = 0;
Copy link

Choose a reason for hiding this comment

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

Why a const return value if it's not a reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed const
(I don't know why the author added const, probably wanted to somehow semantically show that there is no need to change this list...
I just renamed the method to match our style)

src/framework/ui/internal/uiactionsregister.h
@@ -69,6 +73,8 @@ class UiActionsRegister : public IUiActionsRegister, public Injectable, public a
void updateShortcuts(const actions::ActionCodeList& codes);
void updateShortcutsAll();

void updateActions(const UiActionList& actions);
Copy link

Choose a reason for hiding this comment

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

Not override ? Aren't clients supposed to interact with abstract IUiActionsRegister ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not override
the reasons are described here (number two)
#24834 (comment)

@igorkorsukov
Copy link
Contributor Author

I'm surprised that the method to modify the actions isn't part of the abstract interface. Besides that LGTM.

There are two reasons for this:

  1. We had no need to change the properties of actions for the user.
  2. It is made so that actions are managed and owned only by the module to which they belong, which processes them. Accordingly, there is no need to provide the possibility for someone from outside to change some action in some module.

@saintmatthieu
Copy link

Can't find the "Approve" button, but I approve :) Thanks !

@igorkorsukov igorkorsukov merged commit 58ff01f into musescore:master Sep 20, 2024
11 checks passed
@shoogle
Copy link
Contributor

shoogle commented Sep 20, 2024

Can't find the "Approve" button

Files changed tab > Review changes.

However, the 'approve' option has disappeared now that the PR is merged.

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

@saintmatthieu saintmatthieu saintmatthieu left review comments

@RomanPudashkin RomanPudashkin RomanPudashkin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@igorkorsukov @saintmatthieu @shoogle @RomanPudashkin