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.

Sign up for GitHub

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

Jump to bottom

fix(calendar): fix bug with empty date #744

Merged
merged 1 commit into from
Jun 9, 2024
Merged

fix(calendar): fix bug with empty date #744

merged 1 commit into from
Jun 9, 2024

Conversation

seflue
Copy link
Contributor

@seflue seflue commented Jun 4, 2024

  • simplify state by replacing month with date and month method
  • handle rendering correctly when changing month
  • work around a bug in date (will be fixed later)

@seflue seflue force-pushed the quickfix_calendar branch from 7503acd to 47d86e0 Compare June 4, 2024 14:53
@seflue
Copy link
Contributor Author

seflue commented Jun 4, 2024 •
edited
Loading

@kristijanhusak @chipsenkbeil I discovered a bug in Calendar while working with Org-Roam I may have introduced myself, because my changes might have exposed a bug in Date. I didn't had the time to actually take a look at the Date class, but I managed to implement a tiny workaround in Calendar and even add some tests.

I would appreciate if we could review and merge this quickly, because the bug is currently on master.

Now that I have tests (and actually an idea, how to test the widget) I will continue to improve the calendar with further PRs. I might also fix the problem in Date.

@chipsenkbeil
Copy link
Contributor

@seflue what is the actual bug you're solving?

@seflue
Copy link
Contributor Author

seflue commented Jun 4, 2024

It is an edge case with the end of month, which leads to unexpected nil date, which breaks the widget. It is in the date class, but easily to work around.

@seflue
Copy link
Contributor Author

seflue commented Jun 4, 2024

The fix of the actual cause is a bit more complicated - I will make a second PR soon.

@seflue
Copy link
Contributor Author

seflue commented Jun 5, 2024

In #748 I fixed the underlying problem, which is explained in #747. We can merge #748 before this one, then I will also remove the workaround in Calendar.

@kristijanhusak
Copy link
Member

I don't remember why I had both month and date, but could we just ditch the month and use only date? @chipsenkbeil would that cause some issues for you?

@chipsenkbeil
Copy link
Contributor

Gave a scan over where date is used (only in one file, IIRC) within org roam. I only access month as a field from OrgDate, not OrgCalendar. So I think this is fine.

@seflue
Copy link
Contributor Author

seflue commented Jun 5, 2024 •
edited
Loading

I don't remember why I had both month and date, but could we just ditch the month and use only date? @chipsenkbeil would that cause some issues for you?

That's what I actually did. The month member function is now deriving the month from the date state. Actually I had a hard time thinking over this, when I wrote my first implementation of the time picker - and already tried to get rid of month as an independent state, which was not easy with the original implementation. But with the refactoring of calendar it got reintroduce - meanwhile I had mostly forgotten, what I initially did. When I discovered and investigated the bugs, the deja vus came on after another ... With this fix it should now be much more stable.
Actually the ideas of improving the class are stacking up, so expect more PRs here.

@seflue seflue force-pushed the quickfix_calendar branch 3 times, most recently from 8126ee2 to 1fb0243 Compare June 7, 2024 00:02
@seflue
Copy link
Contributor Author

seflue commented Jun 7, 2024 •
edited
Loading

I rebased the code onto master and removed the quickfix. Now the essence of this PR is the removal of the month field in Calendar and adding some tests for it.
@kristijanhusak Ready for merge from my side.

kristijanhusak
kristijanhusak requested changes Jun 7, 2024
View reviewed changes
Copy link
Member

@kristijanhusak kristijanhusak left a comment

Choose a reason for hiding this comment

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

Looks good, just some minor changes.

Also please rebase from master. I found a bug with the calendar not being able to go to previous/next year because the month number overflows, and days_of was not able to find the correct number of days.

lua/orgmode/objects/calendar.lua Outdated Show resolved Hide resolved
lua/orgmode/objects/calendar.lua Outdated Show resolved Hide resolved
tests/plenary/object/calendar_spec.lua Outdated Show resolved Hide resolved
@seflue seflue force-pushed the quickfix_calendar branch 2 times, most recently from f919c29 to 80755ae Compare June 7, 2024 17:13
@seflue seflue requested a review from kristijanhusak June 7, 2024 17:15
kristijanhusak
kristijanhusak reviewed Jun 8, 2024
View reviewed changes
Copy link
Member

@kristijanhusak kristijanhusak left a comment

Choose a reason for hiding this comment

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

Works great! Just few minor things and it's good to go.

lua/orgmode/objects/calendar.lua Outdated Show resolved Hide resolved
tests/plenary/object/calendar_spec.lua Outdated Show resolved Hide resolved
@seflue seflue force-pushed the quickfix_calendar branch 2 times, most recently from 5a2e6bd to 05e95f2 Compare June 8, 2024 19:35
@seflue seflue requested a review from kristijanhusak June 8, 2024 19:39
@seflue
fix(calendar): fix bug with empty date
ef1cbc1 
- simplify state by replacing month with date
- handle rendering correctly when changing month
@seflue seflue force-pushed the quickfix_calendar branch from 05e95f2 to ef1cbc1 Compare June 8, 2024 19:45
@seflue
Copy link
Contributor Author

seflue commented Jun 9, 2024

@kristijanhusak I don't know, why the tests fail, it is unrelated to the code and it doesn't fail, when I run the tests locally. But unfortunately I don't have permissions to rerun the tests.

kristijanhusak
kristijanhusak approved these changes Jun 9, 2024
View reviewed changes
Copy link
Member

@kristijanhusak kristijanhusak left a comment

Choose a reason for hiding this comment

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

I can reproduce the test fail on master branch with nightly, so it's unrelated to this. Everything looks good now, merging. Thanks!

@kristijanhusak kristijanhusak merged commit 3e4dbeb into nvim-orgmode:master Jun 9, 2024
5 of 6 checks passed
@seflue seflue deleted the quickfix_calendar branch June 10, 2024 19:36
SlayerOfTheBad pushed a commit to SlayerOfTheBad/orgmode that referenced this pull request Aug 16, 2024
@seflue
fix(calendar): fix bug with empty date (nvim-orgmode#744)
8d4d8db 
- simplify state by replacing month with date
- handle rendering correctly when changing month

Co-authored-by: Sebastian Flügge <seflue@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kristijanhusak kristijanhusak kristijanhusak approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@seflue @chipsenkbeil @kristijanhusak