Skip to content

[DateTimeWidget] add key_events and fix setting dates#13214

Merged
Frenzie merged 15 commits into
koreader:masterfrom
Commodore64user:datetime
Feb 27, 2025
Merged

[DateTimeWidget] add key_events and fix setting dates#13214
Frenzie merged 15 commits into
koreader:masterfrom
Commodore64user:datetime

Conversation

@Commodore64user

@Commodore64user Commodore64user commented Feb 5, 2025

Copy link
Copy Markdown
Member

what's new

  • Added a callback to the year picker to handle leap year changes, ensuring February 29th is adjusted to February 28th if the new year is not a leap year.
  • Added a callback to the month picker to adjust the day value if it exceeds the number of days in the selected month.
  • Updated the day picker to reference the selected month and year, ensuring the correct number of days is displayed.
  • Updated the info_text for setting the date to clarify the date format.
  • Refactored the key event registration logic by moving it to a new method registerKeyEvents to handle different configurations based on the number of pickers.
  • Added a new method onDateTimeButtonPressed to process value changes for the widget components based on the direction of the spin. This method ensures that the correct widget is modified and updates the maximum days in a month dynamically.
  • Updated the createLayout method to include date_month and date_year properties for the day widget to facilitate dynamic date validation.

screenshots

this madness won't occur again ;), we can no longer select any fake dates.

before / after

depends on #13208


This change is Reviewable

Comment thread frontend/ui/widget/datetimewidget.lua Outdated
extra_callback = nil,
}

local IMPOSSIBLE_DATES = {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't believe they're impossible. That is, 31 April is automatically resolved to 1 May, and so forth.

@Commodore64user Commodore64user Feb 5, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

"impossible to happen in real life", nervous laughter... I know it's resolved, however, I believe that is not the ideal way of dealing with it. If I end up accidentally choosing the wrong date, and since the computer will just accept anything, I might end up with a completely wrong date. We should not expect everyone to double check the message on-screen afterwards. Example, I might choose to set the day first (31st) then want to select July but by accident choose June instead, now my date was set to 2025-07-01 (we're off by a whole month)... I didn't pay attention to the screen or pressed somewhere and that went away quickly, yada yada yada, all my dates are now wrong until sometime in future when I notice it and curse the heavens.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Have no idea at this particular codepath, so maybe my suggestion isn't as easy to implement.

When I'm on the 30 of April I would expect that pressing the up arrow on the day would set the date to 01 May.
When I'm on the 31 of May I would expect that pressing up the arrow the month would set the date to 30 June.

That would solve the "invalid date, please try again" that the current PR enforces and sounds like an improvement for your usercase:

I know it's resolved, however, I believe that is not the ideal way of dealing with it. If I end up accidentally choosing the wrong date

Because you're accepting the date you're seeing on the screen and that cannot be wrong :)

Just my two cents. Current implementation works for me :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Have no idea at this particular codepath, so maybe my suggestion isn't as easy to implement.

When I'm on the 30 of April I would expect that pressing the up arrow on the day would set the date to 01 May.
When I'm on the 31 of May I would expect that pressing up the arrow the month would set the date to 30 June.

I think it should be surprisingly simple.

> t = os.time{year=2025, month=13, day=35}
> date = os.date("*t", t)
> print(date.year, date.month, date.day)
2026    2       4

See https://www.lua.org/pil/22.1.html

Current implementation works for me :)

I recently referred to the behavior as convenient, even if I wasn't really referring to this particular use case. (There's time sync, which is much easier to use since it does it automatically.)

@Commodore64user Commodore64user Feb 6, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

under normal circumstances, i.e setting the date in year-month-day order, one should never even get to see things like the 30th of Feb, the system now will automatically cap you at 28 or 29 days accordingly (for feb, whatever the correct number of days for other months). In other words, if you start setting the date in a sensible manner you should never even get to have those edge cases.

however, we can can still trick the system by doing what I described in my previous reply, and that is where the big NO hits you in the face.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What I'm saying is you can leave all the logic to the system.

If day > 28 or month > 12 parse it and adjust the display if necessary.

Comment thread frontend/ui/widget/datetimewidget.lua Outdated
{ month = 11, day = 31 },
}

function DateTimeWidget:isValidDate(year, month, day)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reinventing the wheel.

function NumberPickerWidget:getDaysInMonth(month, year)

@Commodore64user Commodore64user changed the title [DateTimeWidget] add key_events to dateTimeWidget [DateTimeWidget] add key_events and fix setting dates Feb 6, 2025
Comment thread frontend/ui/widget/datetimewidget.lua Outdated
Comment on lines +192 to +196
-- check if we're on the 29th February
if current_month == 2 and current_day == 29 then
-- check if the new year-value is a leap year
if value % 4 ~= 0 or (value % 100 == 0 and value % 400 ~= 0) then
-- change to 28th February

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't that also use the daysInMonth thing like on line 221? Looks good otherwise.

@Frenzie Frenzie added this to the 2025.02 milestone Feb 6, 2025
@Commodore64user

Copy link
Copy Markdown
Member Author

@Frenzie remember that this PR depends on #13208

@Commodore64user

Copy link
Copy Markdown
Member Author

unrelated to this, do you know if a Sony model: PSR T1 can run KOReader?

@poire-z

poire-z commented Feb 7, 2025

Copy link
Copy Markdown
Contributor

It you meant to write "PRS" (and not "PSR"), we may have had support for it until we removed the build because we couldn't keep building for it and it felt nobody was using it.
https://github.com/koreader/koreader/issues?q=sort%3Aupdated-asc%20PRSTUX%20

@Frenzie

Frenzie commented Feb 8, 2025

Copy link
Copy Markdown
Member

We stopped building it nine months ago, but we don't know if it even worked on one of the devices before that. (It did work years ago of course.)
https://gitlab.com/koreader/nightly-builds/-/commit/22c65a4e5e34eb3aafebc0ee069ab3b2c147f666

@Commodore64user

Copy link
Copy Markdown
Member Author

someone is selling one of those, I guess I'll pass on it then. thanks!

@NiLuJe

NiLuJe commented Feb 10, 2025

Copy link
Copy Markdown
Member

Nobody could ever reproduce the necessary changes to the system image ;).

@Commodore64user

Copy link
Copy Markdown
Member Author

👀

@Frenzie Frenzie merged commit 42fd1e8 into koreader:master Feb 27, 2025
@Commodore64user Commodore64user deleted the datetime branch February 27, 2025 19:51
0xstillb pushed a commit to 0xstillb/koreader-thai that referenced this pull request May 9, 2026
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.

6 participants