-
-
Notifications
You must be signed in to change notification settings - Fork 60
Date picker #244
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
base: main
Are you sure you want to change the base?
Date picker #244
Conversation
|
The thought just occurred to me to observe NSSystemTimeZoneDidChange to properly redraw views that use |
|
There’s a backend method called setEnvironmentChangeHandler or something like that. That’s where we currently observe system theme changes and would be where timezone observation should live as well. |
|
Right, I'll go see if I can get that to work in UIKitBackend and AppKitBackend first, and then maybe the other backends. I don't think it'll necessarily work on Linux though |
|
Marking this as draft because UIDatePicker (at least on iOS 17) completely misbehaves if the user changes the time zone in settings |
stackotter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this super detailed PR! Safari was lagging the entire time that I was reviewing your changes lol. I've left a bunch of requests and comments from an initial read through of all of the code.
I haven't had time to test out the changes myself yet, but I'll make sure to do that before merging and will update the PR with any additional feedback I have after testing things out.
|
|
||
| var body: some Scene { | ||
| WindowGroup("Date Picker") { | ||
| VStack { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add #hotReloadable { ... } around this VStack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
| allStyles = [.automatic] | ||
|
|
||
| if #available(iOS 14, macCatalyst 14, *) { | ||
| allStyles.append(.graphical) | ||
| } | ||
|
|
||
| #if !canImport(GtkBackend) | ||
| if #available(iOS 13.4, macCatalyst 13.4, *) { | ||
| allStyles.append(.compact) | ||
| #if os(iOS) || os(visionOS) || canImport(WinUIBackend) | ||
| allStyles.append(.wheel) | ||
| #endif | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the differences in platform support for each style are kinda complicated, it could be a good idea to add a supportedDataPickerStyles property to the environment so that devs can implement runtime compatibility checks a bit more easily than this. The environment variable should be a get-only property with a getter that references some internal property that SwiftCrossUI can write to. Then when creating the default environment values, SwiftCrossUI should query the backend for its list of supported date picker styles (via a new supportedDatePickerStyles property of some sort).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me, will implement
| ) | ||
| .datePickerStyle(style ?? .automatic) | ||
|
|
||
| Button("Reset date") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Reset date to now"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, this button was a last-minute addition when I got lost using calendars and time zones
| public var timeZone: TimeZone | ||
|
|
||
| #if !os(tvOS) | ||
| public var datePickerStyle: DatePickerStyle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document this property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added documentation
| // choice for the current calendar means the cursor position is reset after every keystroke. I | ||
| // know of no simple way to tell whether NSDatePicker requires or forbids eras for a given | ||
| // calendar, so in lieu of that I have hardcoded the calendar identifiers. | ||
| private let calendarsWithEras: Set<Calendar.Identifier> = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to this thread, the Gregorian calendar also has eras: https://www.reddit.com/r/iOSProgramming/comments/7brq81/what_is_the_importance_of_the_calendar_component/
It could make more sense to name this property to calendarsRequiringEra or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's why I don't just check the length of the eras property on the calendar object. Gregorian has two (AD/BC or CE/BCE depending on your preference), and some of the ones that require it only have one. The suggested rename makes sense to me.
| /// - label: The view to be shown next to the date input. | ||
| public nonisolated init( | ||
| selection: Binding<Date>, | ||
| range: ClosedRange<Date> = Date.distantPast...Date.distantFuture, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Label this parameter in to match SwiftUI. (same with the alternative init below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to say anything but I already did this
| #if os(tvOS) | ||
| preconditionFailure() | ||
| #else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With my comment about making updateDatePicker available on tvOS, I think this conditional compilation should be made redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, already done
| var environment = defaultEnvironment | ||
|
|
||
| environment.toggleStyle = .switch | ||
| environment.timeZone = .current |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like AppKitBackend would also prefer that timeZone is set to .current. Should AppKitBackend be doing that? Or maybe should EnvironmentValues be doing that? I can see reasons for and against;
- If the default was current, then backends that don't implement timezone change observation (such as GtkBackend) would get out of sync
- If the default was autoupdatingCurrent, then whenever you accessed the environment property you would technically get the current timezone, but it wouldn't capture the current timezone in the way that you'd expect cause the value's meaning could change out from under you.
Now that I think about it, having it be autoupdatingCurrent could cause issues further down the line when we do more surgical propagation of environment changes. From SwiftCrossUI's perspective, the old timezone and the new one would always seem to be identical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The entire extent of my logic was that I booted up a Swift REPL and ran
import SwiftUI
EnvironmentValues().timeZone == .autoupdatingCurrentAnd seeing that that's true (and knowing .autoupdatingCurrent != .current), I put that as the default in SwiftCrossUI.
|
|
||
| customDatePicker.toggleTimeView(shown: components.contains(.hourAndMinute)) | ||
|
|
||
| if environment.timeZone != .autoupdatingCurrent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would have to be updated to current if the environment timezone default value gets changed to .current (as discussed in one of my comments on UIKitBackend.
| #if compiler(>=6.2) | ||
| case .vietnamese: "VietnameseLunarCalendar" | ||
| #endif | ||
| case let id: fatalError("Unsupported calendar identifier \(id)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this fatalError would be relatively easy to come across when trying to make an app support many locales and calendars. It'd be annoying to find out by getting a bug report from someone softlocked out of using the app because of their locale, so I think it'd be best to print a warning here and fallback on gregorian. That way people can at least use the app even if they try to intentionally (or unintentionally via locale) use an unsupported calendar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense to me.
Add DatePicker for AppKitBackend Add DatePickerExample to macOS CI Fix DatePicker update logic for AppKitBackend Update argument name to match SwiftUI Add more availability annotations Shut up tvOS let me see if the iOS CI will pass please work. Fine, here's your view Initial WinUI implementation Reformat WinUI code Implement minYear/maxYear for DatePicker Improve WinUI sizing code Fix CalendarDatePicker size Minor cleanup Generate GTK classes and improve manual type conversion oops Fix casing of calendar name Saving partial work on GtkBackend More partial work Use Gtk.Calendar Add missing parts to GtkBackend.updateDatePicker Add DatePickerExample to Linux CI Fix one Mac availability error Add availability annotation on unused widget Add time zone listener for UIKitBackend Add listener for AppKitBackend
Update some doc comments
Fixes #233
Each backend has its own shortcomings:
hourMinuteAndSecond, and doesn't respect font colorwheelpicker stylehourMinuteAndSecond, doesn't respect font color, and doesn't respectenvironment.timeZone(instead always using the current time zone)graphicalpicker style, doesn't respectenvironment.timeZone(instead always using UTC), and only supports picking the date, not the time