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

Fix glitch in undo/redo of note edits via the menu #5789

Merged
merged 2 commits into from
Nov 19, 2020

Conversation

thmueller64
Copy link
Contributor

Addresses #5722.

@LmmsBot
Copy link

LmmsBot commented Nov 16, 2020

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

Windows

macOS

🤖
{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://10524-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.14%2Bgf24b32e-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10524?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://10520-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.14%2Bgf24b32e41-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10520?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://10522-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.14%2Bgf24b32e41-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10522?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/oehvudo5cqu5gf66/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36343611"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/4pcqdclsb455tjx9/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36343611"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://10523-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.14%2Bgf24b32e41-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10523?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "f61aed0364fcba740c2a4f884cc8d64885816d6b"}

@zonkmachine zonkmachine self-requested a review November 16, 2020 20:24
@zonkmachine
Copy link
Member

I've tested this and it fixes #5722

@Veratil
Copy link
Contributor

Veratil commented Nov 16, 2020

@zonkmachine can you make sure this doesn't interfere with any other normal operation?

@zonkmachine
Copy link
Member

@zonkmachine can you make sure this doesn't interfere with any other normal operation?

Yes, I need to test if the commit that caused this issue still works.

@superpaik
Copy link
Contributor

Undo via menu works well. Via hotkey too.
But there is something weird with redo. Via menu works. But via hotkeys there is a strange behaviour. Steps:

  • put three notes on the piano roll.
  • undo via menu (last note is deleted) OK
  • redo via hotkey doesn't work. KO
  • undo again via menu (second note is deleted) OK
  • redo via hotkey works well (second note appears) redo via hotkey again (last note appears) OK

@DomClark DomClark linked an issue Nov 17, 2020 that may be closed by this pull request
@thmueller64
Copy link
Contributor Author

@superpaik : That's strange indeed. I can also reproduce this with the current master, so it appears not to be related to this PR. Maybe file another issue if you can also confirm this on master?

@superpaik
Copy link
Contributor

@thmueller64 Yes, it happens in the master as well, so it's not related to this PR.
I won't create another issue because there are few already related to undo/redo and the piano roll, and I've been told that this functionality (undo/redo) needs some kind of refactoring because of it's glitches.
thanks!

@zonkmachine
Copy link
Member

@zonkmachine can you make sure this doesn't interfere with any other normal operation?

Yes, I need to test if the commit that caused this issue still works.

Some testing. At a quick workthrough everything looks fine to me. I'm not sure I tested everything related to 5d7e672 . I don't have time to do a deeper check right now but I don't think that's necessary. I think this is fine to merge.

@zonkmachine zonkmachine merged commit 87875a1 into LMMS:master Nov 19, 2020
@zonkmachine
Copy link
Member

I could replicate the issue that @superpaik describes but only once. It's definitely intermittent for me.
Merging!

IanCaio pushed a commit to IanCaio/lmms that referenced this pull request Mar 28, 2021
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
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.

Undo/Redo of note edits via the menu is glitchy
6 participants