pglet / pglet-python

Python client for Pglet - build real-time interactive web apps in Python
MIT License
18 stars 7 forks source link

DatePicker does not accept date object as value #42

Open mikaelho opened 2 years ago

mikaelho commented 2 years ago

DatePicker._set_attronly handles datetime objects, should also expect date objects (check whether object is instance of date, as datetime is a subclass of date).

Also, using datetime value for a DatePicker is confusing as browsers will helpfully look at user's timezone and store the UTC value here (which might actually have a different date than the one picked).

FeodorFitsner commented 2 years ago

Could you please give me an example of how do you use it with date object?

mikaelho commented 2 years ago

Here's a repl to play with, code copied below as the repl might not be there forever.

import datetime
from types import MethodType

import pglet
from pglet import DatePicker, Stack

def main(page):
    page.theme = 'light'

    datetime_value = datetime.datetime(2021, 12, 17, 1, 1, 1)
    date_value = datetime.date(2021, 12, 17)

    picker = DatePicker(on_change=print_value)
    patched_picker = DatePicker(on_change=print_value)
    setattr(patched_picker, '_set_attr', MethodType(_patched_set_attr, patched_picker))

    page.add(Stack(
        horizontal=True,
        controls=[picker, patched_picker]
    ))

    # datetime works
    picker.value = datetime_value
    page.update()

    # date does not - funny, really, since it is called DatePicker
    try:
        picker.value = date_value
    except TypeError as error:
        assert str(error) == "an integer is required (got type str)", str(error)

    # Both date and datetime work with patched picker
    patched_picker.value = date_value
    page.update()

    assert patched_picker.value.isoformat() == "2021-12-17"

    patched_picker.value = datetime_value
    page.update()

    value = patched_picker.value.isoformat()
    assert value == "2021-12-17T01:01:01"

# Both patched and non-patched versions have this behaviour in my +2 timezone
def print_value(event):
    print(event.control.value)  # Choosing Dec 14, I get 2021-12-13T22:00:00.000Z
    print(event.control.value.date())  # And the wrong date, 2021-12-13

def _patched_set_attr(self, name, value, dirty=True):
        d = value
        if d == "":
            d = None
        elif name == "value" and d != None and not isinstance(d, datetime.date):
            d = datetime.datetime.fromisoformat(value.replace('Z', '+00:00'))
        self._set_attr_internal(name, d, dirty)

pglet.app("index", target=main, local=True)
FeodorFitsner commented 2 years ago

Do I understand it correctly that the final patched _set_attr should have condition checking both datetime and date?

    def _set_attr(self, name, value, dirty=True):
        d = value
        if d == "":
            d = None
        elif name == "value" and d != None and not isinstance(d, datetime) and not isinstance(d, date):
            d = datetime.fromisoformat(value.replace('Z', '+00:00'))
        self._set_attr_internal(name, d, dirty)
FeodorFitsner commented 2 years ago

Regarding the returned date - DatePicker returns selected date in UTC (hence +00:00). Try converting the value to your local datetime: https://stackoverflow.com/questions/4770297/convert-utc-datetime-string-to-local-datetime

mikaelho commented 2 years ago

You do not have to check for both because datetime.datetime is a subclass of datetime.date, the single isinstance captures both.

Converting the value to local datetime does not work, because the app does not know which timezone the user is in. Overall, dealing with timezones is a bit confusing - it is a DatePicker, and dates should not have timezones.

I looked at the Fluent UI docs, which talk about setting a strings prop to e.g. defaultDatePickerStrings, but if I understand correctly, that is only about allowing different formats on date entry.

I am really impressed with pglet and the Python client for it, but some of the details of these Fluent controls seem a bit "last millennia", given how HTML5 date, datetime and time controls have for years used the browser/UI locale settings, both for presenting and parsing the values.

FeodorFitsner commented 2 years ago

OK, I'm not giving it up on it yet... :)

DatePicker itself returns a local date with correct, local time zone. I've add console.log in onSelectedDate event and see this:

image

which does look correct.

Next, to return the value to Pglet server the date is converted to ISO format which is UTC.

When Pglet client (Python program), Pglet Server and user's browser are all running on the same machine (e.g. "local" mode) working with zone-specific dates is easy and makes sense. However, imagine Pglet Server, the app and the browser are on different computers in different time zones - operating all dates in UTC is a must in this case.

However, we could build date conversion from UTC-to-local and vice versa into Python client, to simplify the things for the end users. This way we assume that all dates in Pglet program are in locale of computer where this program is running.

mikaelho commented 2 years ago

Thanks for sticking with me. 👍

Maybe I am missing something fundamental here, but in your example above, you have selected the date Dec 18, 2021. Why are we not just getting '2021-12-08' from the control? "Dates in UTC" does not make sense, as dates do not have timezones.

I do not know the architecture of the pglet enough so that I could understand how much code and control you have at the user's browser. Could you convert the datetime value to string, using the browser's locale in order to not accidentally change the date, then grab the date (as string if you have to) and drop the time? Then server's and Python code's locations would not matter.

FeodorFitsner commented 2 years ago

OK, let's say I'm sharing with your Pglet program which is running on my computer. I'm in GMT-8 time zone and you are in GMT+2, so it's 10 hours ahead of me. So, at some point if you choose in DatePicker your "today" and we just convert it to a local date string, take date part, e.g. 2012-12-18, strip the rest and pass to the server (me) this could look like "tomorrow" on the server side. I believe it's a common problem of client-server apps where clients could be in any time zone. Additionally, if the app uses a database all dates should be stored in UTC too, so clients in multiple zones get the same results.

FeodorFitsner commented 2 years ago

...yep, dates in Python are tricky. datetime object could have or could not have time zone information attached to it. The current implementation datetime.fromisoformat(value.replace('Z', '+00:00')) sets time zone to UTC (printing the value gives datetime.datetime(2021, 12, 18, 8, 0, tzinfo=datetime.timezone.utc)), so the rest of app logic could use that.

I think Pglet client should not convert the selected date to a local date on user's behalf and leave it to a user. Anyway, in the most cases UTC dates are used to query database or work with 3rd-party APIs.

To summarize, to pre-select a current date in DatePicker do picker.value = datetime.datetime.utcnow() and when you read DatePicker's selected date it is UTC which could be converted to a local date with the following:

def datetime_from_utc_to_local(utc_datetime):
    now_timestamp = time.time()
    offset = datetime.fromtimestamp(now_timestamp) - datetime.utcfromtimestamp(now_timestamp)
    return utc_datetime + offset

I'm going to update documentation with the note that dates are UTC.

mikaelho commented 2 years ago

Documentation update +1.

Fundamentally, my "issue" with the Fluent UI DatePicker component has nothing to do with Python dates, but the fact that it is an incoherent combination of date and datetime pickers. It looks like the Fluent UI developers have decided to represent dates with midnight UTC times, which of course is not really the same thing as a date.

This is relatively ok as long as the developer knows to expect the date in this format. But it all breaks down if you "allow_text_input" which seems to allow entering an exact time, after which the app no longer has any real clue whether the user intended to provide a date or a datetime, and how the value should be treated. Maybe some heuristic that if the time happens to be midnight UTC, then it should be treated as a date, otherwise as a datetime?

So I would maybe also have some type of note in the docs that if you allow the text input, your results may vary. Otherwise, people with web development background will assume that the control is based on and will work like the input type date.

mikaelho commented 2 years ago

I took another look at this and I think the key point would be to get an aware datetime including the user's timezone from the browser to the code, as sending the time in UTC loses information which is needed for the code to handle e.g. the date coherently.

I spent a little time surprised that Javascript does not natively support formatting datetimes with timezone information. I am assuming we would rather not add a major dependency like moment.js for this, so here is a plain function that could be used:

function toIsoString(date) {
    const tzo = -date.getTimezoneOffset(), sign = tzo >= 0 ? '+' : '-';
    const year = date.getFullYear();
    const month = (date.getMonth()+1).toString().padStart(2, '0');
    const day = date.getDate().toString().padStart(2, '0');
    const hours = date.getHours().toString().padStart(2, '0');
    const minutes = date.getMinutes().toString().padStart(2, '0');
    const seconds = date.getSeconds().toString().padStart(2, '0');
    const milliseconds = date.getMilliseconds().toString().padStart(3, '0');
    const tzoHours = (Math.floor(Math.abs(tzo) / 60)).toString().padStart(2, '0');
    const tzoMinutes = (Math.abs(tzo) % 60).toString().padStart(2, '0');

    return `${year}-${month}-${day}T${hours}:${minutes}:${seconds}.${milliseconds}${sign}${tzoHours}:${tzoMinutes}`
}