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

Added a Calendar control #1244

Merged
merged 9 commits into from Nov 7, 2017

Conversation

Projects
None yet
4 participants
@sdoroff
Contributor

sdoroff commented Oct 24, 2017

The control is a direct port of the Silverlight Calendar control.
The original source is available here: https://github.com/MicrosoftArchive/SilverlightToolkit

@jkoritzinsky jkoritzinsky requested a review from kekekeks Oct 24, 2017

@kekekeks

LGTM, but change thread check to 1Dispatcher.UIThread.VerifyAccess`

private void EnsureValidThread()
{
if (Thread.CurrentThread != _dispatcherThread)

This comment has been minimized.

@kekekeks

kekekeks Oct 24, 2017

Member

Use Dispatcher.UIThread.VefiryAccess() instead.

internal static class CalendarExtensions
{
private static Dictionary<IAvaloniaObject, Dictionary<AvaloniaProperty, bool>> _suspendedHandlers = new Dictionary<IAvaloniaObject, Dictionary<AvaloniaProperty, bool>>();

This comment has been minimized.

@kekekeks

kekekeks Oct 24, 2017

Member

Why is it static?

@grokys

This is great! I think there's a few things that could be improved in porting it over from Silverlight though.

Unfortunately it's also showing that we're still quite slow when opening the Calendar page on ControlCatalog... This will be a good test for performance.

Margin="0,4,0,4"
HorizontalAlignment="Center"
VerticalAlignment="Center"
Text="{Binding}" />

This comment has been minimized.

@grokys

grokys Oct 24, 2017

Member

Interesting, this binding produces ~50 binding errors saying:

Binding: Error in binding to "Avalonia.Controls.TextBlock"."Text": "Null value in expression '' at ''."

in Visual Studio's output window when the Calendar page in the ControlCatalog is selected. Looks like I need to look into why this might be happening, may be a problem with our binding system or notifications.

This comment has been minimized.

@sdoroff

sdoroff Oct 24, 2017

Contributor

I have a feeling the binding error are happening because the controls generated by this template are being added to the visual tree before their DataContext is getting set. I'll figure it out.

This comment has been minimized.

@grokys

grokys Nov 7, 2017

Member

This seems to have gone away, did you work out what was causing it?

internal static class CalendarExtensions
{
private static Dictionary<IAvaloniaObject, Dictionary<AvaloniaProperty, bool>> _suspendedHandlers = new Dictionary<IAvaloniaObject, Dictionary<AvaloniaProperty, bool>>();

This comment has been minimized.

@grokys

grokys Oct 24, 2017

Member

I don't think that this (and the related extension methods in CalendarExtension) is needed. As far as I can tell, this was to get around the fact that Silverlight didn't have coercion on DependencyPropertys. WPF does, and Avalonia does (see here for example) so I think rather than checking the value is valid in the OnXPropertyChanged handler and setting it back if it's not, the relevant AvaloniaProperty should be given a Validate method. The suspension stuff can then be deleted from CalendarExtensions.

Does that make any sense?

This comment has been minimized.

@sdoroff

sdoroff Oct 24, 2017

Contributor

That makes sense and should be pretty easy to implement.

<Style Selector="Button.Previous">
<Setter Property="Template">
<ControlTemplate>
<Panel Cursor="Hand">

This comment has been minimized.

@grokys

grokys Oct 24, 2017

Member

You should probably set Background="Transparent" on this panel because at the moment hit-testing is only picking up that the cursor is over the button when it's right over the > icon which is kinda hard to hit.

</Style>
<Style Selector="Button.Previous">
<Setter Property="Template">

This comment has been minimized.

@grokys

grokys Oct 24, 2017

Member

It looks like styling the buttons using separate templates here comes from Silverlight, and I assume that's necessary because of their styling system, but in here it would be better to just set properties on the button rather than re-templating them I think. As they are, they're not re-templatable here like they are in Silverlight anyway.

<Setter Property="Focusable" Value="False"/>
<Setter Property="Template">
<ControlTemplate>
<Panel>

This comment has been minimized.

@grokys

grokys Oct 24, 2017

Member

Should probably set Background="Transparent" on this panel because the days are a bit hard to hit with the mouse currently. It looks like in Silverlight the Opacity is set on pointer enter/leave which would have the same effect, but here you're setting Rectangle#Background's visibility instead.

sdoroff added some commits Oct 30, 2017

Correctly verified access from UI thread
Used Dispatcher.UIThread.VerifyAccess() instead of a
custom implementation
Removed dependency on IsHandlerSuspended extensions
Replaced the IsHandlerSuspended and SetValueNoCallback extension
methods with a Validate function for the CalendarMode property and
a _displayDateIsChanging flag for the various DisplayDate properties
Expanded HitTest regions of internal elements
Set Backgrounds to Transparent for various buttons in the control
templates to allow for expanded HitTest regions allowing for better
PointerOver and Clicking behavior
Fixed binding errors caused by DayTitleTemplate
Fixed by initializing the DataContext before adding elements to the
VisualTree
@sdoroff

This comment has been minimized.

Contributor

sdoroff commented Oct 30, 2017

I pushed each fix out as separate commit. For the future, is that the preferred method or would a single commit be better?

Updated Theme template to avoid retemplating buttons
Removed the re-templating of the navigation buttons within the Calendar
header
@sdoroff

This comment has been minimized.

Contributor

sdoroff commented Oct 30, 2017

I'm not sure why a test (MemberSelectorTests.Should_Select_Child_Property_Value) failed during the AppVeyor build. The commits don't change anything that would affect that test.

@jkoritzinsky

This comment has been minimized.

Member

jkoritzinsky commented Oct 31, 2017

That test is one of the ones we have that has spurious failures in CI. I've restarted the build for you.

@grokys

grokys approved these changes Nov 7, 2017

Just taking another look at the reason for the slowness here, and saw that it's creating 357 layers! Our deferred renderer currently creates a new layer for each control that has an opacity not set to 1, and a lot of controls here have that! I really need to fix this.

Other than that, this looks great! Thanks for doing this. I'm going to merge it in even with the performance problems it has and work on fixing those problems.

@grokys grokys merged commit 37d3fe7 into AvaloniaUI:master Nov 7, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sdoroff sdoroff deleted the sdoroff:calendar-control branch Jan 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment