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

Adding spec for Date/TimePicker design updates #4490

Merged
merged 22 commits into from
May 19, 2022

Conversation

anawishnoff
Copy link
Contributor

@anawishnoff anawishnoff commented Mar 11, 2021

Replacing the PR here: microsoft/microsoft-ui-xaml-specs#114, moving this onto the main repo.

@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Mar 11, 2021
@ranjeshj ranjeshj added area-DateTimePickers DatePicker, TimePicker, CalendarDatePicker, CalendarView SpecInReview team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Mar 12, 2021
@anawishnoff anawishnoff marked this pull request as draft March 12, 2021 16:27
@anawishnoff anawishnoff marked this pull request as ready for review March 16, 2021 14:28
specs/DateTimePicker-Visual-Updates-Spec.md Outdated Show resolved Hide resolved
specs/DateTimePicker-Visual-Updates-Spec.md Show resolved Hide resolved
specs/DateTimePicker-Visual-Updates-Spec.md Outdated Show resolved Hide resolved
specs/DateTimePicker-Visual-Updates-Spec.md Outdated Show resolved Hide resolved
specs/DateTimePicker-Visual-Updates-Spec.md Outdated Show resolved Hide resolved
specs/DateTimePicker-Visual-Updates-Spec.md Show resolved Hide resolved
specs/DateTimePicker-Visual-Updates-Spec.md Show resolved Hide resolved
specs/DateTimePicker-Visual-Updates-Spec.md Outdated Show resolved Hide resolved
specs/DateTimePicker-Visual-Updates-Spec.md Show resolved Hide resolved
specs/DateTimePicker-Visual-Updates-Spec.md Outdated Show resolved Hide resolved
@anawishnoff
Copy link
Contributor Author

@MikeHillberg @jevansaks - I just pushed updates based on the spec review. There's still one to-do about adding a reflection example using the WPF Visual Brush. Is that needed?

@oldnewthing
Copy link
Member

I don't think a reflection example is really necessary. It's not in line with the intent of this API.

specs/DateTimePicker-Visual-Updates-Spec.md Outdated Show resolved Hide resolved
specs/DateTimePicker-Visual-Updates-Spec.md Outdated Show resolved Hide resolved
specs/DateTimePicker-Visual-Updates-Spec.md Outdated Show resolved Hide resolved
specs/DateTimePicker-Visual-Updates-Spec.md Outdated Show resolved Hide resolved
specs/DateTimePicker-Visual-Updates-Spec.md Outdated Show resolved Hide resolved
specs/DateTimePicker-Visual-Updates-Spec.md Show resolved Hide resolved
specs/DateTimePicker-Visual-Updates-Spec.md Show resolved Hide resolved
specs/DateTimePicker-Visual-Updates-Spec.md Outdated Show resolved Hide resolved
@mdtauk
Copy link
Contributor

mdtauk commented Mar 23, 2021

MonochromaticOverlayPresenter implies the control will only render greyscale or black and white elements.

@mdtauk
Copy link
Contributor

mdtauk commented Mar 23, 2021

MonochromaticOverlayPresenter implies the control will only render greyscale or black and white elements.

What about something like RecolorOverlayPresenter

@anawishnoff
Copy link
Contributor Author

Thanks @oldnewthing - I've applied your suggestions and made more changes based on your feedback.

@mdtauk - I think the discussion/reasoning for "monochromatic" over something with "color" was that it's not an all-purpose tool to replace colors. @jevansaks can fill in the details that I've missed there, though

@jevansaks
Copy link
Member

@mdtauk - I think the discussion/reasoning for "monochromatic" over something with "color" was that it's not an all-purpose tool to replace colors. @jevansaks can fill in the details that I've missed there, though

Right, it turns all colors in the SourceElement into one single color. Hence, monochrome.

@mdtauk
Copy link
Contributor

mdtauk commented Mar 23, 2021

@mdtauk - I think the discussion/reasoning for "monochromatic" over something with "color" was that it's not an all-purpose tool to replace colors. @jevansaks can fill in the details that I've missed there, though

Right, it turns all colors in the SourceElement into one single color. Hence, monochrome.

I think I get that, like Transparent = Red for instance. But it is essentially a recolouring of elements, or a Color Swapper - and is not restricted to a greyscale palette. I guess its more of a designer interpretation, rather than a technical one.

Also, has there been any thought put to swapping background and foreground colours? The text colour would adapt to contrast with the background colour.

@jevansaks
Copy link
Member

I guess its more of a designer interpretation, rather than a technical one.

I was trying to see if there was an industry term for this operation but I couldn't find it. It reminds me of a DirectX fixed function operation, but I can't think of what it would be in Photoshop.

Also, has there been any thought put to swapping background and foreground colours? The text colour would adapt to contrast with the background colour.

I'm not sure what you mean? This primitive helps with that -- ReplacementColor is the foreground and the Background is the background. Are you thinking we could auto-magically determine the correct foreground and contrasting background color?

@mdtauk
Copy link
Contributor

mdtauk commented Mar 23, 2021

I guess its more of a designer interpretation, rather than a technical one.

I was trying to see if there was an industry term for this operation but I couldn't find it. It reminds me of a DirectX fixed function operation, but I can't think of what it would be in Photoshop.

Photoshop has a Color Overlay effect that replaces the opaque pixels with a new colour.
image

Also, has there been any thought put to swapping background and foreground colours? The text colour would adapt to contrast with the background colour.

I'm not sure what you mean? This primitive helps with that -- ReplacementColor is the foreground and the Background is the background. Are you thinking we could auto-magically determine the correct foreground and contrasting background color?

I must have misunderstood your comment ''it turns all colors in the SourceElement into one single color''

So the replacement colour is for the foreground colours, like the text colour, or the fill colour of an element within this panel. And the panel's background colour renders below the elements being filtered?

@jevansaks
Copy link
Member

So the replacement colour is for the foreground colours, like the text colour, or the fill colour of an element within this panel. And the panel's background colour renders below the elements being filtered?

Exactly.


[MUX_PROPERTY_CHANGED_CALLBACK(TRUE)]
[MUX_PROPERTY_CHANGED_CALLBACK_METHODNAME("OnPropertyChanged")]
unsealed runtimeclass MonochromaticOverlayPresenter : Windows.UI.Xaml.Controls.FrameworkElement
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we currently inherit from Grid for this class, as we use the Background and CornerRadius properties that don't exist in FrameworkElement or Panel (Background exists but CornerRadius doesn't)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MikeHillberg Are there other types that would be better to derive from? I think Control wouldn't work because Control only has the properties, it doesn't do the drawing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation can derive from Grid, we just don't need/want the public API to have extra unnecessary members.

@MikeHillberg
Copy link
Contributor

About the "monochrome" name, we had the same difficulty naming BitmapIcon.ShowAsMonochrome. According to the dictionary it's technically accurate to be a color other than black.

@robloo
Copy link
Contributor

robloo commented Apr 2, 2021

MonochromaticOverlayPresenter is an interesting concept. I understand how this is needed to achieve the design goals and like the direction this is going. However, this seems like a bit of a hack. I think a correct design for the XAML framework would be far more complex. The generalized solution here should be the ability to draw the same control using two different styles -- and then change which area a style applies to resulting in a final composite of both styles.

Using a contrived example: Lets say you have a control that is styled using a light theme. However, you want a very specific geometric subsection (could be rectangular) to be rendered with the dark theme. Such a CompositeStylePresenter control would allow a specific geometry of the source control to actually appear to the user on screen using a different, arbitrary style. You could show a subset of a control with dark theme that is otherwise using light theme.

Such a generalized approach is not only more correct from a framework standpoint, it also ensures this is future proofed for further design direction. There are a lot of complexities with this idea though, and there would be some major restrictions that mean basically only color can be changed between the two styles (even changing a border thickness would cause misalignment and the inability to composite a usable final result).

@jevansaks
Copy link
Member

I think a correct design for the XAML framework would be far more complex.

Perhaps. We went this direction primarily because for WinUI2, changing the XAML framework for things that we want to work downlevel isn't possible (yet). So we resort to creative hacks such as this. :)

Once we have WinUI3 we can certainly discuss more holistic approaches. But even when we can, sometimes the creative targeted solution like this is still the best option because we have limited time to work on everything and a fully generalized solution isn't a great return on investment.

In this case, where else would such a generalized solution be applicable? And, more importantly, in what ways would that solution handle things that this solution doesn't?

@robloo
Copy link
Contributor

robloo commented Apr 3, 2021

We went this direction primarily because for WinUI2, changing the XAML framework for things that we want to work downlevel isn't possible (yet). So we resort to creative hacks such as this. :) ... Once we have WinUI3 we can certainly discuss more holistic approaches. But even when we can, sometimes the creative targeted solution like this is still the best option because we have limited time to work on everything and a fully generalized solution isn't a great return on investment.

Yea, that's certainly understood.

As always, I also understand your resource constraints but still push for what seems best for the future of the framework itself. Nothing is more permanent than a temporary solution. That said, I don't think there are many concerns here as the MonochromaticOverlayPresenter is just a control (as opposed to a lower-level framework feature) that could be obsolete in the future if more generalized solutions become available.

In this case, where else would such a generalized solution be applicable? And, more importantly, in what ways would that solution handle things that this solution doesn't?

Think of this more as you just got me thinking of some brand-new ideas. I don't recall this scenario being considered before like this so it's new territory. With new territory I think the generalized solution should always be investigated. I'll have to keep thinking about this over time and other more powerful use cases might become apparent.

Finally, the current MonochromaticOverlayPresenter concept seems to be getting into the old WPF BitmapEffect territory. I wonder if resurrecting those ideas (and naming/class conventions) would be beneficial here for the 'hack' as I call it :)

@jevansaks
Copy link
Member

With new territory I think the generalized solution should always be investigated. I'll have to keep thinking about this over time and other more powerful use cases might become apparent.

For sure. The feature that came up in our API spec discussion is that this is very similar to the VisualBrush feature in WPF. What I think would be more generalized is a control that exposes the ability to do a VisualSurface of anything and then apply any composition effect brush to it. That's something which we could easily build without new framework functionality and could enable some cool effects.

@robloo
Copy link
Contributor

robloo commented Apr 4, 2021

For sure. The feature that came up in our API spec discussion is that this is very similar to the VisualBrush feature in WPF.

I may have misunderstood how this was being implemented then. Additionally, VisualBrushes to me are used more for repeating patterns (It would have been nice to have these for the color picker checkered backgrounds). So other than perhaps similar API patterns I'm not sure how this is similar to a VisualBrush. However:

What I think would be more generalized is a control that exposes the ability to do a VisualSurface of anything and then apply any composition effect brush to it.

Conceptually I was thinking the same as what you describe here -- allowing any effect to be applied to the underlying image -- like filters in graphics software. I guess my assumption was the operations to swap out and modify the colors were being done on the rendered bitmap. Seems not? I don't know enough about WinUI's underlying composition system but this still sounding like 'BitmapEffect' from WPF for the most part. You know a lot more about how this is working behind the scenes so I could easily be wrong here.

Also note that a system to swap styles would still be more powerful and general purpose for this specific case (you could modify the background/foreground brushes as the same time in XAML and do it before things are rendered to a bitmap). However, this DatePicker design can also be achieved using a filter effect as you described.

@bkudiess
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bkudiess bkudiess merged commit bd80a3a into main May 19, 2022
@bkudiess bkudiess deleted the anawish/adding-datetimepicker-spec branch May 19, 2022 16:17
@ghost
Copy link

ghost commented Jun 2, 2022

🎉Microsoft.UI.Xaml v2.8.0-prerelease.220601001 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-DateTimePickers DatePicker, TimePicker, CalendarDatePicker, CalendarView SpecInReview team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants