Add "Assign to FX Channel" context button to track #1572

badosu · 2015-01-08T03:18:28Z

Add a new submenu to assign quickly a new or existing FX Channel for a track. This should help a lot when creating songs as it's so cumbersome to assign them manually today.

The name of the channel is set as the name of the instrument when it's created.

This relates to #48 and maybe to #1215

badosu · 2015-01-08T03:21:11Z

include/FxMixerView.h

@@ -103,12 +103,14 @@ class EXPORT FxMixerView : public QWidget, public ModelView,
 	// useful for loading projects
 	void refreshDisplay();

+public slots:
+	int addNewChannel();


I had to expose this method to use it on the Track view, is there a better approach?

tresf · 2015-01-08T03:31:29Z

Very nice! I'll let Vesa speak to the question you have.

diizy · 2015-01-08T05:13:41Z

"If the instrument is already assigned to a FX Channel"

The problem is, instruments are always assigned to a FX channel. Master is also a FX channel, it's no different from any other, other than it can't be routed and outputs sound. I'm not sure if we should treat master as a sort of "anti-channel" here...

The context view is also a bit ambiguous when there's a channel assigned - not instantly obvious what the option does when it only shows the current channel name. Maybe the menu option should just always be "Assign to current FX channel"... @tresf, thoughts?

The slot usage is fine.

Sti2nd · 2015-01-08T07:32:32Z

The problem is, instruments are always assigned to a FX channel

Why is that a problem? If those images are screenshots it looks fine too me, it don't show master channel. I think everyone knows that instruments must be connected to master channel to hear sound :)

badosu · 2015-01-08T13:01:51Z

@diizy What I mean with that sentence is: If the assigned channel is different than 0 (master).

This PR is aimed at helping with somethng that's very difficult to do today, if the context option when there's already an independent channel assigned is ambiguous we can remove it (although I don't understand why it's not obvious).

We can always change the PR with suggestions until it's acceptable for you guys, just tell me what's wrong.

badosu · 2015-01-08T13:09:33Z

Maybe we can change the label "Assign to FX Channel" to "Assign to independent FX Channel" in case this improves the tool.

tresf · 2015-01-08T20:14:56Z

@tresf, thoughts?

I like it, a lot actually... but I understand the hesitation here... It adds convenience but also more mystique and uncertainty to what people are doing.

Even if it's technically correct, I'm not sure I agree with the statement that they already have a channel assigned. I agree that these are "routed" through master by default... but... that is more properly "auto-assigned". "Auto-assign" without delegation doesn't really mean "assign" in any work flow that I use on a daily basis.

But perhaps we're overthinking this feature. If we're going to have quick assignment, why not make it default, such as a checkbox "Auto-assign instruments dedicated FX tracks" or something like that.

Then we use the context menu to change FX assignment to available channels and add a new item "+Add New FX Channel" at the end.

But perhaps I'm over thinking it... :O)

badosu · 2015-01-08T20:24:25Z

But perhaps we're overthinking this feature. If we're going to have quick assignment, why not make it default, such as a checkbox "Auto-assign instruments dedicated FX tracks" or something like that.

This was already suggested on #1215 and was my inspiration to make this feature.

Actually I wanted to address that before but I thought that it was very intrusive and different from what we have today. So instead of developing that big feature I wanted to take a baby step towards a better functionality, that was the low hanging fruit in solving the usability problem of assigning independent FX Channel for an instrument.

badosu · 2015-01-08T20:34:16Z

@diizy Now I got what you mean, retain the same label for the button whatever is the case for the instrument so that it is clear what is that button about, that's a pretty good idea.

Is "Assign to independent FX Channel" more clear? Or instead of FX Channel using the word Mixer, just channel, etc..?

tresf · 2015-01-08T20:45:06Z

that was the low hanging fruit in solving the usability problem

Yes of course....

Is "Assign to independent FX Channel" more clear? Or instead of FX Channel using the word Mixer, just channel, etc..?

