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

Add datetime and time pickers #2715

Merged
merged 9 commits into from
Jun 30, 2021
Merged

Conversation

vidartf
Copy link
Member

@vidartf vidartf commented Jan 10, 2020

Resolves #2548

@vidartf
Copy link
Member Author

vidartf commented Jan 20, 2020

Ready for review! :)

@pbugnion
Copy link
Member

pbugnion commented Jan 26, 2020

This looks great! I've played around with the interface. I've not looked at the code at all.

  • In both the TimePicker and the DatetimePicker, the type of value can be None. It might be worth clarifying that in the docstring (right now, it suggests that these are always a valid datetime.time or datetime.datetime instance).
  • In the TimePicker, the default value of step is 60 (at least on Chrome?). When you set "any", it defaults to 60? Is it worth specifying this? Also, you're allowed decimal values, which is not obvious. I think both these behaviours are sensible, but it may be worth clarifying this in the docstring description of the step parameter?
  • In the TimePicker, the max value is inclusive, which is maybe counter-intuitive. Is it worth clarifying this in the docstring?
  • In both docstrings, the examples still refer to ipydatetime.

In the DatetimePicker, there's some strange behaviour around zoned date times and offset-native datetimes. As far as I can tell (I haven't looked at the code), setting the value by playing around with the UI control sets a time-zone aware datetime (at least on Chrome). However, I can then set the value manually via an assignment to value, with an offset-native datetime.

The most straightforward way to exploit this discrepancy to cause an exception is:

  1. Create and display a DatetimePicker
import ipywidgets as widgets
import datetime
dt = widgets.DatetimePicker()
dt
  1. Set the control manually using the picker. dt.value is now offset-aware:
dt.value.tzname()  # => 'GMT' for me
  1. Assign an offset-native datetime to min or max:
dt.max = datetime.datetime(2018, 1, 1)

gives:

~/oss/ipywidgets/ipywidgets/widgets/widget_datetime.py in _validate_max(self, proposal)
     82         if self.min and max < self.min:
     83             raise TraitError("setting max < min")
---> 84         if self.value and max < self.value:
     85             self.value = max
     86         return max

TypeError: can't compare offset-naive and offset-aware datetimes

It feels like the only sensible way to deal with this is to either only allow zoned datetimes or offset-native datetimes, but not a mix. From your entry in the documentation, it looks like you expect the user to select a zoned datetime, not an offset-native one. Maybe we could introduce a new traitlet, or allow customization of the current one?

@vidartf
Copy link
Member Author

vidartf commented Jan 29, 2020

It feels like the only sensible way to deal with this is to either only allow zoned datetimes or offset-native datetimes, but not a mix.

Seems reasonable. I've also added a naive picker in ipydatetime, so I might just add validators to both of them to enforce it (should it coerce silently, or fail noisily?).

@vidartf
Copy link
Member Author

vidartf commented Feb 4, 2020

@pbugnion I've added a separate picker for naive datetimes (NaiveDatetimePicker), and made DatetimePicker strictly timezone aware.

@vidartf
Copy link
Member Author

vidartf commented Feb 18, 2020

Now green ✔️

@jasongrout
Copy link
Member

From ipywidgets meeting today: we should have further discussion about serialization formats. CC @SylvainCorlay and @vidartf

@epifanio
Copy link
Contributor

Hi, I was looking for a DatetimePicker in IPywidgets and I came across this :) what is the status? will it be implemented?

@vidartf
Copy link
Member Author

vidartf commented Nov 20, 2020

It is currently available in the package ipydatetime. The hope/plan is to get this into core ipywidgets (since it is a "standard" browser control), but I need to update the PR with the latest changes from that package.

@davidbrochart
Copy link
Member

@vidartf I rebased this PR on master and opened a PR in your repo: vidartf#1

@vidartf
Copy link
Member Author

vidartf commented Feb 15, 2021

This should be ready now. @SylvainCorlay I didn't touch datetime serialization format. Let's discuss any changes in a followup issue/PR. For context: we were discussing using a different format for serialization, either Unix timestamp or Isoformat string (full version or limited to Python stdlib supported version).

@vidartf
Copy link
Member Author

vidartf commented Mar 25, 2021

Don't merge until I've fixed the schemas becoming binary ⚠️

@jasongrout jasongrout marked this pull request as draft March 25, 2021 15:26
@jasongrout
Copy link
Member

Don't merge until I've fixed the schemas becoming binary ⚠️

Marked as draft for now, then.

@vidartf vidartf marked this pull request as ready for review March 25, 2021 15:31
@vidartf
Copy link
Member Author

vidartf commented Mar 25, 2021

Fixed 👍

@jasongrout
Copy link
Member

@jtpio - is this good to merge?

@jtpio
Copy link
Member

jtpio commented Jun 10, 2021

If anyone wants to try this branch on Binder:

https://mybinder.org/v2/gh/vidartf/ipywidgets/datetime?urlpath=lab/tree/docs/source/examples/

image

* Called when the model is changed. The model may have been
* changed by another view or by a state update from the back-end.
*/
update2(model?: Backbone.Model, options?: any): void {
Copy link
Member

Choose a reason for hiding this comment

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

Should we use a more descriptive name than update2 for this method?

#!/usr/bin/env python
# coding: utf-8

# Copyright (c) Vidar Tonaas Fauske.
Copy link
Member

Choose a reason for hiding this comment

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

Looks like a couple of these headers here and below could be updated to match the existing ones.

@jtpio
Copy link
Member

jtpio commented Jun 14, 2021

@vidartf mind rebasing on the latest master to pick up the update to TypeScript 4.3 to check everything compiles correctly?

@vidartf
Copy link
Member Author

vidartf commented Jun 22, 2021

@jtpio Rebased, and all green ✔️

@jasongrout
Copy link
Member

@jtpio, is this good to go in?

@jtpio
Copy link
Member

jtpio commented Jun 30, 2021

I think so yes. In the dev meeting last week we had decided to wait until a couple of dependency update PRs were merged and a new alpha release cut. So it should be good to go now.

Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Thanks!

@jtpio jtpio merged commit bf56eb7 into jupyter-widgets:master Jun 30, 2021
@vidartf vidartf deleted the datetime branch July 5, 2021 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Time and Datetime picker widgets
6 participants