Fixed MIDI Autodetect always On (#1813) #7564

Rhodexa · 2024-10-25T21:51:43Z

The Autodetect checkbox for MIDI Controller Connections is now only enabled if there’s no existing connection between a controller and a parameter. Additionally, the Autodetect feature will disengage automatically when a MIDI CC signal is recognized. This prevents accidental reassignment of connections and ensures clear visibility of active controls

szeli1

I reviewed your PR, please make the following changes

(DID NOT TEST)

szeli1 · 2024-10-26T06:42:10Z

src/gui/modals/ControllerConnectionDialog.cpp

@@ -125,8 +125,7 @@ public slots:
 namespace gui
 {

-ControllerConnectionDialog::ControllerConnectionDialog( QWidget * _parent, 
-		const AutomatableModel * _target_model ) :
+ControllerConnectionDialog::ControllerConnectionDialog( QWidget * _parent, const AutomatableModel * _target_model ) :


Suggested change

ControllerConnectionDialog::ControllerConnectionDialog( QWidget * _parent, const AutomatableModel * _target_model ) :

ControllerConnectionDialog::ControllerConnectionDialog(QWidget* _parent, const AutomatableModel* _target_model) :

Make sure you follow the style guidelines!

szeli1 · 2024-10-26T06:42:44Z

src/gui/modals/ControllerConnectionDialog.cpp

 	m_midiGroupBox = new GroupBox( tr( "MIDI CONTROLLER" ), this );
 	m_midiGroupBox->setGeometry( 8, 10, 240, 80 );
 	connect( m_midiGroupBox->model(), SIGNAL(dataChanged()),
 			this, SLOT(midiToggled()));

-	m_midiChannelSpinBox = new LcdSpinBox( 2, m_midiGroupBox,
-			tr( "Input channel" ) );
+	m_midiChannelSpinBox = new LcdSpinBox( 2, m_midiGroupBox, tr( "Input channel" ) );


Suggested change

m_midiChannelSpinBox = new LcdSpinBox( 2, m_midiGroupBox, tr( "Input channel" ) );

m_midiChannelSpinBox = new LcdSpinBox(2, m_midiGroupBox, tr("Input channel"));

szeli1 · 2024-10-26T06:43:21Z

src/gui/modals/ControllerConnectionDialog.cpp

 	m_midiChannelSpinBox->addTextForValue( 0, "--" );
 	m_midiChannelSpinBox->setLabel( tr( "CHANNEL" ) );
 	m_midiChannelSpinBox->move( 8, 24 );

-	m_midiControllerSpinBox = new LcdSpinBox( 3, m_midiGroupBox,
-			tr( "Input controller" ) );
+	m_midiControllerSpinBox = new LcdSpinBox( 3, m_midiGroupBox, tr( "Input controller" ) );


Suggested change

m_midiControllerSpinBox = new LcdSpinBox( 3, m_midiGroupBox, tr( "Input controller" ) );

m_midiControllerSpinBox = new LcdSpinBox(3, m_midiGroupBox, tr("Input controller"));

szeli1 · 2024-10-26T06:43:57Z

src/gui/modals/ControllerConnectionDialog.cpp

-	m_midiAutoDetectCheckBox =
-			new LedCheckBox( tr("Auto Detect"),
-				m_midiGroupBox, tr("Auto Detect") );
+	m_midiAutoDetectCheckBox = new LedCheckBox( tr("Auto Detect"), m_midiGroupBox, tr("Auto Detect") );


Suggested change

m_midiAutoDetectCheckBox = new LedCheckBox( tr("Auto Detect"), m_midiGroupBox, tr("Auto Detect") );

m_midiAutoDetectCheckBox = new LedCheckBox(tr("Auto Detect"), m_midiGroupBox, tr("Auto Detect"));

szeli1 · 2024-10-26T06:45:27Z

src/gui/modals/ControllerConnectionDialog.cpp

-		rp_btn->setGeometry( 160, 24, 32, 32 );
-		rp_btn->setMenu( m_readablePorts );
-		rp_btn->setPopupMode( QToolButton::InstantPopup );
+			rp_btn->setText( tr( "MIDI-devices to receive MIDI-events from" ) );


Please remove unnecessary white spaces here.

szeli1 · 2024-10-26T06:46:22Z

src/gui/modals/ControllerConnectionDialog.cpp

@@ -200,7 +199,7 @@ ControllerConnectionDialog::ControllerConnectionDialog( QWidget * _parent,
 			this, SLOT(userSelected()));


-	// Mapping functions
+	// MAPPING FUNCTION Section


Suggested change

// MAPPING FUNCTION Section

// Mapping function section

szeli1 · 2024-10-26T06:51:27Z

src/gui/modals/ControllerConnectionDialog.cpp

+	// TODO: This should be reworked into some sort of "shadow" state rather than turning it off...
+	// since it makes for switching between the USER mode and CONTROLLER mode a bit awkward
+	// If you toggle the autodetect then move from MIDI to USER then back to MIDI it resets, which is not ideal.
+	m_midiAutoDetect.setValue( enabled && (!m_targetModel->controllerConnection()) );


Suggested change

m_midiAutoDetect.setValue( enabled && (!m_targetModel->controllerConnection()) );

m_midiAutoDetect.setValue(enabled && (!m_targetModel->controllerConnection()));

Usual white space issue

szeli1 · 2024-10-26T06:52:02Z

src/gui/modals/ControllerConnectionDialog.cpp

@@ -420,6 +424,7 @@ void ControllerConnectionDialog::midiValueChanged()
 	if( m_midiAutoDetect.value() )
 	{
 		m_midiController->useDetected();
+		m_midiAutoDetect.setValue( false );


Suggested change

m_midiAutoDetect.setValue( false );

m_midiAutoDetect.setValue(false);

szeli1 · 2024-10-26T06:52:29Z

src/gui/modals/ControllerConnectionDialog.cpp

@@ -382,7 +385,7 @@ void ControllerConnectionDialog::midiToggled()



-
+// USER CONTROLLER Section logic (Doubles as a Radio-button kinda logic together with MIDI CONTROLLER Section, apparently)


Put this inside the header please

szeli1 · 2024-10-26T06:52:37Z

src/gui/modals/ControllerConnectionDialog.cpp

-
-
+// Comments from someone trying to get their head around Qt!
+// MIDI CONTROLLER Section logic (Doubles as a Radio-button kinda logic together with USER CONTROLLER Section, apaprently)


Please add these comments inside the header like this:

//! MIDI controller section logic (Doubles as a Radio-button kinda logic together with user controller section, apparently?)

Rhodexa · 2024-10-28T07:00:28Z

Hey, thanks for the feedback, @szeli1, I have revised the entire file to correct code style issues in general. Additionally, I have a also fixed similar issues that were present in the header file. I hope it now adheres better to the ideal structure!

Please let me know if there’s anything else you’d like me to adjust!

szeli1 · 2024-10-28T07:15:46Z

Usually the only lines that are formatted are the lines that were changed. I'm sorry I didn't mention this before and I'm not sure what should you do with this knowledge. If I were you I would undo these formatting changes for all lines that were not changed.

(edit: you can mark my review as closed once you implemented the changes I requested)

szeli1

This was a style review.

It is difficult to see what was changed because lots of lines are only formatted so it is hard to spot the differences.

szeli1 · 2024-10-28T07:20:20Z

src/gui/modals/ControllerConnectionDialog.cpp

 	{
 		cc = m_targetModel->controllerConnection();
-
-		if( cc && cc->getController()->type() != Controller::ControllerType::Dummy && Engine::getSong() )
+		if(cc && cc->getController()->type() != Controller::ControllerType::Dummy && Engine::getSong())


Suggested change

if(cc && cc->getController()->type() != Controller::ControllerType::Dummy && Engine::getSong())

if (cc && cc->getController()->type() != Controller::ControllerType::Dummy && Engine::getSong())

"if" or "for" and "(" should have a space between.

szeli1 · 2024-10-28T07:21:28Z

src/gui/modals/ControllerConnectionDialog.cpp

-	{
-		m_midiGroupBox->model()->setValue( true );
-	}
+	if(!cc) { m_midiGroupBox->model()->setValue(true); }


Suggested change

if(!cc) { m_midiGroupBox->model()->setValue(true); }

if (!cc) { m_midiGroupBox->model()->setValue(true); }

szeli1 · 2024-10-28T07:22:40Z

src/gui/modals/ControllerConnectionDialog.cpp

 	{
 		m_midiController->useDetected();
-		if( m_readablePorts )
+		m_midiAutoDetect.setValue(false);
+		if(m_readablePorts)


Suggested change

if(m_readablePorts)

if (m_readablePorts)

szeli1 · 2024-10-28T07:22:49Z

src/gui/modals/ControllerConnectionDialog.cpp

 void ControllerConnectionDialog::midiValueChanged()
 {
-	if( m_midiAutoDetect.value() )
+	if(m_midiAutoDetect.value())


Suggested change

if(m_midiAutoDetect.value())

if (m_midiAutoDetect.value())

Fix MIDI Autodetect (LMMS#1813)

b0b5ff8

szeli1 suggested changes Oct 26, 2024

View reviewed changes

Improved code style to align with community guidelines.

8a62cad

Fixed 'm_readablePorts' initialization order

93961a8

szeli1 suggested changes Oct 28, 2024

View reviewed changes

JohannesLorenz added the rework required Pull requests which must be reworked to make it able to merge or review label Nov 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed MIDI Autodetect always On (#1813) #7564

Fixed MIDI Autodetect always On (#1813) #7564

Rhodexa commented Oct 25, 2024

szeli1 left a comment

szeli1 Oct 26, 2024

szeli1 Oct 26, 2024

szeli1 Oct 26, 2024

szeli1 Oct 26, 2024

szeli1 Oct 26, 2024

szeli1 Oct 26, 2024

szeli1 Oct 26, 2024

szeli1 Oct 26, 2024

szeli1 Oct 26, 2024

szeli1 Oct 26, 2024

Rhodexa commented Oct 28, 2024

szeli1 commented Oct 28, 2024 •

edited

Loading

szeli1 left a comment

szeli1 Oct 28, 2024

szeli1 Oct 28, 2024

szeli1 Oct 28, 2024

szeli1 Oct 28, 2024

	ControllerConnectionDialog::ControllerConnectionDialog( QWidget * _parent, const AutomatableModel * _target_model ) :
	ControllerConnectionDialog::ControllerConnectionDialog(QWidget* _parent, const AutomatableModel* _target_model) :

	m_midiChannelSpinBox = new LcdSpinBox( 2, m_midiGroupBox, tr( "Input channel" ) );
	m_midiChannelSpinBox = new LcdSpinBox(2, m_midiGroupBox, tr("Input channel"));

	m_midiControllerSpinBox = new LcdSpinBox( 3, m_midiGroupBox, tr( "Input controller" ) );
	m_midiControllerSpinBox = new LcdSpinBox(3, m_midiGroupBox, tr("Input controller"));

	m_midiAutoDetectCheckBox = new LedCheckBox( tr("Auto Detect"), m_midiGroupBox, tr("Auto Detect") );
	m_midiAutoDetectCheckBox = new LedCheckBox(tr("Auto Detect"), m_midiGroupBox, tr("Auto Detect"));

	m_midiAutoDetect.setValue( enabled && (!m_targetModel->controllerConnection()) );
	m_midiAutoDetect.setValue(enabled && (!m_targetModel->controllerConnection()));

	m_midiAutoDetect.setValue( false );
	m_midiAutoDetect.setValue(false);

		@@ -382,7 +385,7 @@ void ControllerConnectionDialog::midiToggled()




		// USER CONTROLLER Section logic (Doubles as a Radio-button kinda logic together with MIDI CONTROLLER Section, apparently)



		// Comments from someone trying to get their head around Qt!
		// MIDI CONTROLLER Section logic (Doubles as a Radio-button kinda logic together with USER CONTROLLER Section, apaprently)

	if(cc && cc->getController()->type() != Controller::ControllerType::Dummy && Engine::getSong())
	if (cc && cc->getController()->type() != Controller::ControllerType::Dummy && Engine::getSong())

	if(!cc) { m_midiGroupBox->model()->setValue(true); }
	if (!cc) { m_midiGroupBox->model()->setValue(true); }

Fixed MIDI Autodetect always On (#1813) #7564

Are you sure you want to change the base?

Fixed MIDI Autodetect always On (#1813) #7564

Conversation

Rhodexa commented Oct 25, 2024

szeli1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rhodexa commented Oct 28, 2024

szeli1 commented Oct 28, 2024 • edited Loading

szeli1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szeli1 commented Oct 28, 2024 •

edited

Loading