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

feat: add month calendar #3667

Draft
wants to merge 95 commits into
base: main
Choose a base branch
from

Conversation

TomJGooding
Copy link
Contributor

@TomJGooding TomJGooding commented Nov 12, 2023

Description

Add a MonthCalendar widget, intended to form part of a DatePicker widget.

Note

Currently very much a work in progress wanting feedback from Textual maintainers and users!

Related Issue

Checklist:

  • Docstrings on all new or modified functions / classes
  • Updated documentation
  • Updated CHANGELOG.md (where appropriate)

Copy link
Member

@darrenburns darrenburns left a comment

Choose a reason for hiding this comment

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

Early review with some comments and general out loud thinking. Good progress!

src/textual/widgets/_month_calendar.py Outdated Show resolved Hide resolved
src/textual/widgets/_month_calendar.py Outdated Show resolved Hide resolved
src/textual/widgets/_month_calendar.py Show resolved Hide resolved
from textual.widgets import MonthCalendar


class MonthCalendarApp(App):
Copy link
Member

Choose a reason for hiding this comment

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

Adding a CSS variable containing the following CSS to this app yields better results. I reckon we should add the equivalent CSS to the widget itself to override the default DataTable behaviour (unless you can think of a reason not to).

    MonthCalendar { height: auto; width: auto; }
    MonthCalendar > DataTable { height: auto; width: auto; }

We don't do this in the DataTable itself because it's intended to scroll, but this widget has a very constrained height compared to the DataTable which can be arbitrarily large.

Copy link
Contributor Author

@TomJGooding TomJGooding Nov 14, 2023

Choose a reason for hiding this comment

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

Sorry I should have mentioned, I haven't started looking at the DEFAULT_CSS yet as I wanted to focus on the core functionality first. Do you think the table/columns widths should just be static, or allow the developer to specify the column width (or perhaps even grow dynamically according to the widget width)?

Copy link
Member

Choose a reason for hiding this comment

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

I haven't started looking at the DEFAULT_CSS yet as I wanted to focus on the core functionality first.

Seems sensible.

Do you think the table/columns widths should just be static, or allow the developer to specify the column width (or perhaps even grow dynamically according to the widget width)?

I think static for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've now added some basic DEFAULT_CSS in cfcac14. I've flagged min-width as TODO as this might depend on locale?

class MonthCalendar(Widget):
year: Reactive[int | None] = Reactive[int | None](None)
month: Reactive[int | None] = Reactive[int | None](None)
first_weekday: Reactive[int] = Reactive(0)
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering about using Literal["monday", "tuesday", ...] here to remove any second-guessing about how days are indexed. Will leave it as an open thought here if you or anyone else wants to chip in.

Copy link
Contributor Author

@TomJGooding TomJGooding Nov 23, 2023

Choose a reason for hiding this comment

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

Perhaps this could be an IntEnum similar to the calendar module in 3.12?

self.first_weekday = first_weekday

def compose(self) -> ComposeResult:
yield DataTable()
Copy link
Member

Choose a reason for hiding this comment

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

Open thoughts: I wonder if we should display the month name above the DataTable. I wonder if it should be something that can be toggled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm assuming this would simply display "November 2023" for example, and then if developers wanted something different they would be expected to create a custom widget and set MonthCalendar.month_header to False?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure - I guess the month name would be defined by locale too? @willmcgugan any thoughts?

@TomJGooding
Copy link
Contributor Author

TomJGooding commented Nov 14, 2023

I'm struggling with handling the reactives for this widget, due to a class of problem explained this issue:

I'm afraid I'm blocked on this, unless there's a workaround that would include a cursor reactive. My current call_after_refresh is really a hacky workaround just so tests pass locally (though obviously not in CI!)

@darrenburns
Copy link
Member

Hi @TomJGooding, could you try adding a boolean @property called is_mounted to Widget which checks that self.mounted_event.is_set(), and then inside your watchers, only do things relating to the DOM (querying, updating, etc.) if the DOM is ready? Would that resolve the issue?

Let me know if I've misunderstood the issue!

@TomJGooding
Copy link
Contributor Author

Thanks Darren for your help, it looks like this works:

    def watch_month(self) -> None:
        if not self.is_mounted:
            return
        self._update_calendar_days()

