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

FIXES #4070 The Pitch piemenu do not remember the last selected accidental value #4071

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

omsuneri
Copy link
Contributor

@omsuneri omsuneri commented Nov 21, 2024

fixing issue #4070
Now the pitch piemenu remembers the last selected accidental value
custom pitch selected ;
Screenshot 2024-11-22 at 2 02 21 AM
the wheel represents the same value as selected ;
Screenshot 2024-11-22 at 2 02 32 AM


// Store the selected accidental in the block for later use.
prevAccidental = selection["attr"];
block.prevAccidental = prevAccidental;
Copy link
Member

Choose a reason for hiding this comment

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

This retains the value during the session, but not between sessions. Maybe we need to think about saving/restoring this attribute with the block (much the way we do with the start block attributes).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like do you want the pitch block to save the data for last session like this @walterbender

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So what should I change in this pr regarding the issue @walterbender

Copy link
Member

Choose a reason for hiding this comment

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

I think you don't need to change anything in what you have already done, but in this function in activity.js, this.prepareExport, you can see how some attributes are stored in the start block. We'll want to do something similar with the pitch block. And then restore the state on project import (in loadNewBlocks in blocks.js.

But maybe all that complexity can be dealt with in a separate PR. Let me test this code as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay @walterbender i ll make a new PR in regards to these changes and will resolve soon.

@walterbender
Copy link
Member

Hmm. One problem. It does remember the accidental selection, but when I leave the pie menu the 2nd time, it does not display the accident in the pitch block.

Screenshot From 2024-11-21 17-00-50
Screenshot From 2024-11-21 17-00-33
Screenshot From 2024-11-21 17-01-06

@omsuneri
Copy link
Contributor Author

@walterbender yaa i see this issue will make it resolved soon

@omsuneri
Copy link
Contributor Author

@walterbender i m not getting that why this happen so as when you change the pitch combination after every time you opens the piemenu it works fine but when you click and dont change the attributes it dont print the accidental value
i think this might be the issue causing due to the state of the block better you merge this PR and I ll create an another issue for the same or should i do changes in this PR only

@walterbender
Copy link
Member

It may be a caching problem with the artwork. In any case, I think we should try to fix it in this PR.

@walterbender
Copy link
Member

Well, maybe not. I think the value is getting reset.

@omsuneri
Copy link
Contributor Author

@walterbender I was trying to fix it in the new pr

@omsuneri
Copy link
Contributor Author

Or should I create a new ticket for this issue as what you would recommend

@walterbender
Copy link
Member

I think it is related to this MR, so I think it should be fixed here.

@omsuneri
Copy link
Contributor Author

@walterbender okay so I ll be updating this PR soon to resolve all the issues

@walterbender
Copy link
Member

In the old version, the selector would be reset to the mode, so if you entered and left w/o clicking, the accidental would be reset. In your patch, it doesn't change to the mode after the first time in the selector, but it seems to reset the value anyway.

@omsuneri
Copy link
Contributor Author

Okay I ll make the changes accordingly

by this commit the note name get displayed coreectly even after no selection after the piemenu is opened
@omsuneri
Copy link
Contributor Author

@walterbender now when the user did not select the any value and simple closes the pie menu the block still displayes the name of the accidental correctly

@omsuneri
Copy link
Contributor Author

@walterbender you can see the changes here

Screen.Recording.2024-11-24.at.12.28.04.AM.mov

if (selectedAccidental === "♮" || selectedAccidental === "") {
that.text.text = selectedNote; // Natural or no accidental
} else {
that.text.text = selectedNote + selectedAccidental; // Combine note and accidental
Copy link
Member

Choose a reason for hiding this comment

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

please fix the indentation

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.

2 participants