Or change it to "FX 0: Master" by default and use a sub-menu to allow the designation of any channel, with the option to add new at the end?

badosu · 2015-01-08T20:46:34Z

Or change it to "FX 0: Master" by default and use a sub-menu to allow the designation of any channel, with the option to add new at the end?

This would only be possible if the listed channels on the submenu are not already occupied.

tresf · 2015-01-08T20:46:51Z

This would only be possible if the listed channels on the submenu are not already occupied.

Why?

badosu · 2015-01-08T20:48:18Z

Well, I guess you're right, we can have more than one instrument on a channel.

But still, a submenu with 30 channels would be very bad for usability, specially if the "Assign to new channel" is at the end.

tresf · 2015-01-08T20:52:39Z

But still, a submenu with 30 channels would be very bad for usability, specially if the "Assign to new channel" is at the end.

Then we put it at the beginning?

I know this can get rather large, but I think overflowing context menus are something we're rather familiar with in the GUI world, no? :) And how nice would it be to combine two channels without leaving the Song Editor? !!!! :)

Old projects may have a lot of them due to the old default channel count, but new projects should have less than twenty channels I'd hope. There are exceptions to this, but those exceptional cases can use the mixer, no?

badosu · 2015-01-08T21:01:51Z

@tresf Cool, I am convinced this can work. Let's just wait for @diizy to give some input then I can start experimenting with this.

tresf · 2015-01-08T21:03:02Z

