Skip to content
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

Add 'Change color' option to mixer channels #5085

Closed
wants to merge 7 commits into from

Conversation

v0idzz
Copy link
Contributor

@v0idzz v0idzz commented Jul 20, 2019

This enables the user to choose whatever color they want for a mixer channel. This is my first... bigger(?) PR, so don't be surprised if something's wrong. 😝

@LostRobotMusic
Copy link
Contributor

LostRobotMusic commented Jul 20, 2019

I gave it a test, it seems to work fine for the most part. I did notice one very minor bug where resetting the color of the selected bus to default doesn't update the color on the screen until you click on the channel or switch to another channel.

Also, how the colors are handled may need some discussion. From what I can see, any unselected channels are combined with the color black, which can make the channel colors look ugly. Maybe instead, we should make the default-colored channels the dark color, and then mix the selected channel with the color white? Any opinions? Maybe that would break some LMMS themes, I'm not sure.

Otherwise, I did not notice any bugs or problems. 👍

@PhysSong
Copy link
Member

The bug fixed. 👍

@enp2s0
Copy link
Contributor

enp2s0 commented Jul 22, 2019

I agree with @douglasdgi, we should lighten the selected channel rather than darkening all the unselected ones. Qt has a function QColor::lighten() that could be used for this. I don't think its a big deal if it breaks themes, its not a major change and it will look different but not necessarily bad.

@qnebra
Copy link
Collaborator

qnebra commented Jul 22, 2019

It colors "song editor" too, or only mixer?

@v0idzz
Copy link
Contributor Author

v0idzz commented Jul 23, 2019

It colors "song editor" too, or only mixer?

It has no effect on the song editor.

I agree with @douglasdgi and @noahb01. Pushing changes which make it work the opposite way to what I have implemented.

@Reflexe Reflexe requested a review from PhysSong August 23, 2019 17:43
@Reflexe Reflexe added needs code review A functional code review is currently required for this PR needs style review A style review is currently required for this PR needs testing This pull request needs more testing labels Aug 23, 2019
@PhysSong
Copy link
Member

To remove the GUI dependency added in this PR, some extra changes are needed to loading/saving FX channel settings.

@ryuukumar
Copy link
Member

Didn't know this existed... made a PR #5589 which does this same thing.

@claell
Copy link
Contributor

claell commented Jul 21, 2020

@russiankumar Too bad. Is there any chance this can get reviewed, tested and merged soon?

@ryuukumar
Copy link
Member

I kind of see no point, considering the newer one can already be merged without any rebasing hassle, while this one will have to wait. The newer one was already tested by some as well, seems to work as expected, so I’d just go with that one. Though I want a mod to confirm this, since technically I’m biased.

@claell
Copy link
Contributor

claell commented Jul 21, 2020

Probably the best, especially since this seems not to be very active at the moment.

@Spekular
Copy link
Member

Superceded by #5589, which is now merged.

@Spekular Spekular closed this Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs code review A functional code review is currently required for this PR needs style review A style review is currently required for this PR needs testing This pull request needs more testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants