-
Notifications
You must be signed in to change notification settings - Fork 104
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
Time ranges are not properly shown in calendar #124
Comments
Mine draws multiple periods for one time range. Is yours the same? I'm running with this patch and it looks ok so far in
which is merged in my fork |
Tested and it works great. Thank you!! |
My version still works. Though I figured out there is a two line fix maybe it will help you: diff --git a/calfw-org.el b/calfw-org.el
index 4590f1c..24b9980 100644
--- a/calfw-org.el
+++ b/calfw-org.el
@@ -238,12 +238,11 @@ If TEXT does not have a range, return nil."
(match-string 1 extra)))
(total-days (string-to-number
(match-string 2 extra)))
- (start-date (time-subtract
- (org-read-date nil t date-string)
- (seconds-to-time (* 3600 24 (- cur-day 1)))))
+ (start-date (org-read-date nil t date-string)
+ )
(end-date (time-add
- (org-read-date nil t date-string)
- (seconds-to-time (* 3600 24 (- total-days cur-day))))))
+ (org-read-date nil t date-string)
+ (seconds-to-time (* 3600 24 total-days)))))
(list (calendar-gregorian-from-absolute (time-to-days start-date))
(calendar-gregorian-from-absolute (time-to-days end-date)) text))
))))) explanation: looks like kiwanami coded such that There's a few other oddities in the outer loop I havent gotten around to fixing. For instance it creates the time period three times for a three day long period but its fine because it is only drawn once. |
Thanks @zemaye. We are fortunate having you using calfw and optimizing it. |
Thanks @zemaye for the fix! I don't know if it's only me but time ranges still span one too many day.
with
It'd be very cool if you could submit your fix as a pull request so that it's integrated in the mainline. Thanks! |
It would be very nice to have this fixed, and a new melpa package released too! Can I help in some ways for that? |
Apparently I managed to create the pull request on my own forked repo… 🤦 |
made org-get-timerange parse the actualy timerange rather than doing relative calculations dotime appears to have contained something else in the past zemaye@3d17649 kiwanami#124 (comment)
Pull request #134 correctly shows time ranges but only if they don't indicate times.
The event is duplicated in the calendar as two 5 days events: one that starts at <2021-06-03 Thu 08:00>, another that starts at <2021-06-07 Thu 09:00> |
Indeed! The problem seems to be deeper, in the generation of the time periods from which |
Indeed that solves the issue. With pull-request #134 the calendar works for me as I'd expect it, with any time range. Thanks @MaximeWack! |
Hmm, actually I just encountered another bug with this. Consider the following events that overlap the weekend:
When viewing the monthly calendar both events are displayed fine. |
Okay. So this definitely has to do with the outer loop. As @zemaye said time ranges generate multiple time periods. In details, a time period without times <2021-06-05 Sat>-<2021-06-08 Tue> generates 4 periods with the following data
And this works fine and generates the correct periods in all display modes (day/week/month) and duplicates are safely ignored. A time period with a time in either the start time or the end time, such as <2021-06-05 Sat 08:00>-<2021-06-08 Tue 09.00> generates only two periods:
This is what generated duplicates, as the start date is taken from dotime and the end date is computed, thus generating different time periods. This is where the check I added apparently fixed the bug: if current-day is equal to total-days then we are in that second case and do not generate the erroneous second time period of same length but starting at the end. As periods are not created for every day of a time period with times, this is what might affect rendering periods in other modes (day/week). The problem might come from |
I think it should be
|
The text was updated successfully, but these errors were encountered: