mini fx rewrite without merge conflicts #7438

enp2s0 · 2024-08-07T16:39:05Z

Recent commits introduced some merge conflicts with #7242; this PR replaces that one. This PR also does not attempt to make the EffectView resizable or modify the wet/dry behavior and instead just focuses on UI improvements:

Hide the auto quit decay knob if autoquit is disabled in settings.
Remove the gate knob.
Shrink the height of effects from 60px to 35px.
Use the effect label itself as the button to open the controls rather than a separate button.

The new design in action:

And in the classic theme (as an aside, the new mixer strip design doesn't seem to be compatible at all with the classic theme, but the FX rack is good enough):

~~This is mostly complete. Currently hunting down a small bug where the controls button stays in the "open" state even when you close the FX window, but this is likely an easy fix.~~ Issue is fixed.

Replaces #7242.

Rossmaxx · 2024-08-08T02:43:14Z

I said it there and I'll say it again, consider spinning up the gate knob removal into it's own seperate PR. Makes it easier to review.

Rossmaxx

My initial review. It's mostly on style.

src/gui/EffectView.cpp

src/core/Effect.cpp

src/gui/EffectView.cpp

Rossmaxx

Follow up style review

include/EffectLabelButton.h

src/gui/EffectView.cpp

src/gui/widgets/EffectLabelButton.cpp

Rossmaxx

Looks good now.

michaelgregorius

The new effect view looks much more polished and less crowded than the old one! 👍

include/EffectLabelButton.h

include/Effect.h

src/gui/widgets/EffectLabelButton.cpp

michaelgregorius · 2024-08-08T16:01:11Z

src/gui/widgets/EffectLabelButton.cpp

+	setAcceptDrops(false);
+
+	setCursor(QCursor(embed::getIconPixmap("hand"), 3, 3));
+	setStyleSheet("text-align:left;padding:2px;");


This line should be removed and the definition should be done in the style sheets of the default and classic theme. This way users can override the style.

Did you check the look of the changes with the classic theme by the way?

This line is identical to the one in the TrackLabelButton class (which I based this class off of), so I think for consistency it makes sense to keep it this way unless we want to change both of them.

Added classic theme screenshots to the top message. As an aside, it looks like the recent mixer strip PR was not tested in the classic theme since it's completely black...

You're right. I have just checked version 1.2 with the classic theme and here the mixer strip is gray:

Might be something for a new issue.

@michaelgregorius did you convert this to use the stylesheet in your local branch? I'm asking because the latest update you posted seems to use the stylesheet.

@Rossmaxx, in the message below I simply added some style sheet which sets a font size for EffectLabelButtons that are below the effects view, i.e.:

lmms--gui--EffectRackView lmms--gui--EffectLabelButton { font-size: 12px; }

But it has nothing to do with the mixer which this thread seems to be about. Or am I missing something?

I have checked with the current master. The currently selected channel is rendered with a black background in the classic theme:

Might be something for a new issue if that is not intended.

I was supposed to point out the line in question, not the mixer. But since the line is marked in this review, I can't spin up a new one.

The setStyleSheet call to be more precise

I don't really have a branch for the stuff that I have proposed in this comment: #7438 (comment). It's just some local changes that I have tried out without committing them somewhere.

I have noticed though that my original statement that I have made here is likely wrong anyway because the style sheets are applied after construction and therefore they should be able to override any default style sheet settings made in the constructor anyway.

On a side note: I cannot find any calls to setStyleSheet in TrackLabelButton in the current master (anymore?): https://github.com/LMMS/lmms/blob/master/src/gui/tracks/TrackLabelButton.cpp. So if EffectLabelButton is intended to be similar to that class as indicated in this comment then it might need some adjustment. I have also noticed that both classes differ with regards to their base class but that's something for another PR remark.

include/EffectView.h

src/gui/EffectView.cpp

src/gui/widgets/EffectLabelButton.cpp

include/EffectLabelButton.h

src/gui/widgets/EffectLabelButton.cpp

Newline at the end of the `EffectLabelButton` files. Cleanup of includes and forward declarations. Remove commented out friend declaration.

michaelgregorius · 2024-08-09T18:15:02Z

@enp2s0, I took the liberty to apply some changes (forward includes, newline at end of file) myself. That's faster than some back and forth in comment. I hope that's okay for you.

enp2s0 · 2024-08-09T19:06:43Z

@michaelgregorius all good, thanks!

DomClark · 2024-08-09T21:14:51Z

Can we preserve the knob labels? I think it would be clearer that way. Also, how about making the effect buttons more consistent in style to the track buttons (i.e. they're flat and borderless when not pressed)?

enp2s0 · 2024-08-11T01:38:13Z

@DomClark As far as knob labels are concerned the reason for removing them is to significantly reduce the required height. I did try adding labels to the left or right at one point, but it leaves very little space for the effect name/button itself, and increasing the width of the effect chain widget causes it to not fit properly inside the 400x400 windows for instruments.

As far as making the FX buttons match the track buttons, this is a good idea. Working on implementing that now.

Rossmaxx · 2024-08-11T03:21:18Z

I personally am willing to trade a little bit of size reduction for clear knob labels

RoxasKH · 2024-08-12T16:09:46Z

Knob labels are back. I honestly prefer the look without them but it seems I am in the minority on this.

I'm actually in favour of removing label too, as there's tooltips. The issue is that tooltips aren't enough and those knobs should just be there for convenience and should be mirrored in a bar on top of the FX windows (which would have labels, just like it's done for LMMS instruments Volume and Panning knobs), which, as of now, doesn't exist yet.
So it's probably better to keep the for now, although it clearly reduces in effectiveness the compactness the PR was trying to achieve.

Rossmaxx · 2024-08-20T06:38:40Z

Great. Sounds like a nitpick at this point, but can you vertically make the effect button larger? That way, it'll look slightly better.

I second this request. IMO the button should be almost as big as the surrounding effect box so that it is easy to hit.
The text in the buttons also looks very cramped. I think some larger margins would be good here.

This is left and we good for merge.

Gabrielxd195 · 2024-08-21T00:01:31Z

I tried this, but the only problem is the "W/D" label next to the knob, because visually it makes the plugin space look unnecessarily larger and the difference with the old design is not very noticeable. Please remove the label before merging so that it looks more minimalist like the one in the first image.

RoxasKH · 2024-08-22T20:26:22Z

I tried this, but the only problem is the "W/D" label next to the knob, because visually it makes the plugin space look unnecessarily larger and the difference with the old design is not very noticeable. Please remove the label before merging so that it looks more minimalist like the one in the first image.

Wholeheartedly agree (but please don't thumb up yourself). The label honestly shouldn't be in such a place where it's a convenient knob but as i said in a mirrored bigger knob inside the fx window.

The real beneft coming from this PR is actually there only with the size proposed at the beginning, and it's pretty clear how effective it gets. You should still be able to distinguish the 2 knobs as one starts from the center while the other doesn't, and through tooltips. The difference isn't quite as effective with labels added back in

Before vs after. A simple bar with at most w/d knob is also usually the default behaviour afaik in most daws.

Gabrielxd195 · 2024-08-22T21:16:22Z

@RoxasKH Actually without the labels the change is quite noticeable, look at all the space that is saved, it looks more modern. I think that all labels should be optional in LMMS, because that is why we have tooltips.

DomClark · 2024-09-03T23:49:55Z

Tooltips are for additional information, and are not meant to be the sole way of distinguishing UI elements. Labelling elements with tooltips is fine if the elements have sufficiently distinct and suggestive icons, but LMMS has a huge number of near-identical knobs.

Omitting labels hampers feature discovery, as the user has to hover over the appropriate knob even to know that a feature exists. Users familiar with other DAWs, who might reasonably anticipate a particular feature to exist, still have to hover over each knob to find it. Even experienced LMMS users, who know what features are available, have to memorise which knob is which or face the same problem.

Here's some relevant guidance from an article on tooltips by a UX company:

Don’t use tooltips for information that is vital to task completion.

Users shouldn’t need to find a tooltip in order to complete their task. Tooltips are best when they provide additional explanation for a form field unfamiliar to some users or reasoning for what may seem like an unusual request. Remember that tooltips disappear, so instructions or other directly actionable information, like field requirements, shouldn’t be in a tooltip. (If it is, people will have to commit it to their working memory in order to be able to act upon it.)

Surely the name of the knob isn't "additional information" or "reasoning"? And I would consider "which knob does what" to be fairly vital information when trying to do something, that users shouldn't have to memorise.

I realise this is just two knobs for now (the screenshots don't show the decay knob), but it already has minor issues with feature discovery and distinguishing controls, and would quickly degrade the usability of the program if taken as a precedent. While aesthetics should of course be considered, they shouldn't be prioritised over UX.

bratpeki · 2024-09-04T09:59:16Z

Looks great, I would love to test this! Can I get a little summary of everything so far, and points that are worth testing?

bratpeki · 2024-09-04T10:10:39Z

Knob labels are back. I honestly prefer the look without them but it seems I am in the minority on this.

Definitely keep the labels, it makes for better UX.

RoxasKH · 2024-09-04T18:26:52Z

Tooltips are for additional information, and are not meant to be the sole way of distinguishing UI elements. Labelling elements with tooltips is fine if the elements have sufficiently distinct and suggestive icons, but LMMS has a huge number of near-identical knobs.

Omitting labels hampers feature discovery, as the user has to hover over the appropriate knob even to know that a feature exists. Users familiar with other DAWs, who might reasonably anticipate a particular feature to exist, still have to hover over each knob to find it. Even experienced LMMS users, who know what features are available, have to memorise which knob is which or face the same problem.

Here's some relevant guidance from an article on tooltips by a UX company:

Don’t use tooltips for information that is vital to task completion.

Users shouldn’t need to find a tooltip in order to complete their task. Tooltips are best when they provide additional explanation for a form field unfamiliar to some users or reasoning for what may seem like an unusual request. Remember that tooltips disappear, so instructions or other directly actionable information, like field requirements, shouldn’t be in a tooltip. (If it is, people will have to commit it to their working memory in order to be able to act upon it.)

Surely the name of the knob isn't "additional information" or "reasoning"? And I would consider "which knob does what" to be fairly vital information when trying to do something, that users shouldn't have to memorise.

I realise this is just two knobs for now (the screenshots don't show the decay knob), but it already has minor issues with feature discovery and distinguishing controls, and would quickly degrade the usability of the program if taken as a precedent. While aesthetics should of course be considered, they shouldn't be prioritised over UX.

I agree
But it's not just about "aesthetics", slimmer effects improve usability of the drag and drop reordering fx chain, and help to get a better understanding of an fx chain at a glance.
This is obviously still an improvement over the initial design, and i'm all for it, but i believe from my screenshot it's kind of easy to tell how less effective the rework becomes once label are added.
It goes from 27px saved to 14, basically a 50% reduction in height reduction. The first implementation almost slims the original 57px it by 50%, compared to just 28% with added labels.

About "Users familiar with other DAWs, who might reasonably anticipate a particular feature to exist", to my knowledge and after a quick research, users familiar with other DAWs would most likely not anticipate this. As far as i've seen most daws actually don't have additional controls in the effects chain list aside from active/inactive, but just keep them inside the effect window, usually with a topbar. Unless they integrate the effect window inside the chain, like ableton does afaik
The only DAW which has something like that is FL Studio, which only has 1 knobs, Wet/Dry, which is mirrored in the effect window inside a topbar. The thing is, controls here should be there just for convenience, and i believe LMMS will also need in the future to move them or at least mirror those controls inside the effect window. It makes sense, even just from a consistency standpoint, as this is already how it's done for instruments, with convenience controls on the track that are also in the instrument window. Once mirrored, convenience controls could possibly not need a label, as the main control inside the effect window would, at least that's how i see it.
It's also to be noted that mos DAWs don't have such an height for FX chains elements, and it's probably for the same reasons.

All of this to just say either way it's an improvement, i'm ok with it, and there's always room for possible changes later

Gabrielxd195 · 2024-09-04T20:55:30Z

@DomClark It should be noted that in this specific case there is only one knob with a very basic function, with the option to "restore" it to default value and with the tooltips with the information of the knob with the mouse movement, so new users already have enough information to know what that knob does. Adding the label makes this pr pointless because I guess the idea is to simplify the effects chain and make it more intuitive. Also there are cases in music composition and sound design where you need to add many plugins to get the desired sound, and this improvement in the effects chain (without the label) makes the workflow better, because now you can see the plugins at a glance and not have to keep scrolling.

messmerd · 2024-09-04T21:09:31Z

If we want to keep the "W/D" label, maybe it could be placed to the side of the knob and rotated 90 degrees? It shouldn't impact the vertical height that way. I'm not sure how nice it would look, but might be worth trying.

The auto quit knob probably shouldn't be in the EffectView widget at all since it provides such a specific functionality that few people use, though moving it to a topbar in the effect window as @RoxasKH mentioned could be done in a later PR.

michaelgregorius · 2024-09-05T15:59:52Z

I think the best solution might be to make the list widget of the plugins more flexible so that it can deal with effect widgets that render themselves at different sizes.

The effect widget could then be implemented in such a way that it checks a setting which decides whether users want to see the labels or not. Seeing the labels should be the default settings so that users can learn the interface. Once they know it they could decide to switch to a "compact mode" in the settings where the labels are not shown and not much space is used.

sakertooth

The PR looks good to me overall, but:

I think the black background for the effects provided a good contrast. This is subjective though, but I'm wondering why it was changed.
I'm wondering if we should keep the effect name bold or not.

sakertooth · 2024-09-18T23:25:45Z

src/gui/EffectView.cpp


 	setModel( _model );

-	if( effect()->controls()->controlCount() > 0 )
+	if(hasControls)


Suggested change

if(hasControls)

if (hasControls)

Rossmaxx · 2024-09-28T07:41:50Z

Another point that i would like to mention here is that since SlicerT is now larger, the current EffectView looks like this.

Address this and the other concerns and we good.

sakertooth · 2024-09-28T12:37:36Z

Another point that i would like to mention here is that since SlicerT is now larger, the current EffectView looks like this.

Address this and the other concerns and we good.

Addressing this here might be out of scope.

Rossmaxx · 2024-09-28T14:17:49Z

Addressing this here might be out of scope.

It could be extended to include the scope, as it's just a matter of making the incoming layout respond to horizontal resizing, which i believe is far easier because only the effect name widget should be resized and the rest are of fixed sizes, and it's similar to the work done on the mixer ui recently.

michaelgregorius · 2024-09-28T15:12:02Z

IMO fixing the effect view is out of scope of this issue. The ideal fix would make the effect list view flexible, e.g. by using a layout to manage the effects, and would make the effect view scalable so that it can render at arbitrary widths. This is a large change though.

Rossmaxx · 2024-09-28T15:14:10Z

If everyone says so, alrite. Btw when is the name button fix coming? We've been waiting for this for some time.

Rossmaxx · 2024-09-28T15:23:43Z

by using a layout to manage the effects, and would make the effect view scalable

I'd like to avoid just ->move()ing them since the rest is done with layouts.

I think it already does. That's why i suggested the fix.

michaelgregorius · 2024-10-07T15:10:29Z

Adding the following code after the creation of the EffectLabelButton increases the height of the button so that it is easier to interact with:

m_label->setSizePolicy(QSizePolicy::Expanding, QSizePolicy::Expanding);
m_label->setMaximumHeight(height() - 12);

This in turn enables to increase the font size for better readability via the following style sheet snippet:

lmms--gui--EffectRackView lmms--gui--EffectLabelButton {
	font-size: 12px;
}

The result looks as follows:

Rossmaxx · 2024-10-08T01:41:49Z

@michaelgregorius why does it look like the effect view is now larger than the old view?

Gabrielxd195 · 2024-10-08T02:02:34Z

If they leave the design like the one in the previous photo, then this design change will not have been worth it. So simple that it is to remove the label and give us a smaller and more modern design.

michaelgregorius · 2024-10-08T08:50:17Z

@michaelgregorius why does it look like the effect view is now larger than the old view?

Because I have global fractional scaling set to 125% which means that Qt renders the view with more pixels on my screen. I have not made any changes to the height of the effect views.

michaelgregorius · 2024-10-08T08:54:51Z

If they leave the design like the one in the previous photo, then this design change will not have been worth it. So simple that it is to remove the label and give us a smaller and more modern design.

The ideal solution would adjust the list that shows the effect views such that it can manage effect views at different sizes. That way the effect view could be implemented to be configurable to allow different layouts (compact, normal, etc.). However, the current implementation is rather inflexible.

michaelgregorius · 2024-10-08T11:22:38Z

include/EffectLabelButton.h

+
+class EffectView;
+
+class EffectLabelButton : public QPushButton


I have just noticed that this class inherits from QPushButton whereas TrackLabelButton inherits from QToolButton. I think the letter is done so that the button can be put into a checked state when for example an instrument window is opened. So I wonder if the same would make sense for opened effects.

mini fx rewrite without merge conflicts

34daf03

enp2s0 mentioned this pull request Aug 7, 2024

WIP: new effect rack interface and features #7242

Closed

7 tasks

enp2s0 changed the title ~~WIP: mini fx rewrite without merge conflicts~~ mini fx rewrite without merge conflicts Aug 7, 2024

clear checked status of label button when fx window is closed

124a894

Rossmaxx reviewed Aug 8, 2024

View reviewed changes

enp2s0 added 2 commits August 8, 2024 09:56

style & cleanup

9d01945

add spaces

efd8cfe

Rossmaxx reviewed Aug 8, 2024

View reviewed changes

enp2s0 added 3 commits August 8, 2024 10:07

more

eaf94e4

effectlabelbutton style cleanup

889818a

remove unused graphics

5b3ce79

Rossmaxx approved these changes Aug 8, 2024

View reviewed changes

Rossmaxx requested a review from michaelgregorius August 8, 2024 14:26

michaelgregorius requested changes Aug 8, 2024

View reviewed changes

enp2s0 added 6 commits August 8, 2024 23:26

effectlabelbutton cleanup

af9ed03

classes are no longer friends, they hate eachother now

34a48d8

simpler margin calc and connect()

7f541f5

missing space

5127544

one more space

89cb23f

wrong place

809a152

michaelgregorius reviewed Aug 9, 2024

View reviewed changes

include/EffectLabelButton.h Outdated Show resolved Hide resolved

michaelgregorius reviewed Aug 9, 2024

View reviewed changes

src/gui/widgets/EffectLabelButton.cpp Outdated Show resolved Hide resolved

Code cleanup

e6aa0ea

Newline at the end of the `EffectLabelButton` files. Cleanup of includes and forward declarations. Remove commented out friend declaration.

michaelgregorius approved these changes Aug 9, 2024

View reviewed changes

messmerd mentioned this pull request Sep 4, 2024

Remove "gate" option completely #7486

Closed

sakertooth reviewed Sep 18, 2024

View reviewed changes

Rossmaxx added 2 commits September 28, 2024 13:12

Merge branch 'master' into minifx_fixed

a7d7597

Merge branch 'master' into minifx_fixed

f059a2b

Fix build

082e068

Rossmaxx mentioned this pull request Oct 3, 2024

Fix resize issues #7524

Draft

michaelgregorius reviewed Oct 8, 2024

View reviewed changes


		class EffectView;

		class EffectLabelButton : public QPushButton

mini fx rewrite without merge conflicts #7438

Are you sure you want to change the base?

mini fx rewrite without merge conflicts #7438

Conversation

enp2s0 commented Aug 7, 2024 • edited Loading

Rossmaxx commented Aug 8, 2024

Rossmaxx left a comment

Choose a reason for hiding this comment

Rossmaxx left a comment

Choose a reason for hiding this comment

Rossmaxx left a comment

Choose a reason for hiding this comment

michaelgregorius 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

michaelgregorius commented Aug 9, 2024

enp2s0 commented Aug 9, 2024

DomClark commented Aug 9, 2024

enp2s0 commented Aug 11, 2024

Rossmaxx commented Aug 11, 2024

RoxasKH commented Aug 12, 2024 • edited Loading

Rossmaxx commented Aug 20, 2024

Gabrielxd195 commented Aug 21, 2024

RoxasKH commented Aug 22, 2024 • edited Loading

Gabrielxd195 commented Aug 22, 2024

DomClark commented Sep 3, 2024

Don’t use tooltips for information that is vital to task completion.

bratpeki commented Sep 4, 2024

bratpeki commented Sep 4, 2024 • edited Loading

RoxasKH commented Sep 4, 2024

Don’t use tooltips for information that is vital to task completion.

Gabrielxd195 commented Sep 4, 2024

messmerd commented Sep 4, 2024

michaelgregorius commented Sep 5, 2024

sakertooth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rossmaxx commented Sep 28, 2024

sakertooth commented Sep 28, 2024

Rossmaxx commented Sep 28, 2024

michaelgregorius commented Sep 28, 2024

Rossmaxx commented Sep 28, 2024

Rossmaxx commented Sep 28, 2024

michaelgregorius commented Oct 7, 2024

Rossmaxx commented Oct 8, 2024

Gabrielxd195 commented Oct 8, 2024

michaelgregorius commented Oct 8, 2024

michaelgregorius commented Oct 8, 2024

Choose a reason for hiding this comment

enp2s0 commented Aug 7, 2024 •

edited

Loading

RoxasKH commented Aug 12, 2024 •

edited

Loading

RoxasKH commented Aug 22, 2024 •

edited

Loading

bratpeki commented Sep 4, 2024 •

edited

Loading