Fix: unnecessary space in Update EqControlsDialog.cpp

[Gootector]

Fix: unnecessary space in Update EqControlsDialog.cpp

Greetings,
Gootector

[Rossmaxx]

I appreciate your concern and your willingness to contribute but this is not the way to contribute. Typo fixes are usually discouraged to be seperate PRs. I didn't comment on this the previous time because i didn't want to put out a bad vibe here. Please refrain from such PRs in the future. If you have something valuable to contribute, feel free to do so.

I can understand this message might come out as harsh but had to say it. I myself have done some typo fix contributions in the past, only to get even stronger no's. I'll merge this one for you with a style fix on my end.

Or if you still want to fix typos, consolidate everything into a single PR and we'll consider it in a better light.

[Gootector]

Do you merge or not?

[Rossmaxx]

Do you merge or not?

I will, for now. Just don't repeat it

[Gootector]

Don't worry - the last PR for LMMS. Bye!

[Gootector]

@Rossmaxx You're weird. You only fixed that one line. Other lines in this file haven't received "optimization" 🤣

[Rossmaxx]

The policy here is "fix only those lines you touch".

[bratpeki]

One of the reasons @Rossmaxx touches on everything he mentioned is because of just how much "heavier" you made the LMMS codebase with no practical addition. This is the diff of your PR:

From eb96666ed0ef4229b9ad93468ea624ffb5ac3b82 Mon Sep 17 00:00:00 2001
From: Grzegorz Pruchniakowski <[email protected]>
Date: Tue, 3 Sep 2024 14:16:18 +0200
Subject: [PATCH 1/2] Fix: unnecessary space in Update EqControlsDialog.cpp

Fix: unnecessary space in Update EqControlsDialog.cpp

Greetings,
Gootector
---
 plugins/Eq/EqControlsDialog.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/plugins/Eq/EqControlsDialog.cpp b/plugins/Eq/EqControlsDialog.cpp
index 8394569f668..e7ada404688 100644
--- a/plugins/Eq/EqControlsDialog.cpp
+++ b/plugins/Eq/EqControlsDialog.cpp
@@ -110,7 +110,7 @@ EqControlsDialog::EqControlsDialog( EqControls *controls ) :
 		resKnob->setVolumeKnob(false);
 		resKnob->setModel( m_parameterWidget->getBandModels( i )->res );
 		if(i > 1 && i < 6) { resKnob->setHintText( tr( "Bandwidth: " ) , tr( " Octave" ) ); }
-		else { resKnob->setHintText( tr( "Resonance : " ) , "" ); }
+		else { resKnob->setHintText( tr( "Resonance: " ) , "" ); }
 
 		auto freqKnob = new Knob(KnobType::Bright26, this);
 		freqKnob->move( distance, 396 );

From a162524d7eb14e597f48370bf9ffbd2cd1683d8f Mon Sep 17 00:00:00 2001
From: Rossmaxx <[email protected]>
Date: Tue, 3 Sep 2024 17:55:31 +0530
Subject: [PATCH 2/2] Style fix from Ross

---
 plugins/Eq/EqControlsDialog.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/plugins/Eq/EqControlsDialog.cpp b/plugins/Eq/EqControlsDialog.cpp
index e7ada404688..1fb10e2bb68 100644
--- a/plugins/Eq/EqControlsDialog.cpp
+++ b/plugins/Eq/EqControlsDialog.cpp
@@ -110,7 +110,7 @@ EqControlsDialog::EqControlsDialog( EqControls *controls ) :
 		resKnob->setVolumeKnob(false);
 		resKnob->setModel( m_parameterWidget->getBandModels( i )->res );
 		if(i > 1 && i < 6) { resKnob->setHintText( tr( "Bandwidth: " ) , tr( " Octave" ) ); }
-		else { resKnob->setHintText( tr( "Resonance: " ) , "" ); }
+		else { resKnob->setHintText(tr("Resonance: "), ""); }
 
 		auto freqKnob = new Knob(KnobType::Bright26, this);
 		freqKnob->move( distance, 396 );

You might notice that ALL the surrounding data takes up more space in the diff than your change. So you altered two lines but made the pull request longer by several deltas.

Your willingness to contribute is greatly appreciated. But there's loads of files that have these whitespace issues. Fix as many of them as you can sniff out. That's a proper PR.

[DomClark]

The purpose of this PR was to remove the space between "resonance" and the following colon, which is a valid change to the UI. The formatting of the modified line was also updated to match our current conventions, as we do in most pull requests.

Also, it won't have made the repository noticeably heavier. Git doesn't store diffs, so the context size in the diff is meaningless for measuring its weight. What has been added (if I correctly recall how Git works) is a new commit object, new objects for the /, /plugins, and /plugins/Eq trees, and a new copy of plugins/Eq/EqControlsDialog.cpp. These are compressed alongside the old versions, so will take up very little additional space.

We don't need PRs that are focused solely on code formatting. These genuinely contribute nothing to how the program functions, while polluting blame output and taking up reviewer time. We've already decided to go ahead with fixing individual lines when we modify them, which is working well enough for now.

[Gootector]

"We don't need PRs" because our PRIVATE closed-source project is dead. Bye, guys!

[DomClark]

"We don't need PRs" because our PRIVATE closed-source project is dead. Bye, guys!

This comes across as deliberately misrepresenting what I wrote. I said "we don't need PRs that are focused solely on code formatting", which was in response to bratpeki's comment: "There's loads of files that have these whitespace issues. Fix as many of them as you can sniff out. That's a proper PR." I wanted to discourage specifically such formatting-only PRs, and most certainly not PRs in general. Not only is this what I explicitly wrote, but it should also be fairly clear from the reasons I gave to back up my statement.

The earlier part of my comment was a defence of your PR: I explained that it was a useful change, as it updated UI text and not only code formatting (I think several people overlooked this), and also that it wouldn't measurably "make the codebase heavier" or anything to that effect.

Finally, whether you agree with my stance on formatting-only PRs or not, all projects are going to have some limits on what PRs they are willing to accept. Merging PRs is not the end goal of the LMMS project: it is a means to the actual goal, which is to develop an open-source digital audio workstation. While we welcome PRs from contributors all around the world, without which this project would be far behind where it is today, I don't think it is unreasonable to reject PRs that don't advance the project, and I'm sure the same goes for the vast majority of open-source projects out there.

If you want to be sure your efforts won't be wasted, reach out to a project before undertaking a large piece of work; this will help avoid any anger and upset from unsolicited changes being rejected. I hope my comments help clarify what sort of changes we are after in LMMS, and, to be clear, we do appreciate typo fixes, though batching them into a single PR is preferable to individual PRs for each.