-
-
Notifications
You must be signed in to change notification settings - Fork 994
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
mini fx rewrite without merge conflicts #7438
base: master
Are you sure you want to change the base?
Conversation
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My initial review. It's mostly on style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow up style review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new effect view looks much more polished and less crowded than the old one! 👍
setAcceptDrops(false); | ||
|
||
setCursor(QCursor(embed::getIconPixmap("hand"), 3, 3)); | ||
setStyleSheet("text-align:left;padding:2px;"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newline at the end of the `EffectLabelButton` files. Cleanup of includes and forward declarations. Remove commented out friend declaration.
@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. |
@michaelgregorius all good, thanks! |
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)? |
@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. |
I personally am willing to trade a little bit of size reduction for clear knob labels |
Updated screenshots? |
Great. Sounds like a nitpick at this point, but can you vertically make the effect button larger? That way, it'll look slightly better. |
Consider wrapping the knob + label on a QVBoxLayout and see if it fixes it. |
The label and the knob are already wrapped together -- I'm just running |
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. |
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. |
@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. |
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:
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. |
Looks great, I would love to test this! Can I get a little summary of everything so far, and points that are worth testing? |
I agree 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 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 |
@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. |
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 |
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. |
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:
The new design in action:
![image](https://private-user-images.githubusercontent.com/22487040/355902452-741c6762-ed83-458c-9851-29bb4457a514.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjYxNDQ4OTYsIm5iZiI6MTcyNjE0NDU5NiwicGF0aCI6Ii8yMjQ4NzA0MC8zNTU5MDI0NTItNzQxYzY3NjItZWQ4My00NThjLTk4NTEtMjliYjQ0NTdhNTE0LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MTIlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTEyVDEyMzYzNlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWQxNGUxZjIxOTMyZDUyZDU1YmU1NTI3MDRkN2NhOTZmNjFiNjg3MTEyODA2NjA1MTEwNTkwNzFlN2VkNGFlNzYmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.2P7TWVXdATDDtTA3g94_SNECEfrricb1FfsF8iJTAls)
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):
![image](https://private-user-images.githubusercontent.com/22487040/356457020-1237f7a6-b19e-4e6f-a545-22327df9da58.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjYxNDQ4OTYsIm5iZiI6MTcyNjE0NDU5NiwicGF0aCI6Ii8yMjQ4NzA0MC8zNTY0NTcwMjAtMTIzN2Y3YTYtYjE5ZS00ZTZmLWE1NDUtMjIzMjdkZjlkYTU4LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MTIlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTEyVDEyMzYzNlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTcyYmQxYmI3ZThmZTJiOGRiODAxODUyY2Q4ZjBkOWZhMDZmMWM5MDBjZWQ3OTU1Yjg5YjQ4NGMwN2JhNDc5OWUmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.uVQEXK62Ckyrvr0z2Qv9bPolrQxUWxod6BLiiw55e84)
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.