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

Change colors for SampleTrack #816

Closed
wants to merge 1 commit into from
Closed

Conversation

ycollet
Copy link
Contributor

@ycollet ycollet commented Jun 6, 2014

Allow the user to change the background and foreground colors for sampletrack.

lmms_sample_track

@diizy
Copy link
Contributor

diizy commented Jun 6, 2014

On 06/06/2014 09:13 AM, ycollet wrote:

Allow the user to change the background and foreground colors for
sampletrack.

It's already changeable in the theme, so there's no point in this feature.

I'm completely against this. For reasons, see the earlier threads where
this idea has been discussed.

@diizy
Copy link
Contributor

diizy commented Jun 6, 2014

Furthermore, your implementation messes up the themeability of sampletracks, breaks themes, etc. This should NOT be merged.

@diizy
Copy link
Contributor

diizy commented Jun 6, 2014

For future reference, please discuss things like this on the mailing list before making an implementation, it saves unnecessary work for everyone involved.

@ycollet
Copy link
Contributor Author

ycollet commented Jun 6, 2014

The thing was to be able to change color like with a bb_track. Not via fine tuning the css file, but via a menu and on a sample basis.

@ycollet
Copy link
Contributor Author

ycollet commented Jun 6, 2014

For the theme breakage, maybe it's due to a logic problem I introduced. It can be fixed.

@diizy
Copy link
Contributor

diizy commented Jun 6, 2014

And there's several reasons why that is a bad idea, and why it is causing problems with the bb_track currently.

We already have a perfectly fine theming system where the colours of all track content objects can be defined. Users can create and use themes that set these objects to colours that are consistent and match each other.

A feature that allows changing the tco colour is problematic because it completely contradicts the theming paradigm. When a user sets a theme, and the theme defines the colours of sampletracks to be eg. blue, then the user expects sampletracks to be blue. Then if the user opens a project where the sampletracks have been set red, then the software is not behaving in the way the user asked it to.

The bb-track colour changing feature is kind of grandfathered in, it was implemented before theming support, and it's being addressed - there are github issues where the issue with bb-track colours is discussed, and the problems with it are explained more clearly there.

IF we are going to add a mechanism to change the colours of the individual tco's - sampletracks, patterns, etc. - then it must be done in a way that doesn't contradict the theming paradigm. We don't want to duplicate the same issues we currently have with bb-tracks. In either case, this should be discussed more, until we get to a conclusion on what's the best approach to deal with this.

I suggest closing this pull request for now and dealing with it later when we have a decision and an implementation plan for this issue.

@ycollet
Copy link
Contributor Author

ycollet commented Jun 6, 2014

OK, thanks for these informations.
The idea was not to let the user modify the theme via a menu, but to let the author of a song to change samples color and save these changes in the song. I think this possibility improves the readability of a song.
But if a user want to change its lmms theme, I agree he must use the css file to do this and not change this via menu.

I will leave the pull request open for 1 or 2 days to "let the discussion flow" :) After that, I will close the pull request.

@diizy
Copy link
Contributor

diizy commented Jun 6, 2014

I'm working now on the bb track view, for an implementation that allows both setting the colour in the theme, and setting a custom colour, in a way where the theme colour is used when a custom colour hasn't been set - as was discussed earlier on github, you can find the discussion if you browse the issues...

After that, if we want the same feature for sampletracks (which should still be discussed first), then you can use the bb-track implementation as a model to see how to implement the same thing for sampletracks. But again, I think this should be discussed first, the opinions on this in the previous discussion were mixed...

@ycollet
Copy link
Contributor Author

ycollet commented Jun 6, 2014

OK, thanks for this information.
Do you want me to close th pull request now or do I leave this open for a little while ?

@diizy
Copy link
Contributor

diizy commented Jun 6, 2014

I'd say close it for now, we can continue discussion in the earlier github issue or on the mailing list.

@ycollet
Copy link
Contributor Author

ycollet commented Jun 6, 2014

OK, closed

@ycollet ycollet closed this Jun 6, 2014
@ycollet
Copy link
Contributor Author

ycollet commented Jun 6, 2014

Where is the issue related to sampletrack color ?

----- Mail original -----
De: "Vesa V" notifications@github.com
À: "LMMS/lmms" lmms@noreply.github.com
Cc: "ycollet" ycollette.nospam@free.fr
Envoyé: Vendredi 6 Juin 2014 09:32:52
Objet: Re: [lmms] Change colors for SampleTrack (#816)

I'd say close it for now, we can continue discussion in the earlier github issue or on the mailing list.


Reply to this email directly or view it on GitHub .

@diizy
Copy link
Contributor

diizy commented Jun 6, 2014

Nowhere, but bb track colours have been discussed here: #676

@diizy
Copy link
Contributor

diizy commented Jun 6, 2014

You can also look at #817 to see how the issue was solved for bb-tracks.

@musikBear musikBear mentioned this pull request Apr 12, 2015
29 tasks
@Reaper10
Copy link

Reaper10 commented Sep 7, 2015

The Change colors for SampleTrack options could also be in the gear menu of the Sample Track

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants