Redesign Flanger #3007

RebeccaDeField · 2016-08-28T21:02:24Z

Related topics: #2831 #2978

In this pull request I have:

Updated the knob alignment
Changed Invert text to be aligned left next to the LED
Resized background so that the space around the elements is even

Before

After

@Umcaruje @BaraMGB @LocoMatt

Umcaruje · 2016-08-28T23:49:44Z

plugins/Flanger/FlangerControlsDialog.cpp

 	lfoFreqKnob->setVolumeKnob( false );
 	lfoFreqKnob->setModel( &controls->m_lfoFrequencyModel );
 	lfoFreqKnob->setLabel( tr( "Lfo Hz" ) );
 	lfoFreqKnob->setHintText( tr ( "Lfo:" ) , "Hz" );

 	Knob * lfoAmtKnob = new Knob( knobBright_26, this );
-	lfoAmtKnob->move( 86,10 );
+	lfoAmtKnob->move( 84,10 );
 	lfoAmtKnob->setVolumeKnob( false );
 	lfoAmtKnob->setModel( &controls->m_lfoAmountModel );
 	lfoAmtKnob->setLabel( tr( "Amt" ) );


I'd rename Amt to Amnt, as that's what its called in the Peak Controller and LFO controller. Furthermore, I believe we should convert the knob labels to all caps, to be consistent with the other plugins:

The LED label should stay as is, to be consistent with the peak controller.

I think Amt is a common abbreviation for Amount and Amnt isn't. Amnt is even not recognized by this editor. So, maybe change the other ones?

@BaraMGB I believe you worked on the Peak Controller and LFO controller. Do you have an opinion about this?

AMNT is 4 letters; more consistent with the majority of the knobs to the likes of already abbreviated DCAY, FREQ, GAIN and as a bonus it is less code changes. At the end of the day, we'll always be faced with translation problems with any abbreviated words which causes problems like #2599, but for now, let's keep it AMNT.

I agree with @tresf

So as long as we have a button labeled TRES for threshold, I think AMT is the least of our worries but I agree, if we use DEC in other places, we should standardize that as well, perhaps through a centralized translation file.

We should change tr( "Amt" ) with ABBR( Amount ) and keep abbreviations in a header:

#define ABBR_Amount "AMT" #define ABBR( str ) tr( ABBR_ ## str )

Thank you @zonkmachine @tresf @jasp00 for the input. From reading over the comments here I'm not sure whether we have come to a final decision about keeping it as AMT or changing it to AMNT?

@RebeccaDeField please keep it AMNT for now to move this along and remain consistent with the rest of the software. The task of standardizing our abbreviations (and additionally changing AMNT to AMT) is out of scope and will be handled as a separate initiative.

Sounds good. I will make that change along with the other suggestions from @Umcaruje as soon as I can 👍

RebeccaDeField · 2016-09-03T18:59:43Z

I noticed that my fork has some errors and while fixing them, I accidentally wiped the commit for this PR, which closed it automatically. I have re-committed the files with some of the changes and reopened this PR 😅

RebeccaDeField · 2016-09-03T19:02:19Z

The LED label should stay as is, to be consistent with the peak controller.

When you say "stay as is" could you clarify about which part you want to stay the same? Do you mean that the label, the position, etc?

@Umcaruje I know you're away right now so no pressure on responding to this.

Umcaruje · 2016-09-03T19:34:51Z

I meant that the LED labels should stay the same case, e.g. Sentence case,
as I suggested the other labels to go all caps to be consistent. The
alignment looks good to me.

Regards from Greece

On 3 Sep 2016 22:02, "Rebecca DeField" notifications@github.com wrote:

The LED label should stay as is, to be consistent with the peak controller.

When you say "stay as is" could you clarify about which part you want to
stay the same? Do you mean that the label, the position, etc?

@Umcaruje https://github.com/Umcaruje I know you're away right now so
no pressure on responding to this.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#3007 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AF_bPRIT3-eMKxeTfGJr_JwFh17BAAteks5qmcQ9gaJpZM4JvBGx
.

RebeccaDeField · 2016-09-03T21:02:34Z

@Umcaruje Well I'm glad to hear that is what you meant, because that is what I did 😄

I believe I have now completed all of the requested changes to the Flanger:

@tresf @Umcaruje @BaraMGB

Umcaruje · 2016-09-05T14:14:02Z

"LFO HZ" sounds weird to me. Its the frequency of the LFO, so maybe "FREQ"
could be more suitable.

Also what is the Regeneration? Is it another name for feedback? Anyways,
I'll test this out when I have pc access.

On 4 Sep 2016 00:02, "Rebecca DeField" notifications@github.com wrote:

@Umcaruje https://github.com/Umcaruje Well I'm glad to hear that is
what you meant, because that is what I did 😄

I believe I have now completed all of the requested changes to the Flanger:
[image: unnamed]
https://cloud.githubusercontent.com/assets/7960169/18227609/f5095aa0-71de-11e6-861f-c8a023537f04.png

@tresf https://github.com/tresf @Umcaruje https://github.com/Umcaruje
@BaraMGB https://github.com/BaraMGB

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#3007 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AF_bPUQU2QWQ0lbQ32o_R07RSnFvOcTLks5qmeBsgaJpZM4JvBGx
.

zonkmachine · 2016-09-05T15:10:55Z

"LFO HZ" sounds weird to me. Its the frequency of the LFO, so maybe "FREQ" could be more suitable.

I thought about this too. The two most common labels I've seen is: SPEED and RATE. RATE is what Moog and Electro-Harmonix uses. If you want to be that hip...

Umcaruje · 2016-09-05T18:31:35Z

RATE sounds good to me.

RebeccaDeField · 2016-09-06T22:36:43Z

RATE sounds good to me.

I'm on it. Will @mention you when it's ready for review.

RebeccaDeField · 2016-09-09T23:46:52Z

@Umcaruje This is ready for review when you get the chance.

jasp00 · 2016-09-10T00:10:42Z

plugins/Flanger/FlangerControlsDialog.cpp

-	lfoFreqKnob->setLabel( tr( "Lfo Hz" ) );
-	lfoFreqKnob->setHintText( tr ( "Lfo:" ) , "Hz" );
+	lfoFreqKnob->setLabel( tr( "RATE" ) );
+	lfoFreqKnob->setHintText( tr ( "Rate:" ) , "" );


I would not remove the unit: tr ( "Rate:" ) , "Hz".

@jasp00 Changed per your request 👍

tresf · 2016-09-12T19:09:37Z

@RebeccaDeField zero commits auto-closes a PR. Please feel free to reopen a new one once your commit count is above 0. No worries BTW, this is a github feature, so we just have to deal with it.

RebeccaDeField · 2016-09-12T19:11:44Z

@tresf Yes, sorry about that. Unfortunately I had to reset again because of a bad tutorial messing up my fork. Only saw people commenting that tutorial was wrong after following it halfway. I guess that is what I get from learning git from trial, error and online research :P

RebeccaDeField · 2016-09-12T19:45:07Z

@tresf Issues resolved. PR reopened. :)

@Umcaruje Regen has been updated to FDBK as requested.

Mentioning some people that might have an opinion about the label changes before it is merged @jasp00 @BaraMGB @zonkmachine.

Umcaruje · 2016-09-12T20:21:51Z

plugins/Flanger/FlangerControlsDialog.cpp

 	lfoAmtKnob->setVolumeKnob( false );
 	lfoAmtKnob->setModel( &controls->m_lfoAmountModel );
-	lfoAmtKnob->setLabel( tr( "Amt" ) );
+	lfoAmtKnob->setLabel( tr( "AMNT" ) );
 	lfoAmtKnob->setHintText( tr ( "Amt:" ) , "" );


This should be Amount: rather than Amt:

RebeccaDeField · 2016-09-12T20:25:15Z

@Umcaruje Just want to double check that what you're referring to is the hint text?

Umcaruje · 2016-09-12T20:28:31Z

@RebeccaDeField I was talking about the hint text, yeah, the knob label is good.

BaraMGB · 2016-09-12T22:10:27Z

I don't know. Do we really need such cryptical abbreviations?

RebeccaDeField · 2016-09-12T22:12:34Z

@Umcaruje Done.

@BaraMGB Thank you for the feedback. Would you prefer that it is switched back to Regen, that we abbreviate it differently or maybe have a different word to propose altogether?

BaraMGB · 2016-09-12T22:18:27Z

No, I think "feedback" is the right name for this knob in a flanger effect. But FDBK looks strange for me. But I fear I got no better idea at the moment. I'm just not a friend of abbreviations.

tresf · 2016-09-12T22:36:21Z

FBACK?

RebeccaDeField · 2016-09-12T23:13:22Z

I guess FB is another one, but that also means Facebook.

BaraMGB · 2016-09-13T06:11:43Z

FEEDB, perhaps. I don't know... FBACK could be okay, too.

RebeccaDeField · 2016-09-13T20:18:04Z

Ideas so far...

FEEDB
FBACK
FB
FDBK

Reposting from Discord chat:
It is also something to consider that FDBK is the only official abbreviation of feedback that I could find online. Maybe we can go forward with FDBK and then change it later if need be?

RebeccaDeField · 2016-09-13T20:37:14Z

Reposting from Discord chat:
@zonkmachine FDBK works and I don't mind REGEN either.
@BaraMGB For me it isn't so important. @rebecca FDBK is good enough.
@Umcaruje I like FDBK more haha

With that, I'm going to keep FDBK for now. If we want to make another PR to change it later, we can do that. If the flanger does not receive any additional changes in a couple days, I will merge. 👍

RebeccaDeField · 2016-09-15T17:44:56Z

It's been two days without any further feedback or changes. Merged.

Umcaruje · 2016-09-16T09:52:55Z

👍

Redesign Flanger

RebeccaDeField mentioned this pull request Aug 28, 2016

Redesign The Effects Plugins #2831

Closed

14 tasks

Umcaruje reviewed Aug 28, 2016
View reviewed changes

RebeccaDeField closed this Sep 3, 2016

RebeccaDeField reopened this Sep 3, 2016

jasp00 reviewed Sep 10, 2016
View reviewed changes

RebeccaDeField closed this Sep 12, 2016

Update Flanger

92e09e8

RebeccaDeField reopened this Sep 12, 2016

RebeccaDeField added 2 commits September 12, 2016 12:40

Background

05df928

Background tweak

4a48a6b

Umcaruje reviewed Sep 12, 2016
View reviewed changes

Hint text update

defb6aa

RebeccaDeField merged commit ec98b9b into LMMS:master Sep 15, 2016

sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022

Merge pull request LMMS#3007 from RebeccaDeField/master

44444a8

Redesign Flanger

Redesign Flanger #3007

Redesign Flanger #3007

Conversation

RebeccaDeField commented Aug 28, 2016 • edited Loading

Umcaruje Aug 28, 2016 • edited Loading

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

tresf Sep 2, 2016 • edited Loading

Choose a reason for hiding this comment

RebeccaDeField Sep 2, 2016 • edited Loading

Choose a reason for hiding this comment

RebeccaDeField commented Sep 3, 2016

RebeccaDeField commented Sep 3, 2016

Umcaruje commented Sep 3, 2016

RebeccaDeField commented Sep 3, 2016 • edited Loading

Umcaruje commented Sep 5, 2016

zonkmachine commented Sep 5, 2016

Umcaruje commented Sep 5, 2016 via email

RebeccaDeField commented Sep 6, 2016 • edited Loading

RebeccaDeField commented Sep 9, 2016 • edited Loading

Choose a reason for hiding this comment

RebeccaDeField Sep 10, 2016 • edited Loading

Choose a reason for hiding this comment

tresf commented Sep 12, 2016

RebeccaDeField commented Sep 12, 2016 • edited Loading

RebeccaDeField commented Sep 12, 2016 • edited Loading

Choose a reason for hiding this comment

RebeccaDeField commented Sep 12, 2016 • edited Loading

Umcaruje commented Sep 12, 2016

BaraMGB commented Sep 12, 2016

RebeccaDeField commented Sep 12, 2016 • edited Loading

BaraMGB commented Sep 12, 2016

tresf commented Sep 12, 2016

RebeccaDeField commented Sep 12, 2016 • edited Loading

BaraMGB commented Sep 13, 2016 • edited Loading

RebeccaDeField commented Sep 13, 2016 • edited Loading

RebeccaDeField commented Sep 13, 2016

RebeccaDeField commented Sep 15, 2016

Umcaruje commented Sep 16, 2016

RebeccaDeField commented Aug 28, 2016 •

edited

Loading

Umcaruje Aug 28, 2016 •

edited

Loading

tresf Sep 2, 2016 •

edited

Loading

RebeccaDeField Sep 2, 2016 •

edited

Loading

RebeccaDeField commented Sep 3, 2016 •

edited

Loading

RebeccaDeField commented Sep 6, 2016 •

edited

Loading

RebeccaDeField commented Sep 9, 2016 •

edited

Loading

RebeccaDeField Sep 10, 2016 •

edited

Loading

RebeccaDeField commented Sep 12, 2016 •

edited

Loading

RebeccaDeField commented Sep 12, 2016 •

edited

Loading

RebeccaDeField commented Sep 12, 2016 •

edited

Loading

RebeccaDeField commented Sep 12, 2016 •

edited

Loading

RebeccaDeField commented Sep 12, 2016 •

edited

Loading

BaraMGB commented Sep 13, 2016 •

edited

Loading

RebeccaDeField commented Sep 13, 2016 •

edited

Loading