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.

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

Issue #21263 allow typing dynamics from keyboard #22607

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Earlgrey99
Copy link

Resolves: #21263

In the future I plan to implement this feature exactly as requested. However, I could not implement adding dynamics through text because of limited time and limited understanding. Instead I added support for adding a piano dynamic and adding a forte dynamic with Shift+Alt+P and Shift+Alt+F, respectively

  • [ x] I signed the CLA
  • [ x] The title of the PR describes the problem it addresses
  • [ x] Each commit's message describes its purpose and effects, and references the issue it resolves
  • If changes are extensive, there is a sequence of easily reviewable commits
  • [ x] The code in the PR follows the coding rules
  • [ x] There are no unnecessary changes
  • [x ] The code compiles and runs on my machine, preferably after each commit individually
  • [x ] I created a unit test or vtest to verify the changes I made (if applicable)

<key>add-piano</key>
<seq>Alt+Shift+P</seq>
</SC>
<SC>
Copy link
Contributor

Choose a reason for hiding this comment

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

These 2-key shortcuts should work too:

  <SC>
    <key>add-pianissimo</key>
    <seq>Alt+Shift+P,Alt+Shift+P</seq>
    </SC>
  <SC>
  <SC>
    <key>add-mezzo-piano</key>
    <seq>Alt+Shift+M,Alt+Shift+P</seq>
    </SC>
  <SC>
  <SC>
    <key>add-mezzo-forte</key>
    <seq>Alt+Shift+M,Alt+Shift+F</seq>
    </SC>
  <SC>
  <SC>
    <key>add-fortissimo</key>
    <seq>Alt+Shift+F,Alt+Shift+F</seq>
    </SC>

with corresponding commands in the code of course

@@ -25,6 +25,7 @@
#include "async/notification.h"
#include "notationtypes.h"
#include "types/retval.h"
#include "engraving\types\types.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Causes build faliures

Copy link
Contributor

Choose a reason for hiding this comment

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

The correct way would be

Suggested change
#include "engraving\types\types.h"
#include "engraving/types/types.h"

but it looks like you don't need this #include at all, because you use the typedef from notationtypes.h.

Copy link
Contributor

@cbjeukendrup cbjeukendrup left a comment

Choose a reason for hiding this comment

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

This PR contains a lot of commented or unused code, and it is unclear whether it is supposed to be functional or not. Please remove the unused code and fix the build problem.


//undoAddElement(dy);

Articulation* na = Factory::createArticulation(this->dummy()->chord());
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that you are abusing Articulations to show dynamics. Why not use the Dynamic class?

@@ -25,6 +25,7 @@
#include "async/notification.h"
#include "notationtypes.h"
#include "types/retval.h"
#include "engraving\types\types.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

The correct way would be

Suggested change
#include "engraving\types\types.h"
#include "engraving/types/types.h"

but it looks like you don't need this #include at all, because you use the typedef from notationtypes.h.

@cbjeukendrup cbjeukendrup marked this pull request as draft May 4, 2024 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow typing dynamics from the keyboard
3 participants