Sounds good... P.S. I did the pixel spacing on 30 FX tracks.... this is what it looks like from a size perspective (please don't mind the wrong font usage here, the spacing is pretty close)

tresf · 2015-01-08T21:04:33Z

Vesa may also advocate for multiple channel selection as well. If possible, I'd advocate for this only with a modifier key on the keyboard (force the use of CTRL or shift for multi-channel assignement... no modified for single-channel assignment).

badosu · 2015-01-08T21:12:12Z

I started to like this proposal, it also addresses @diizy concern of Master as an "anti-channel".

diizy · 2015-01-08T22:58:55Z

On 01/08/2015 03:09 PM, Amadeus Folego wrote:

Maybe we can change the label "Assign to FX Channel" to "Assign to
independent FX Channel" in case this improves the tool.

I'd say "Assign to current FX channel" is the most obvious since that's
what the option does, assign to currently selected channel.

Sti2nd · 2015-01-08T23:13:56Z

I'd say "Assign to current FX channel" is the most obvious since that's
what the option does, assign to currently selected channel.

Ohh, I didn't understand that, because of the wording actually. That was pretty hard to get. So do you mind "Assign to selected FX channel" instead?

diizy · 2015-01-08T23:19:01Z

On 01/08/2015 10:45 PM, Tres Finocchiaro wrote:

Or change it to "FX 0: Master" by default and use a sub-menu to allow
the designation of any channel, with the option to add new at the end?

Sounds like a neat idea.

diizy · 2015-01-08T23:19:34Z

On 01/09/2015 01:13 AM, Stian Jørgensrud wrote:

I'd say "Assign to current FX channel" is the most obvious since
that's
what the option does, assign to currently selected channel.
Ohh, I didn't understand that, because of the wording actually. That
was pretty hard to get. So do you mind "Assign to selected FX channel"
instead?

That works.

badosu · 2015-01-09T09:25:37Z

@tresf @Sti2nd @diizy

Just prototyped the submenu idea:

Some concerns:

I added a separator between "Assign to new FX Channel" and the subsequent actions but there's no visual indication of it's presence
We may need to improve the label of the assignment to other FX buttons, ideas?

Personally, I feel like this prototype already looks better than the original proposal.

tresf · 2015-01-09T15:10:32Z

Oh... I'm so excited about this feature!!!

In your screenshot, I see FX 1: TripleOscillator, FX 2: TripleOscillator. Are those the current names or are those the future names? I recommend using the current names (which will only read FX1 probably, but that would be contextually accurate, it can rename to FX1: TripleOscillator once you assign it).

badosu · 2015-01-09T16:47:23Z

@tresf This is just a coincidence, the names are all equal because I cloned the track.

I updated the description with a better illustration of the feature.

Oh... I'm so excited about this feature!!!

I know, me too! I am using it a lot now, It makes things so easier, I can't even imagine going back :-).

Thanks for the suggestion, it would not be like this if you had not proposed it.

tresf · 2015-01-09T18:03:42Z

src/gui/FxMixerView.cpp

@@ -185,6 +185,8 @@ void FxMixerView::addNewChannel()
 	updateFxLine(newChannelIndex);


Is there a reason we're using the word "line" instead of "channel" here?

Well, this method was not written by me so I don't really know.

I am using the "line" terminology on the parts I wrote to keep consistency with all the other parts of the code that refer to the same functionality.

I can include a commit to rename the Line terminology to Channel, but I need to understand if this is necessary/viable.

No, you're justification is completely valid in my opinion. I didn't realize the nomenclature was already there despite having the audacity to comment under a line you hadn't change. 🍔

tresf · 2015-01-10T00:02:19Z

@badosu I may have found the problem with the separator:

https://github.com/LMMS/lmms/blob/master/data/themes/default/style.css#L71

QMenu::separator shares the same color as the QMenu class. Here's what background: #8d8d8d; looks like... (not sure if all platforms display this correctly tho).

badosu · 2015-01-10T00:07:38Z

@tresf Great find! I'll keep that in mind.

Sti2nd · 2015-01-10T11:36:39Z

I still think it is not obvious that clicking "assign to new Fx channel" assigns it to the selected one. If you could add the word selected there it would be more obvious. "Assign to new (selected) channel"?

If you have a lot of channels to choose from, maybe qt got a scroll arrow features, like that arrow showing up at the bottom in a right click menu when there are really many options.

tresf · 2015-01-10T13:36:08Z

Why even care about selected? Shouldn't this instead be the first avail?

Sti2nd · 2015-01-10T13:43:08Z

Shouldn't this instead be the first avail?

You mean the first empty (not assigned to any instruments) channel? Good point, cause one tend to add many channels at once, not one and one, am I right? And even if the last added channel was automatically selected, it wouldn't work with that workflow.

tresf · 2015-01-10T14:20:46Z

Yes, exactly.

tresf · 2015-01-13T02:17:31Z

Sorry for the inconvenience but this will need to be rebased on master (fast forward and hand-edit any conflicts) before it is merged..

badosu · 2015-01-13T02:45:48Z

Rebased!

@tresf @Sti2nd Should I make further adjustments? Is there anything that need fixing?

@tresf About the separator issue, what should we do? Is it even an issue?

tresf · 2015-01-13T02:51:11Z

Hmm.... Some strange build errors there. The Zyn stuff on the Linux job should be unrelated, but the FxMixerView errors are probably valid.

But to answer your question... did you end up assign the "new" FX to the current channel, or the first unused channel?

tresf · 2015-01-13T02:52:16Z

@tresf About the separator issue, what should we do? Is it even an issue?

Not your problem really. I'll file a separate issue.

badosu · 2015-01-13T02:52:57Z

@tresf I just fixed these!

"Assign to new FX Channel" creates a new FX Channel and assigns the instrument to it. The new FX Channel will have the name of the instrument.

tresf · 2015-01-13T02:54:58Z

👍

tresf · 2015-01-13T03:00:07Z

FYI - Filed the menu bug 1615.

tresf · 2015-01-13T03:02:20Z

Can't wait to test this one... It has had plenty of time for comments, addresses all requests and passes in Travis. This one is getting merged. If issues arise we can patch.

Add "Assign to FX Channel" context button to track

badosu · 2015-01-13T03:08:42Z

Cool! :-)

