Conversation
|
I suggest to use OkHSL by default, so tab with this color space is first on the tab list |
|
Whats the status of this? Creating an issue and adding links to explain what it is and why its needed. |
I've created this PR ages ago and never really finished it, I imagine a lot will need to be updated in the PR to bring it in sync with current master (this PR was made even before pixieditor and colorpicker were migrated to avalonia). It's also unfinished. Can't promise when or if I will work on it again |
|
Thanks for the quick response; I'll wait and see what the maintainers response to the issue is and if/how they'd want it done. Even if you aren't planning to pick it up again, best to leave the PR around for now so we can have a reference of how you approached it. If the issue is accepted and nobody else plans to work on it I'll probably pick it up from there. |
|
@weltkante Equbuxu was the color picker maintainer actually. I don't plan to do this in a near future. I do realize it's a desired feature, but right now I have more pressing things to finish, so feel free to grab this issue |
|
Ok, so I've read through this PR and think I understand what individual changes were done. I also looked into what is necessary to resolve the merge conflict against master - besides file renames, whitespace and member reordering, which are easily undone for a rebase, the only thing thats conflicting is the color sliders having introduced a grayscale mode when being disabled - I think I can come up with some way to transfer that logic into the new slider type abstraction, but its more complicated than simply copy/pasting it over there, since the slider type implementation doesn't have access to the enabled state, so it needs some thinking. Besides that, replicating the changes into the Avalonia version is also doable for me, I think. So I'd be up to making an attempt at the issue/PR. The main question that remains is whether the entire design of having separate model properties for every color space on the same model class? Like the NotifyableColor.cs here: https://github.com/PixiEditor/ColorPicker/pull/43/changes#diff-8be0f58323e469440fd20e6c7f6edf5841d052f8462471ef292b8ca40202003b or the ColorState.cs here: https://github.com/PixiEditor/ColorPicker/pull/43/changes#diff-f78781faeeeeeabe7fcdd96378fba75df905750610d53f8a4bb28498c637f04d for example. That feels kinda awkward in my opinion, but if that design is ok to go with for you, I don't mind replicating it for a full PR. If you don't like it and want me to give a try of my own on a design I'd attempt something more modular by separating the models for separate color spaces, instead of putting it all on one model. Obviously that'd be more review work, as well as causing issues with compatibility and be a breaking change, since in the current version the HSL/HSV properties are already duplicated this way. Your call on chosing how to approach this (preferring modularity/cleanup or compatibility) or if now is not a good time to introduce additional integration work on the PixiEditor (I assume its a major user of the control). In that case we can keep leaving this on hold, I'm ok with that too. |
|
@weltkante I am glad you want to give it a try! As much as I would like to go with modular approach, I believe it may be a significant problem for compatibility. I am not even talking about PixiEditor itself, because adjust it to the new model shouldn't be a big problem. However a lot of projects use our Color Picker and I think compatibility is more important in that case. Moreover, PixiEditor acutally does not use the main NuGet version of this ColorPicker, we have a separate branch that we just build along with PixiEditor. This branch contains gradients support. If you were to change the entire model, it could be a problem with integrating it with gradients. You can actually see how are they implemented here So if you want to give it a try, let's do it with the compatible approach |
4 participants
No description provided.