search

xamarin / Xamarin.PropertyEditing

MIT License
24 stars 16 forks source link

Refactor to have individual Date and Time Controls for better mapping… #756

Closed CartBlanche closed 4 years ago

CartBlanche commented 4 years ago

… for .Forms

This PR gets things working in the respective Stand-Alones. Separate PR will be needed for uitools support.

Windows

Screenshot 2020-09-29 at 16 25 49 Screenshot 2020-09-29 at 16 33 29

MacOS

Screenshot 2020-09-29 at 16 28 40 Screenshot 2020-09-29 at 16 28 58
garuma commented 4 years ago

Nice, definitely going in the right direction

netonjm commented 4 years ago

Absolutely agree @CartBlanche 👍

CartBlanche commented 4 years ago

@netonjm Thanks for the try...catch suggestions, my only concern is our other editors don't make extensive use of them, so feel if we do that we should be consistent, possibly in a separate specific PR, and add them across the board. Also keep in mind that adding try...catch has extra performance overheads. So we may need to profile that and compare with and without.

netonjm commented 4 years ago

@netonjm Thanks for the try...catch suggestions, my only concern is our other editors don't make extensive use of them, so feel if we do that we should be consistent, possibly in a separate specific PR, and add them across the board. Also keep in mind that adding try...catch has extra performance overheads. So we may need to profile that and compare with and without.

Any possibility of catching an exception that could occur over runtime (as is the case in case of set a DateTime) should be ahead of the little memory savings that may cause.

CartBlanche commented 4 years ago

Any possibility of catching an exception that could occur over runtime (as is the case in case of set a DateTime) should be ahead of the little memory savings that may cause.

When I mention performance, I wasn't thinking about memory, but more speed. The setting DateTime incorrectly scenario should be handled. The current Xamarin.Forms DatePicker element has already been using DateTime for over year, maybe 2 by now. I've not seen any DevDiv issue relating to it. Have you?

CartBlanche commented 4 years ago

@mcumming the ToDateTime extension method is here - https://github.com/xamarin/Xamarin.PropertyEditing/blob/main/Xamarin.PropertyEditing.Mac/Controls/DateExtensions.cs#L33-L41