From e91991216df9b2fc260c5c80bb563140459d3b0b Mon Sep 17 00:00:00 2001 From: Amadeus Folego Date: Fri, 23 Jan 2015 05:12:48 -0200 Subject: [PATCH] Fix segfault when moving channels This commit addresses a lot of issues, namely: 1. When the "Move left/right" action was selected on a mixer channel menu sometimes it would segfault due to the fxline object being deleted before it returned to a method inside itself 2. The Fader was declaring a new variable for the model whereas it should used the inherited model from FloatModelView < AutomatableModelView 3. Some methods were doing more things than they should be and performing unnecessary actions. A little cleanup/refactor was made Please notice that a bug of the same class as the one referred at point 1 still exists: clicking on "Remove channel". This commit does not addresses this issue. Fixes #1584 --- include/Fader.h | 6 +-- include/FxMixerView.h | 3 ++ src/core/FxMixer.cpp | 39 ++++++++----------- src/gui/FxMixerView.cpp | 80 +++++++++++++++++++-------------------- src/gui/widgets/Fader.cpp | 10 ++--- 5 files changed, 63 insertions(+), 75 deletions(-) diff --git a/include/Fader.h b/include/Fader.h index 3e808802a..d21b7fa31 100644 --- a/include/Fader.h +++ b/include/Fader.h @@ -100,14 +100,12 @@ private: int knobPosY() const { - float fRange = m_model->maxValue() - m_model->minValue(); - float realVal = m_model->value() - m_model->minValue(); + float fRange = model()->maxValue() - model()->minValue(); + float realVal = model()->value() - model()->minValue(); return height() - ( ( height() - m_knob->height() ) * ( realVal / fRange ) ); } - FloatModel * m_model; - void setPeak( float fPeak, float &targetPeak, float &persistentPeak, QTime &lastPeakTime ); int calculateDisplayPeak( float fPeak ); diff --git a/include/FxMixerView.h b/include/FxMixerView.h index 66a9143b4..08d24fd09 100644 --- a/include/FxMixerView.h +++ b/include/FxMixerView.h @@ -53,6 +53,8 @@ public: public: FxChannelView(QWidget * _parent, FxMixerView * _mv, int _chIndex ); + void setChannelIndex( int index ); + FxLine * m_fxLine; PixmapButton * m_muteBtn; PixmapButton * m_soloBtn; @@ -97,6 +99,7 @@ public: // move the channel to the left or right void moveChannelLeft(int index); + void moveChannelLeft(int index, int focusIndex); void moveChannelRight(int index); // make sure the display syncs up with the fx mixer. diff --git a/src/core/FxMixer.cpp b/src/core/FxMixer.cpp index 00143b2ee..943341a23 100644 --- a/src/core/FxMixer.cpp +++ b/src/core/FxMixer.cpp @@ -327,6 +327,19 @@ void FxMixer::deleteChannel( int index ) for( int i = index; i < m_fxChannels.size(); ++i ) { validateChannelName( i, i + 1 ); + + // set correct channel index + m_fxChannels[i]->m_channelIndex = i; + + // now check all routes and update names of the send models + foreach( FxRoute * r, m_fxChannels[i]->m_sends ) + { + r->updateName(); + } + foreach( FxRoute * r, m_fxChannels[i]->m_receives ) + { + r->updateName(); + } } } @@ -364,18 +377,9 @@ void FxMixer::moveChannelLeft( int index ) { inst->effectChannelModel()->setValue(a); } - } } } - - // actually do the swap - FxChannel * tmpChannel = m_fxChannels[a]; - m_fxChannels[a] = m_fxChannels[b]; - m_fxChannels[b] = tmpChannel; - - validateChannelName( a, b ); - validateChannelName( b, a ); } @@ -758,22 +762,9 @@ void FxMixer::loadSettings( const QDomElement & _this ) void FxMixer::validateChannelName( int index, int oldIndex ) { - FxChannel * fxc = m_fxChannels[ index ]; - if( fxc->m_name == tr( "FX %1" ).arg( oldIndex ) ) + if( m_fxChannels[index]->m_name == tr( "FX %1" ).arg( oldIndex ) ) { - fxc->m_name = tr( "FX %1" ).arg( index ); - } - // set correct channel index - fxc->m_channelIndex = index; - - // now check all routes and update names of the send models - foreach( FxRoute * r, fxc->m_sends ) - { - r->updateName(); - } - foreach( FxRoute * r, fxc->m_receives ) - { - r->updateName(); + m_fxChannels[index]->m_name = tr( "FX %1" ).arg( index ); } } diff --git a/src/gui/FxMixerView.cpp b/src/gui/FxMixerView.cpp index 141a37619..5e99ca32d 100644 --- a/src/gui/FxMixerView.cpp +++ b/src/gui/FxMixerView.cpp @@ -267,19 +267,20 @@ void FxMixerView::loadSettings( const QDomElement & _this ) FxMixerView::FxChannelView::FxChannelView(QWidget * _parent, FxMixerView * _mv, - int _chIndex ) + int channelIndex ) { - m_fxLine = new FxLine(_parent, _mv, _chIndex); + m_fxLine = new FxLine(_parent, _mv, channelIndex); - FxMixer * m = Engine::fxMixer(); - m_fader = new Fader( &m->effectChannel(_chIndex)->m_volumeModel, - tr( "FX Fader %1" ).arg( _chIndex ), m_fxLine ); + FxChannel *fxChannel = Engine::fxMixer()->effectChannel(channelIndex); + + m_fader = new Fader( &fxChannel->m_volumeModel, + tr( "FX Fader %1" ).arg( channelIndex ), m_fxLine ); m_fader->move( 16-m_fader->width()/2, m_fxLine->height()- m_fader->height()-5 ); m_muteBtn = new PixmapButton( m_fxLine, tr( "Mute" ) ); - m_muteBtn->setModel( &m->effectChannel(_chIndex)->m_muteModel ); + m_muteBtn->setModel( &fxChannel->m_muteModel ); m_muteBtn->setActiveGraphic( embed::getIconPixmap( "led_off" ) ); m_muteBtn->setInactiveGraphic( @@ -289,23 +290,34 @@ FxMixerView::FxChannelView::FxChannelView(QWidget * _parent, FxMixerView * _mv, ToolTip::add( m_muteBtn, tr( "Mute this FX channel" ) ); m_soloBtn = new PixmapButton( m_fxLine, tr( "Solo" ) ); - m_soloBtn->setModel( &m->effectChannel(_chIndex)->m_soloModel ); + m_soloBtn->setModel( &fxChannel->m_soloModel ); m_soloBtn->setActiveGraphic( embed::getIconPixmap( "led_red" ) ); m_soloBtn->setInactiveGraphic( embed::getIconPixmap( "led_off" ) ); m_soloBtn->setCheckable( true ); m_soloBtn->move( 9, m_fader->y()-21); - connect(&m->effectChannel(_chIndex)->m_soloModel, SIGNAL( dataChanged() ), + connect(&fxChannel->m_soloModel, SIGNAL( dataChanged() ), _mv, SLOT ( toggledSolo() ) ); ToolTip::add( m_soloBtn, tr( "Solo FX channel" ) ); // Create EffectRack for the channel - m_rackView = new EffectRackView( &m->effectChannel(_chIndex)->m_fxChain, _mv->m_racksWidget ); + m_rackView = new EffectRackView( &fxChannel->m_fxChain, _mv->m_racksWidget ); m_rackView->setFixedSize( 245, FxLine::FxLineHeight ); } +void FxMixerView::FxChannelView::setChannelIndex( int index ) +{ + FxChannel* fxChannel = Engine::fxMixer()->effectChannel( index ); + + m_fader->setModel( &fxChannel->m_volumeModel ); + m_muteBtn->setModel( &fxChannel->m_muteModel ); + m_soloBtn->setModel( &fxChannel->m_soloModel ); + m_rackView->setModel( &fxChannel->m_fxChain ); +} + + void FxMixerView::toggledSolo() { Engine::fxMixer()->toggledSolo(); @@ -432,53 +444,39 @@ void FxMixerView::deleteUnusedChannels() -void FxMixerView::moveChannelLeft(int index) +void FxMixerView::moveChannelLeft(int index, int focusIndex) { // can't move master or first channel left or last channel right if( index <= 1 || index >= m_fxChannelViews.size() ) return; - int selIndex = m_currentFxLine->channelIndex(); + FxMixer *m = Engine::fxMixer(); - FxMixer * mix = Engine::fxMixer(); - mix->moveChannelLeft(index); + // Move instruments channels + m->moveChannelLeft( index ); - // refresh the two mixer views - for( int i = index-1; i <= index; ++i ) - { - // delete the mixer view - int replaceIndex = chLayout->indexOf(m_fxChannelViews[i]->m_fxLine); + // Update widgets models + m_fxChannelViews[index]->setChannelIndex( index - 1 ); + m_fxChannelViews[index - 1]->setChannelIndex( index ); - chLayout->removeWidget(m_fxChannelViews[i]->m_fxLine); - m_racksLayout->removeWidget( m_fxChannelViews[i]->m_rackView ); - delete m_fxChannelViews[i]->m_fader; - delete m_fxChannelViews[i]->m_muteBtn; - delete m_fxChannelViews[i]->m_soloBtn; - delete m_fxChannelViews[i]->m_fxLine; - delete m_fxChannelViews[i]; + // Swap positions in array + qSwap(m->m_fxChannels[index], m->m_fxChannels[index - 1]); - // add it again - m_fxChannelViews[i] = new FxChannelView( m_channelAreaWidget, this, i ); - chLayout->insertWidget( replaceIndex, m_fxChannelViews[i]->m_fxLine ); - m_racksLayout->insertWidget( replaceIndex, m_fxChannelViews[i]->m_rackView ); - } + // Focus on new position + setCurrentFxLine( focusIndex ); +} - // keep selected channel - if( selIndex == index ) - { - selIndex = index-1; - } - else if( selIndex == index - 1 ) - { - selIndex = index; - } - setCurrentFxLine(selIndex); + + +void FxMixerView::moveChannelLeft(int index) +{ + moveChannelLeft( index, index - 1 ); } void FxMixerView::moveChannelRight(int index) { - moveChannelLeft(index+1); + moveChannelLeft( index + 1, index + 1 ); } diff --git a/src/gui/widgets/Fader.cpp b/src/gui/widgets/Fader.cpp index 0f0bef69d..d8055f5ac 100644 --- a/src/gui/widgets/Fader.cpp +++ b/src/gui/widgets/Fader.cpp @@ -67,7 +67,6 @@ QPixmap * Fader::s_knob = NULL; Fader::Fader( FloatModel * _model, const QString & _name, QWidget * _parent ) : QWidget( _parent ), FloatModelView( _model, this ), - m_model( _model ), m_fPeakValue_L( 0.0 ), m_fPeakValue_R( 0.0 ), m_persistentPeak_L( 0.0 ), @@ -114,7 +113,6 @@ Fader::Fader( FloatModel * _model, const QString & _name, QWidget * _parent ) : Fader::Fader( FloatModel * model, const QString & name, QWidget * parent, QPixmap * back, QPixmap * leds, QPixmap * knob ) : QWidget( parent ), FloatModelView( model, this ), - m_model( model ), m_fPeakValue_L( 0.0 ), m_fPeakValue_R( 0.0 ), m_persistentPeak_L( 0.0 ), @@ -170,7 +168,7 @@ void Fader::mouseMoveEvent( QMouseEvent *mouseEvent ) { int dy = m_moveStartPoint - mouseEvent->globalY(); - float delta = dy * ( m_model->maxValue() - m_model->minValue() ) / (float) ( height() - ( *m_knob ).height() ); + float delta = dy * ( model()->maxValue() - model()->minValue() ) / (float) ( height() - ( *m_knob ).height() ); model()->setValue( m_startValue + delta ); @@ -256,11 +254,11 @@ void Fader::wheelEvent ( QWheelEvent *ev ) if ( ev->delta() > 0 ) { - m_model->incValue( 1 ); + model()->incValue( 1 ); } else { - m_model->incValue( -1 ); + model()->incValue( -1 ); } updateTextFloat(); s_textFloat->setVisibilityTimeOut( 1000 ); @@ -326,7 +324,7 @@ void Fader::updateTextFloat() } else { - s_textFloat->setText( m_description + " " + QString("%1 ").arg( m_displayConversion ? m_model->value() * 100 : m_model->value() ) + " " + m_unit ); + s_textFloat->setText( m_description + " " + QString("%1 ").arg( m_displayConversion ? model()->value() * 100 : model()->value() ) + " " + m_unit ); } s_textFloat->moveGlobal( this, QPoint( width() - ( *m_knob ).width() - 5, knobPosY() - 46 ) ); }