Add ColorPickerArchitecture Doc #3244
Conversation
msft-github-bot
added
the
needs-triage
|
|
||
| TODO | ||
|
|
||
| ## Ideas for Future Implementations |
There was a problem hiding this comment.
I am not sure if this is better kept in an issue instead of the documentation of the control. This section really doesn't talk about the existing control but rather what things one could potentially change. Especially things like "X should be named Y instead" seems a bit strange in the documentation.
There was a problem hiding this comment.
Lessons learned always seem to get lost. Adding them here ensures they survive with the code. Im adamant this section stays.
There was a problem hiding this comment.
Usually there is a specific document or even database for mistakes made in project(s). "Lessons learned" usually lives in a separate area that is easier to search through instead of the architecture documentation. Putting this inside the individual control documentation makes it a pain to search through later because there isn't a single source for "mistakes made" but you have to go through the individual control documentations. @ranjeshj @MikeHillberg What are your thoughts on this?
There was a problem hiding this comment.
@chingucoding We keep disagreeing on almost everything which is making our discussions quite challenging. If any lesson's learned exists it is internal to Microsoft and not visible publicly. Then, as code goes open source like this, it is lost. It's best to keep it in code in this case. Of course you are right and traditionally there is a separate lesson's learned database though.
There was a problem hiding this comment.
I did not imply that we should have it be in an internal or private database, I am just trying to say that I think it should be in a separate document, not next to the actual current documentation of the control. But that's just my opinion ...
@chingucoding We keep disagreeing on almost everything which is making our discussions quite challenging.
Not sure what you want me to say regarding this. We don't have to agree on anything and yet we can have fruitful discussions.
|
I think naming the file "readme.md" might be better as this document essentially is the "you need to know" information of that control and explains the structure. Also that allows GitHub to show that document when you open said folder on GitHub. I assume, in the ColorPicker section you will also mention the template parts and their purpose right? |
|
I followed the same organizational structure you did for the Navigation view. Readme makes more sense and is what I had originally. However, consistency was more important in this case so developers know where to look. If we change this, it needs to change for NavView as well.
Yes, I will have the completed document done in about an hour. |
|
I am not sure what you are referring to. The NavigationView has a readme.md file on the top level of the control explaining the overall structure while linking to documents in /docs which go into more detail for specific areas. So for consistency, I think its better to have this be a readme.md inside the colorpicker folder instead of sitting under /docs inside the ColorPicker folder. Also, I think there are a few flows/events that are not yet in this document, will you include those too? |
Ah, ok I missed that and didn't follow the original discussion very closely.
When I said ready for feedback that implies feedback on the content that is already included. Of course when the final document is there please be specific and I'll be happy to include more flows where appropriate. It is already quite exhaustive though and is intended only as an overview. |
ranjeshj
added
documentation
dev/ColorPicker/readme.md
Outdated
|
|
||
| ### Template Parts | ||
|
|
||
| The color picker looks for and uses the following parts in the control template. |
There was a problem hiding this comment.
I think those template parts are not looked up by the ColorPicker but the ColorSpectrum right?
There was a problem hiding this comment.
yes, that's a copy-paste error
dev/ColorPicker/readme.md
Outdated
| * `LayoutRoot` : [Grid] The root element of the control. | ||
| * `SizingGrid` : [Grid] The grid representing the actual width/height that the spectrum will be drawn to. | ||
| * `SpectrumRectangle` : [Rectangle] The background rectangle use to represent the color spectrum gradient. This is blended with the overlay rectangle using opacity (see notes below on rendering). This rectangle is only visible in Box shape. | ||
| * `SpectrumEllipse` : [Ellipse] The background ellipse use to represent the color spectrum gradient. This is blended with the overlay ellipse using opacity (see notes below on rendering). This ellipse is only visible in Ring shape. | ||
| * `SpectrumOverlayRectangle` : [Rectangle] The overlay rectangle use to represent the color spectrum gradient. This is blended with the background rectangle using opacity (see notes below on rendering). This rectangle is only visible in Box shape. | ||
| * `SpectrumOverlayEllipse` : [Ellipse] The overlay ellipse use to represent the color spectrum gradient. This is blended with the background ellipse using opacity (see notes below on rendering). This ellipse is only visible in Ring shape. | ||
| * `InputTarget` : [FrameworkElement] The control used to track pointer interaction by the user and get X/Y coordinates to change the selected color in the spectrum. | ||
| * `SelectionEllipsePanel` : [Panel] The panel representing the selected color indicator shown on the spectrum. | ||
| * `ColorNameToolTip` : [ToolTip] The tool-tip used to present the selected color's displayed name to the user. |
There was a problem hiding this comment.
While it's good to know what they are doing inside the template, I am not entirely sure if I need to name something LayoutRoot. Are all properties listed in the TemplatePart sections referenced from the controls code behind or are there parts that are only referenced from inside the template if at all?
In the case of NavigationView, there were a few parts that were named, but not referenced from inside the "logic" of NavigationView. When retemplating, it's quite helpful to know if you actually need to port those parts too or if I can skip them as they are not referenced form the NavigationView code behind.
There was a problem hiding this comment.
I never consider elements template parts based on whether or not they have a name. You can name a control and use it only in the XAML as well. I consider an element a template part only if it is looked up in the visual tree from code-behind (usually within OnApplyTemplate()) which is where this list comes from. So yes, it's considered necessary when re-templating. In the case of LayoutRoot this is needed to re-calculate the spectrum if the overall size changes. I can add a note for how it is used.
There was a problem hiding this comment.
Right I see, thanks for the clarification.
| * `ColorPreviewRectangleCheckeredBackgroundImageBrush` : [ImageBrush] The image brush fill of the checkered background rectangle that must be below the `ColorPreviewRectangle`. The checkered background rectangle itself is not a template part but should still be in a control template -- it just isn't used in code-behind. | ||
|
|
||
| * `ThirdDimensionSlider` : [ColorPickerSlider] The 1-dimensional slider representing the third channel of the selected color not shown in the `ColorSpectrum`. This will always be an HSV channel. | ||
| * `ThirdDimensionSliderGradientBrush` : [LinearGradientBrush] The image brush fill of the background rectangle that must be below the `ThirdDimensionSlider`. It visually represents a gradient of values the user can choose from. The background gradient rectangle itself is not a template part but should still be in the control template -- it just isn't used in code-behind. The background image brush is not managed by the slider but instead by the `ColorPicker`. |
There was a problem hiding this comment.
Why should I include it if it is not referenced in code behind?
There was a problem hiding this comment.
Because the GradientBrush has to live inside something.
There was a problem hiding this comment.
And the gradientbrush is set through code behind? In that case it is implicitly referenced from code behind.
dev/ColorPicker/readme.md
Outdated
|
|
||
| * The 'ThirdDimension' slider really should not be named 'dimension'. 'Channel' is the standardized term so 'ThirdChannelSlider' would be a better choice. 'Components' is also the term used in control properties. | ||
|
|
||
| * HSV color representation should be externally exposed by the color picker for those applications that require higher levels of precision. A new HsvColor struct should be added to the existing RGB struct already in WinUI. |
There was a problem hiding this comment.
This sounds like a feature gap, so I think it's probably better to create a proposal for this instead of just leaving this in here or a "things to learn from this" document.
There was a problem hiding this comment.
Different opinion, that's fine. This is a roll-up of lessons learned various places and hopefully if the ColorPickerButton ideas from the community toolkit ever come back to this repo changes like this can be made.
There was a problem hiding this comment.
And why not make this change regardless of the WCT ColorPickerButton? This is a feature gap and just saying "yeah this was a mistake" and not fixing this seems a bit strange in my opinion.
There was a problem hiding this comment.
I'm inclined to agree with @chingucoding - a readme should document existing design and behavior; if there's additional bug fixes or feature requests, those should be filed as issues rather than being put here.
There was a problem hiding this comment.
If I'm in the minority it can be changed. I just have a differing opinion and didn't want to change that without a majority consensus. That said, I think there are two types of topics in this section: feature additions (like HsvColor being external) and architecture changes. The feature additions probably should be separated into different issues. Architecture changes to the control though still seem like a better fit in this document.
There was a problem hiding this comment.
What I've done is separated features from implementation/architectural changes. Architectural changes should still live in this file as I don't see them changing for a long time if-ever. However, if ColorPicker is ever re-written it would be good to make some changes.
This means the HsvColor Property is now a feature-request #3281
| 1. On Windows version 1607 (RS1) and before, the spectrum is rendered as an array of individual bytes, converted to a WritableBitmap, set as the source of an ImageBrush and then finally set as the background/fill of a spectrum rectangle. | ||
| 2. On Windows version 1703 (RS2) and later, the spectrum images are placed in a loaded image surface, which is then put into a SpectrumBrush. This technique uses the Composition APIs. | ||
|
|
||
| Rendering is done asynchronously for performance. However, it still can take 10s of milliseconds to re-render the spectrum. This time becomes negligible as the spectrum does not need to re-render itself as the selected color changes. A special trick is used as documented in the `ColorSpectrum.cpp` source: |
There was a problem hiding this comment.
Where is that number coming from, did you measure it? Or is that already mentioned somewhere in the documentation?
There was a problem hiding this comment.
I wouldn't focus on this number so much as it's approximate. Visually you can see the delay. This was also discussed here: https://github.com/maxkatz6/Avalonia.ColorPicker/issues/1#issuecomment-686168526
There was a problem hiding this comment.
I just wanted to know where this number is from.
| @@ -0,0 +1,141 @@ | |||
| # ColorPicker Architecture | |||
|
|
|||
| This document is primarily intended for developers and contributors to WinUI. For additional information see: | |||
There was a problem hiding this comment.
nit: For information on how to use the ColorPicker see:
If you are already in the source code of WinUI and see the readme of ColorPicker, either you need this information and the API/design docs aren't that helpful anymore to you or you are "lost" and actually want to use the control, not modify it, that's at least how I would see it.
There was a problem hiding this comment.
Being overly specific here isn't needed. (1) This section could include any related links other than those listed (2) As a developer you most likely already know the current links therefore it's there to save time only.
|
|
||
| Most of the detailed functionality specific to the `ColorPicker` is implemented in the primitive controls outlined below. See the links above and at the top of this document for full details on the usage of the `ColorPicker` itself. | ||
|
|
||
| The `ColorPicker` functions by monitoring for changing in properties or user selection and then updating the selected color and graphical controls (preview rectangles, sliders, input text boxes, etc.) accordingly. It also ensures the various properties and input is validated before computing the updated selected color. |
There was a problem hiding this comment.
So when I click on the colorspectrum what happens after that? How do changes in properties of the subcomponents get reflected in the overall control?
There was a problem hiding this comment.
The color spectrum is linked to the color picker using the Vector4 HsvColor property and the ColorChanged event. The other properties are going ColorPicker -> ColorSpectrum and are simply set in the style. I would encourage anyone to read the code for this level of information.
There was a problem hiding this comment.
Added a note to clarify this section regardless
dev/ColorPicker/readme.md
Outdated
| * `SaturationTextBox` : [TextBox] The saturation channel input text box the user can modify. | ||
| * `ValueTextBox` : [TextBox] The value channel input text box the user can modify. | ||
| * `AlphaTextBox` : [TextBox] The alpha channel input text box the user can modify (shared for both RGB/HSV). | ||
| * `HexTextBox` : [TextBox] The hexadecimal HTML color input text box the user can modify. This is always in RGB. |
There was a problem hiding this comment.
Afaik hexadecimal colors are quite universal and not HTML specific. Also it's not always RGB but also ARGB if alpha is enabled.
There was a problem hiding this comment.
Yes, HTML isn't needed here
There was a problem hiding this comment.
RGB in this context means color representation not pixel format.
There was a problem hiding this comment.
Is there a HSL/HSV hexadecimal representation? Afaik hexadecimal always refers to a (A)RGB representation.
There was a problem hiding this comment.
I'm not sure if one is widely used. The problem arises with 0..359 for Hue being outside a byte. That said, there is no harm in being overly specific here I added it so no one would need to think about it.
dev/ColorPicker/readme.md
Outdated
|
|
||
| ### Rendering the Spectrum | ||
|
|
||
| There are presently two different techniques used for generating/rendering the color spectrum depending on the version of Windows. |
There was a problem hiding this comment.
I can remove that. In my mind I know this is going to change in the future which is why I think it's better to qualify it with a time. In WinUI 3.0 for example I'm sure the pre-RS2 method of rendering will go away.
There was a problem hiding this comment.
Well a lot of these sections will change over time. Having presently does not help if it's outdated, instead we need to watch out that the documentation stays up to date.
There was a problem hiding this comment.
Either way, it was removed
dev/ColorPicker/readme.md
Outdated
|
|
||
| * `LayoutRoot` : [Grid] The root element of the control. Changes to the size of the root element will trigger re-calculation of the spectrum. | ||
| * `SizingGrid` : [Grid] The grid representing the actual width/height that the spectrum will be drawn to. | ||
| * `SpectrumRectangle` : [Rectangle] The background rectangle use to represent the color spectrum gradient. This is blended with the overlay rectangle using opacity (see notes below on rendering). This rectangle is only visible in Box shape. |
There was a problem hiding this comment.
Either "used" or "is used" instead of "use" here and in the sentences below.
dev/ColorPicker/readme.md
Outdated
| * `SpectrumEllipse` : [Ellipse] The background ellipse use to represent the color spectrum gradient. This is blended with the overlay ellipse using opacity (see notes below on rendering). This ellipse is only visible in Ring shape. | ||
| * `SpectrumOverlayRectangle` : [Rectangle] The overlay rectangle use to represent the color spectrum gradient. This is blended with the background rectangle using opacity (see notes below on rendering). This rectangle is only visible in Box shape. |
dev/ColorPicker/readme.md
Outdated
|
|
||
| A custom slider implementation used for the sliders in the `ColorPicker` control template. These sliders are unique in that they have a gradient background rendered to correspond with a defined HSV color channel (important notes on background below). Sliders provide a one-dimensional method of modifying a color channel. They are used for the third color channel not on the `ColorSpectrum` or the alpha channel. | ||
|
|
||
| Visually to the user, these sliders have a background rendered based on the assigned HSV color channel or alpha. For example, the background could be a gradient of the selected color from minimum saturation to maximum saturation if the slider represents the saturation channel. However, the slider itself does not draw this background gradient. The gradient is managed by the parent `ColorPicker` and shown underneath the slider as the fill of a background rectangle. For the alpha channel slider there is a further checkered background that is rendered separately and then blended using opacity. `ColorPickerSlider` and `ColorPicker` are intrinsically linked together and `ColorPickerSlider` is not useful as a stand-alone control. |
There was a problem hiding this comment.
You say ColorPickerSlider is not a useful as a stand-alone control, yet in the last section you argue that it should be made an independent control. Also is it relevant to mention what is useful as a stand-alone control? Isn't that something everyone has to decide for themselfes and not something the documentation decides?
There was a problem hiding this comment.
Linking two controls together like this isn't very good architecture which is the fundamental concern. Additionally, a lot can be simplified by cleanly separating these controls. That simplification is mentioned.
There was a problem hiding this comment.
You say ColorPickerSlider is not a useful as a stand-alone control
Is as it's coded now.
yet in the last section you argue that it should be made an independent control.
Last section is future areas of improvement.
These are unrelated topics.
There was a problem hiding this comment.
It's still sending mixed messages here. But that's not my main concern, in my opinion documentation shouldn't say "part x of our control is not useful for you". In my opinion documentation should be neutral, not opinionated. But if you think it's fine to include that, then do that, just saying my opinion here.
There was a problem hiding this comment.
It's important that developers don't think they can just use the ColorPickerSlider and expect the background gradient to be rendered. It is important they understand it is done by the ColorPicker itself. This is not an opinion.
There was a problem hiding this comment.
Then say that the background does not render not on it's own instead of "not helpful on it's own". The fact that it doesn't render the background is a fact yes, but if it's useful for me or not is still up for me to decide :)
There was a problem hiding this comment.
"Is not intended as a standalone control".would dial down the wording but the intent stays the same. You are interpreting this very strongly and I don't think others would necessarily read it that way.
There was a problem hiding this comment.
Could be that I am interpreting "is not useful" too strongly here, but might not be the only one concerned by that.
There was a problem hiding this comment.
Yeah, it might be better to say that the ColorPickerSlider is not intended to be used outside of the context of a ColorPicker than to say that it's "not useful". It was created to meet the needs of the ColorPicker control; I can't think of any way it would be used as a standalone control by itself, which is why it's under the Primitives namespace. It's effectively just a custom part of ColorPicker.
There was a problem hiding this comment.
I find the spectrum is quite useful by itself in custom color picker implementations even though it's considered a primitive. The slider though, not so much. Edit: It would be quite useful to clean things up a few places with the slider though to simplify the template.
|
|
||
| The `ColorPicker` functions by monitoring for changing in properties or user selection and then updating the selected color and graphical controls (preview rectangles, sliders, input text boxes, etc.) accordingly. It also ensures the various properties and input is validated before computing the updated selected color. | ||
|
|
||
| Internally, the `ColorPicker` always represents the selected color in HSV instead of RGB. For cases where precision is important this creates some problems as the color is always converted to RGB before being available from external APIs. This is in contrast to the `ColorPickerSlider` and `ColorSpectrum` which pass the color information directly in HSV to avoid loss of precision. |
There was a problem hiding this comment.
It's been a long time since I worked on this control, but if I recall correctly, we didn't expose HSV in a public API because there wasn't a clear scenario for it. If there is one, could you file a feature request on that? It would be simple enough to add to the API surface if there was a need.
There was a problem hiding this comment.
It doesn't harm anything to have this available externally. If there was a scenario where it's possible to edit the same selected color using two different pickers this would be important. Further value would be in adding an HsvColor struct as well. Fundamentally, I don't think WinUI should make an assumption of how the control is used by applications and artificially restrict accuracy like this.
There was a problem hiding this comment.
The issue is that if you're locked in once you ship an API - from that point on, apps will have potentially taken a dependency on that API, meaning it can no longer be changed unless there's an very pressing reason that outweighs the huge downside of breaking existing applications. If there was some deficiency in the API, it's now basically encased in amber. For that reason, we generally want to have a clear scenario whose needs are met by the API so we can validate that it does actually meet the need in question as designed. I would recommend filing a feature request on this so it can be discussed.
There was a problem hiding this comment.
I definitely agree with you in concept and exposing an HsvColor with a Vector4 type is not a great API. However, there are lots of other examples of this including in the Community Toolkit. I think there is enough to design something clean and future-proof here. Regardless, I've already broken this out into a feature request #3281 as it isn't a lesson's learned or something to keep in mind doing a clean re-implementation.
This is actually a good time to advocate again for keeping the lesson's learned in this document. They are changes that should be made to make the design cleaner -- however, changing that probably won't happen as they would be breaking. It is definitely something for posterity to consider in other implementations though. If the WPF team left notes like that UWP would have been a lot better I'm sure.
There was a problem hiding this comment.
How about adding lessons learned in an improvements.md file? That way everyone knows where to look for those kind of things. And whether that is in readme.md or another file does not make a difference regarding whether one would actually learn from that. If WPF had that in readme.md files or in improvements.md files wouldn't have made a difference I think. What do you think about that @robloo ?
There was a problem hiding this comment.
I have no concern moving to a separate file. The information would still be retained alongside the code. I can justify creating a separate file as well since the topic is somewhat different from architecture. Should this be in a Docs folder though? Not sure the criteria decided for NavView.
There was a problem hiding this comment.
In the case of NavView, I created a docs folder since we also needed to document NavViewItem and rendering wasn't easy to explain. I think in the case of ColorPicker, just putting a separate file at the same level of the readme is probably fine.
| * `PreviousColorRectangle` : [Rectangle] A rectangle with the fill set to the previously selected color. | ||
| * `ColorPreviewRectangleCheckeredBackgroundImageBrush` : [ImageBrush] The image brush fill of the checkered background rectangle that must be below the `ColorPreviewRectangle`. The checkered background rectangle itself is not a template part but should still be in a control template -- it just isn't used in code-behind. | ||
|
|
||
| * `ThirdDimensionSlider` : [ColorPickerSlider] The 1-dimensional slider representing the third channel of the selected color not shown in the `ColorSpectrum`. This will always be an HSV channel. |
There was a problem hiding this comment.
"This will always be an HSV channel" seems a bit odd to put here since everything internally functions based on HSV channels. Since you've already noted that above, this seems unnecessary to say.
There was a problem hiding this comment.
Repeating little hints like this usually helps in understanding documentation. It also is helpful to those that don't read other sections.
There was a problem hiding this comment.
That's fair, I have no strong opinions on the matter.
dev/ColorPicker/readme.md
Outdated
|
|
||
| ## ColorPickerSlider | ||
|
|
||
| A custom slider implementation used for the sliders in the `ColorPicker` control template. These sliders are unique in that they have a gradient background rendered to correspond with a defined HSV color channel (important notes on background below). Sliders provide a one-dimensional method of modifying a color channel. They are used for the third color channel not on the `ColorSpectrum` or the alpha channel. |
There was a problem hiding this comment.
These sliders are unique in that they have a gradient background rendered to correspond with a defined HSV color channel (important notes on background below).
They also differ from standard sliders in that the tooltip they display shows the HSV channel and color name, not just a number - might be worth adding that as well.
dev/ColorPicker/readme.md
Outdated
|
|
||
| * The 'ThirdDimension' slider really should not be named 'dimension'. 'Channel' is the standardized term so 'ThirdChannelSlider' would be a better choice. 'Components' is also the term used in control properties. | ||
|
|
||
| * HSV color representation should be externally exposed by the color picker for those applications that require higher levels of precision. A new HsvColor struct should be added to the existing RGB struct already in WinUI. |
There was a problem hiding this comment.
I'm inclined to agree with @chingucoding - a readme should document existing design and behavior; if there's additional bug fixes or feature requests, those should be filed as issues rather than being put here.
|
Could you add the documents to the MUXControls.sln and the MUXControlsInnerloop.sln file so that you can view them when opening the solution view? For the NavigationView, I added it to the solutionfolder "NavigationView". |
|
Its generally not a good thing to include documents in the solution or project files. These are not code and are always treated separately in all projects I've worked on. |
|
The main point for me is that I can just look up something in solution view, and not have to either find the file in folder view which is bloated or even leave visual studio to quickly look something up. |
|
@chingucoding Solutions updated to include the docs Hopefully this can be merged now. |
|
@llongley Can you approve this so it will be unblocked? I believe all comments were addressed and there is nothing holding up a merge. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Description
Add initial documentation to the ColorPicker directory explaining how the ColorPicker and related controls are designed and function.
This is still WIP but feedback is already welcome.Docs are now complete.Motivation and Context
See the discussion in #2812
How Has This Been Tested?
N/A
Screenshots (if appropriate):
N/A