Change behaviour for changing volume/pan on Piano #1804
Conversation
| if( firstNote ) | ||
| { | ||
| // Emit pattern has changed | ||
| m_pattern->dataChanged(); |
There was a problem hiding this comment.
I moved this here because it's not clear to me why should I emit this signal for each note. Let me know if I am mistaken.
There was a problem hiding this comment.
what is m_pattern->dataChanged(); connected to? That would help determine if this is needed per note or just once.
There was a problem hiding this comment.
I can't actually find where this connection is made, this is weird. I can't find any connection to a Pattern::dataChanged call.
Can you help me? I was git blameing this and this is really old code, from 2008, I guess we can even delete this method declaration if this is not used.
There is a recurring pattern on LMMS where a dataChanged emission leads to an update call, but this is not happening here.
It looks like a long time ago @tobydox made a huge refactoring using signals and slots and this is probably an accident where he created this signal but never actually used it. See 5d5ad19
There was a problem hiding this comment.
Ok, so after pondering if I should delete this or not I decided not to touch this right now. But from my investigations it looks like this change should not be a problem.
There was a problem hiding this comment.
Can you help me?
I will take a look now
There was a problem hiding this comment.
Isn't dataChanged() to flag the project as needing to be saved?
I'm not very good with signals and slots yet, but Pattern.h has a line that says using Model::dataChanged(). Does that expose the slot?
Here's Model::dataChanged() https://github.com/LMMS/lmms/blob/master/include/Model.h#L80.
Sorry if this information is redundant with your findings.
There was a problem hiding this comment.
This signal is connected to quantizeChanged() which simply calls update() to redraw the widget. I would agree this only needs to be triggered once.
This would explain why the float text no longer flashes with your patch :)
There was a problem hiding this comment.
The object that is connected to quantizeChanged is not of class Pattern
badosu
force-pushed
the
multiple-volume-changes
branch
4 times, most recently
from
860a3c0 to
e9f0874
Compare
| } | ||
|
|
||
| // Emit pattern has changed | ||
| m_pattern->dataChanged(); |
There was a problem hiding this comment.
This should not be a problem since clearNotes just emits this after deleting all notes, presuming it's correctly implemented.
|
dataChanged() signal is used for signalling the attached PatternView that something has changed and it has to be updated. Not sure whether this is required in the place mentioned above. |
|
@tobydox Yep, this makes sense but can you find out where this connection actually happens? Because I could not. |
|
I had to search a bit as well but now I found it. The connect happens in the base class ModelView in the method doConnections(). PatternView inherits from TrackContentObjectView which inherits ModelView. |
|
Oh ok, so that's how it goes, this one was tricky! Thanks for the feedback! |
|
@badosu if you can rebase, I'll merge. Sorry, we're falling behind on the PRs. As far as testing, your results are sufficient IMO. We'll just tag you if it has any adverse effects. |
All the selected notes are changed by default for the 3 possible events: - Mouse dragging the volume/pan meter - Rolling the mouse wheel over the meter - Double-clicking the meter The user can still change each note individually by holding alt before performing the desired action
badosu
force-pushed
the
multiple-volume-changes
branch
from
e9f0874 to
6e3d4f4
Compare
|
@tresf done! |
Change behaviour for changing volume/pan on Piano
All the selected notes are changed by default for the 3 possible events:
The user can still change each note individually by holding alt before
performing the desired action
Fixes #322
@tresf @Sti2nd please test the alt keybindings on different OSes.