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 zooming and sliding of the waveform view in AudioFileProcessor #7377

Merged

Conversation

szeli1
Copy link
Contributor

@szeli1 szeli1 commented Jul 12, 2024

closes #7375

fixes partially #7074

I have fixed some issues with the graph:

  • sliding the start, loop, end indicator
  • sliding the waveshape
  • zooming in
  • possibly segfaults

The sliding and the zooming was separated in mouseMoveEvent, if the user starts sliding or zooming, they will no longer be able to zoom or slide until next mouse press.

@Rossmaxx
Copy link
Contributor

fixes partially #7074

can you fully fix it? with help from @michaelgregorius

Copy link
Contributor

@Rossmaxx Rossmaxx left a comment

Choose a reason for hiding this comment

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

some minor thoughts

plugins/AudioFileProcessor/AudioFileProcessorWaveView.cpp Outdated Show resolved Hide resolved
plugins/AudioFileProcessor/AudioFileProcessorWaveView.cpp Outdated Show resolved Hide resolved
@szeli1
Copy link
Contributor Author

szeli1 commented Jul 15, 2024

can you fully fix it? with help from @michaelgregorius

I would appreciate some help, but it isn't strictly needed.

Copy link
Contributor

@sakertooth sakertooth left a comment

Choose a reason for hiding this comment

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

Small code suggestions here. Will try to test later.

plugins/AudioFileProcessor/AudioFileProcessorWaveView.cpp Outdated Show resolved Hide resolved
plugins/AudioFileProcessor/AudioFileProcessorWaveView.cpp Outdated Show resolved Hide resolved
plugins/AudioFileProcessor/AudioFileProcessorWaveView.cpp Outdated Show resolved Hide resolved
plugins/AudioFileProcessor/AudioFileProcessorWaveView.h Outdated Show resolved Hide resolved
plugins/AudioFileProcessor/AudioFileProcessorWaveView.h Outdated Show resolved Hide resolved
plugins/AudioFileProcessor/AudioFileProcessorWaveView.h Outdated Show resolved Hide resolved
plugins/AudioFileProcessor/AudioFileProcessorWaveView.h Outdated Show resolved Hide resolved
@qnebra qnebra mentioned this pull request Aug 4, 2024
1 task
@bratpeki
Copy link
Contributor

I'll test this ASAP.

@bratpeki
Copy link
Contributor

When looking at this initially, it seems perfectly fine. I'll install it on my system at test it, you'll have more opinions from me in a few days.

@Rossmaxx says some users mentioned it didn't solve the issue for them. Would you like any info regarding my system and building process?

@szeli1
Copy link
Contributor Author

szeli1 commented Aug 16, 2024

@Rossmaxx says some users mentioned it didn't solve the issue for them. Would you like any info regarding my system and building process?

I'm hearing about this for the first time. I've changed how thing work so I'm not sure what is exactly the issue they are experiencing. I would like to hear info about what is the problem firstly. Thank you for reviewing it and communicating the issue. I would like to know how the testing goes.

@bratpeki
Copy link
Contributor

@szeli1, what do the second and third point mean? I would like to be sure I've tested them, so if you could elaborate on them more, that would be terrific!

@szeli1
Copy link
Contributor Author

szeli1 commented Aug 16, 2024

@szeli1, what do the second and third point mean? I would like to be sure I've tested them, so if you could elaborate on them more, that would be terrific!

In other words:
I need to know what is the issue that they are talking about ("says some users mentioned it didn't solve the issue for them"). Thank you for notifying me that there is an issue and thank you for deciding that you will test this. I meant to say this by the last 3 sentences.

@bratpeki
Copy link
Contributor

bratpeki commented Aug 16, 2024

Oh, no, not that, that's something @Rossmaxx mentioned on Discord. I'm talking about "sliding the waveshape" and "zooming in". I would like you to elaborate on those in a bit more detail, so I can make sure I have tested them properly.

Copy link
Contributor

@sakertooth sakertooth left a comment

Choose a reason for hiding this comment

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

Haven't tested it, but the changes look sane.

plugins/AudioFileProcessor/AudioFileProcessorWaveView.cpp Outdated Show resolved Hide resolved
@sakertooth
Copy link
Contributor

Just tested it, it works a lot better now 👍. We should still address the outstanding comments if possible before merge though.

@sakertooth sakertooth changed the title AudioFileProcessorWaveView fixed zooming and sliding Fix zooming and sliding of the waveform view in AudioFileProcessor Aug 16, 2024
@szeli1
Copy link
Contributor Author

szeli1 commented Aug 17, 2024

I'm talking about "sliding the waveshape" and "zooming in".

Dragging the waveshape left and right or up and down.

@bratpeki
Copy link
Contributor

LGTM, we've discussed the fact that sometimes sliding happens instead of zooming, and vice versa, but I believe this is the minor mouse/trackpad twitches, so it's fine.

If you still want to work on that, I'm more than willing to test it!

@szeli1 szeli1 requested a review from Rossmaxx August 19, 2024 07:56
Copy link
Contributor

@Rossmaxx Rossmaxx left a comment

Choose a reason for hiding this comment

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

Looks good so far.

@sakertooth sakertooth merged commit 5e697f0 into LMMS:master Aug 19, 2024
11 checks passed
@szeli1 szeli1 deleted the AudioFileProcessor_wave_view_clicking_issues branch August 19, 2024 19:01
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.

Buggy points in LMMS AudioFileProcessor
4 participants