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

Fix issue with "Decrease duration" and "Increase duration" in the History panel #26738

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix issue with "Decrease duration" and "Increase duration" in the History panel #26738

wants to merge 1 commit into from
+15 −15

Conversation

Jojo-Schmitz
Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz commented Feb 25, 2025 •
edited
Loading

Resolves: #26733

(Needed for 4.5.0 too)

@Jojo-Schmitz Jojo-Schmitz mentioned this pull request Feb 25, 2025
Merged
@Jojo-Schmitz
Copy link
Contributor Author

Alternative would be to revese the logic (to something logical ;-)) in 4 other places, let me know if that is wanted instead

@bkunda bkunda requested a review from mathesoncalum February 25, 2025 15:26
@cbjeukendrup
Copy link
Contributor

Perhaps we reverse the logic for Master, and go with the super simple safe version for 4.5?

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Feb 25, 2025 •
edited
Loading

I'm fine with that, have a corresponding commit ready, for master and for 4.5.0, but will commit it only to the former for now.

Maybe you change your mind on 4.5.0, seeing how simple that is too.

@Jojo-Schmitz Jojo-Schmitz force-pushed the duration-changes branch 2 times, most recently from fd2f3af to 092db9c Compare February 25, 2025 16:42
Jojo-Schmitz
Jojo-Schmitz commented Feb 25, 2025
View reviewed changes
src/braille/internal/notationbraille.cpp
@@ -683,7 +683,7 @@ void NotationBraille::setKeys(const QString& sequence)
case BieSequencePatternType::Dot: {
LOGD() << "dot " << brailleInput()->dots();
if (brailleInput()->dots() > 0) {
interaction()->increaseDecreaseDuration(-brailleInput()->dots(), true);
interaction()->increaseDecreaseDuration(brailleInput()->dots(), true);
Copy link
Contributor Author

@Jojo-Schmitz Jojo-Schmitz Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This raises the question whether/where for Braille there is a "decrease duration"?

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Feb 25, 2025 •
edited
Loading

Oops, it is not as simple as I thought, cmdIncDecDuration() needs to get changed too, and more...

@Jojo-Schmitz Jojo-Schmitz force-pushed the duration-changes branch 2 times, most recently from 0bd890b to 5d21ccc Compare February 25, 2025 16:59
@Jojo-Schmitz
Copy link
Contributor Author

I hope to not having screwed up the logic...

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Feb 25, 2025 •
edited
Loading

Apparently I did screw up... utests and vtests failing :-(

@Jojo-Schmitz
Copy link
Contributor Author

Back to the simple fix?

@cbjeukendrup
Copy link
Contributor

Strange, your changes actually look fine to me. Also it's remarkable that the VTests are failing. I can hardly believe that the changes from this PR are causing this...

@Jojo-Schmitz
Copy link
Contributor Author

I also did a rebase...

@Jojo-Schmitz
Copy link
Contributor Author

Trying another rebase

@Jojo-Schmitz
Copy link
Contributor Author

same issue :-(

@cbjeukendrup
Copy link
Contributor

I found the problem: turns out TDuration::shiftType is used in some other places too. So those would need updating too. And that will result in other changes elsewhere again... you could try whether that's doable, but if it becomes unmanageable we might just go back to the simple fix.

Also, the reason why the logic was the way it was became more clear to me: it corresponded to the order of the cases of the DurationType enum. Of course, that could be turned upside down as well, but I'm not sure if we should go that far (who knows what consequences that will have...)

@Jojo-Schmitz
Copy link
Contributor Author

I found only one such place (and there twice), which I missed earlier. Added. Let's see...

@Jojo-Schmitz
Copy link
Contributor Author

Better (vtests pass, no utest crashes), but not there yet (2 MusicXML tests fail, apparently wanting additional beams at some tuplets, and 4 engraving tests, also around tuplets, changing their base note, making them longer by 2 steps, e.g. 8th to half, 16th to quarter).

@Jojo-Schmitz
Copy link
Contributor Author

As a) time seems running out on 4.5.0 and b) this change here is way more complex, I think it'd be good to merge the simple one, #26739, to 4.5.0 now.
This one here has got more time to ripe ;-)

@mathesoncalum mathesoncalum requested review from cbjeukendrup and removed request for mathesoncalum February 26, 2025 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbjeukendrup cbjeukendrup Awaiting requested review from cbjeukendrup

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

Successfully merging this pull request may close these issues.

Issue with "Decrease duration" and "Increase duration" in the History panel
2 participants
@Jojo-Schmitz @cbjeukendrup