badosu reviewed Jan 8, 2015
View reviewed changes

badosu force-pushed the assign-instrument-to-fx-channel branch from 850dc48 to fbff7ff Compare January 8, 2015 03:27

badosu force-pushed the assign-instrument-to-fx-channel branch 2 times, most recently from f526fca to 65a5a90 Compare January 8, 2015 04:12

badosu force-pushed the assign-instrument-to-fx-channel branch 2 times, most recently from d1a27e8 to 1eb1c4f Compare January 9, 2015 10:18

tresf reviewed Jan 9, 2015
View reviewed changes

badosu force-pushed the assign-instrument-to-fx-channel branch from 1eb1c4f to b2eb511 Compare January 13, 2015 02:42

Add "Assign to FX Channel" context button to track

2eb420c

badosu force-pushed the assign-instrument-to-fx-channel branch from b2eb511 to 2eb420c Compare January 13, 2015 02:49

tresf added a commit that referenced this pull request Jan 13, 2015

Merge pull request #1572 from badosu/assign-instrument-to-fx-channel

6e899d3

Add "Assign to FX Channel" context button to track

tresf merged commit 6e899d3 into LMMS:master Jan 13, 2015

badosu deleted the assign-instrument-to-fx-channel branch January 13, 2015 03:08

This was referenced Jan 13, 2015

Feature request "Assign to a new FX-mixer channel" button for instruments? #604

Closed

Button to add 5 FX channels #1009

Closed

tresf mentioned this pull request Jan 30, 2015

Automatic FX-mixer strip management #1215

Open

Sti2nd mentioned this pull request Apr 26, 2015

Drag instrument track label onto mixer slot to assign instrument's FX channel #2011

Closed

		@@ -185,6 +185,8 @@ void FxMixerView::addNewChannel()
		updateFxLine(newChannelIndex);

Add "Assign to FX Channel" context button to track #1572

Add "Assign to FX Channel" context button to track #1572

Conversation

badosu commented Jan 8, 2015

badosu Jan 8, 2015

Choose a reason for hiding this comment

tresf commented Jan 8, 2015

diizy commented Jan 8, 2015

Sti2nd commented Jan 8, 2015

badosu commented Jan 8, 2015

badosu commented Jan 8, 2015

tresf commented Jan 8, 2015

badosu commented Jan 8, 2015

badosu commented Jan 8, 2015

tresf commented Jan 8, 2015

badosu commented Jan 8, 2015

tresf commented Jan 8, 2015

badosu commented Jan 8, 2015

tresf commented Jan 8, 2015

badosu commented Jan 8, 2015

tresf commented Jan 8, 2015

tresf commented Jan 8, 2015

badosu commented Jan 8, 2015

diizy commented Jan 8, 2015

Sti2nd commented Jan 8, 2015

diizy commented Jan 8, 2015

diizy commented Jan 8, 2015

badosu commented Jan 9, 2015

tresf commented Jan 9, 2015

badosu commented Jan 9, 2015

tresf Jan 9, 2015

Choose a reason for hiding this comment

badosu Jan 9, 2015

Choose a reason for hiding this comment

tresf Jan 9, 2015

Choose a reason for hiding this comment

badosu Jan 9, 2015

Choose a reason for hiding this comment

tresf Jan 9, 2015

Choose a reason for hiding this comment

tresf commented Jan 10, 2015

badosu commented Jan 10, 2015

Sti2nd commented Jan 10, 2015

tresf commented Jan 10, 2015

Sti2nd commented Jan 10, 2015

tresf commented Jan 10, 2015

tresf commented Jan 13, 2015

badosu commented Jan 13, 2015

tresf commented Jan 13, 2015

tresf commented Jan 13, 2015

badosu commented Jan 13, 2015

tresf commented Jan 13, 2015

tresf commented Jan 13, 2015

tresf commented Jan 13, 2015

badosu commented Jan 13, 2015