Would it be better to add this new Widget.is_mounted property in a separate PR (linked to the issue #3011 mentioned above)?

@darrenburns
Copy link
Member

No problem! I think separate PR would be great for that.

@TomJGooding
Copy link
Contributor Author

TomJGooding commented Nov 29, 2023

My latest commit (334e771) will update the calendar cursor to the same highlighted day when the month changes.

Here's a quick demo recording, notice this accounts for when the new month doesn't have 31 days:

month-calendar-2023-11-29

This requires the python-dateutil package using relativedelta to add/subtract months. I was able to import this, but I'm not sure if only because it is required by one of the dev dependencies?

The problem with updating the cursor when the month changes is I'm not sure how to handle when the highlighted date is outside the month. For example in the demo recording, on the initial month the cursor could be highlighting the first Monday (31st). Where should the cursor highlight when the month changes? If changed to just the previous month, it probably should just highlight the same date, but what if the updated calendar is months or years different?

Perhaps if you don't mind (I appreciate how busy you are), this would a good time for a pre-review review? Tagging @darrenburns and @davep but any feedback would be welcome! Thanks again for your help so far.

(Also just to note, the failing test is a keyline snapshot mismatch and not related to this PR!)

@willmcgugan
Copy link
Collaborator

I don't follow the component class thing. Can somebody summarize?

@darrenburns
Copy link
Member

darrenburns commented Aug 12, 2024

To style this e.g. the cursor of this widget, since it inherits from DataTable, developers need to target component classes named after the DataTable, which isn't very nice. Tom has added a workaround.

@TomJGooding
Copy link
Contributor Author

Thanks @willmcgugan for your feedback. I've now removed the dateutil.relativedelta import which actually wasn't as difficult as I feared!

Hopefully the component classes issue makes sense, but just to clarify I haven't managed to find any workaround. I'm happy to look at adding some mechanism for this, but I'm afraid I would need a little guidance on where to start.

@darrenburns
Copy link
Member

Just to give an example of the issue:

class CalendarGrid(DataTable, inherit_bindings=False):
    # TODO: Ideally we want to hide that there's a DataTable underneath the
    # `MonthCalendar` widget. Is there any mechanism so component classes could be
    # defined in the parent and substitute the component styles in the child?
    # For example, allow styling the header using `.month-calendar--header`
    # rather than `.datatable--header`?
    DEFAULT_CSS = """
    CalendarGrid {
        height: auto;
        width: auto;
        .datatable--header {
            background: $surface;
        }
    }
    """

We need to use the .datatable--header component class selector, which is a bit ugly!

@willmcgugan
Copy link
Collaborator

We need to use the .datatable--header component class selector, which is a bit ugly!

It is a bit of a wart. We discussed it in the office and didn't come up with a conclusion. Suggest we go with this for now.

Copy link
Collaborator

@willmcgugan willmcgugan left a comment

Choose a reason for hiding this comment

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

Hi Tom, sorry for the delay again. Getting caught up.

I know this is marked as draft, just wanted to add a few comments to avoid blocking while waiting for my input.


from rich.text import Text

from textual import on
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you change these to relative imports, which is the convention we use elsewhere.

I think we are going to switch to absolute imports eventually, but best to be consistent for now...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reverted back to absolute imports following #4997!

"month-calendar--outside-month",
}

date: Reactive[datetime.date] = Reactive(datetime.date.today())
Copy link
Collaborator

Choose a reason for hiding this comment

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

The today() is evaluated at import time. We probably want it to be evaluated when the widget is instantiated.

Easy fix: reactives take a callable. So we can just drop the ().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 172fbbe.

show_cursor: Reactive[bool] = Reactive(True)
show_other_months: Reactive[bool] = Reactive(True)

class DateHighlighted(Message):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Docstring pls

def control(self) -> MonthCalendar:
return self.month_calendar

class DateSelected(Message):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Docstring pls

event.stop()
pass

def previous_month(self) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Docstrings on all methods pls. With the exception of inherited methods / handlers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry just to confirm, do all watch and action methods need docstings? Also should these methods be private (i.e. include a leading underscore)? I did have a look at other widgets for guidance but there seems to be a mix.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actions, yes. Just a note to say what they do. Watch methods, the docstrings seems rendundant, so we often skip those unless you want to document for future devs why they are needed.

With regards to marking them private, if there is no earthly reason the user would want to override those, then its best to make them private...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I've added those docstrings but wasn't sure if they should be private so left as-is for now.

@TomJGooding
Copy link
Contributor Author

This PR is approaching its one year anniversary, which is perhaps fitting for a calendar widget! I think I under-estimated some of the intricacies of implementing this, and that's before starting work on the DatePicker! I appreciate there's been a lot happening in Textual during this time, so thanks again for all your help!

Hopefully there is still appetite for this! Since this PR has been open for a while, I wanted to ensure I was keeping track of any outstanding features/issues. I've started a list below, I'd really appreciate some feedback when you have some time.

  • Improve the hacky handling of clicking empty cells - fixed in 6bf3b0a
  • Minimum width, though this may depend on locale? (See #3667 (comment))
  • Change the first_weekday type for clarity? (See #3667 (comment), currently 0-6 based on Python's calendar)
  • Add optional header with the month/year? (See #3667 (comment))
  • Hide the underlying DataTable with component class styling? (See #3667 (comment))
  • Add feature to allow custom formatting of dates? (See #3576 (comment))

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.

3 participants