kiwanami / emacs-calfw

A calendar framework for Emacs
1.16k stars 100 forks source link

Time ranges are not properly shown in calendar #124

Open Ypot opened 4 years ago

Ypot commented 4 years ago

Time/Date range

Two timestamps connected by ‘--’ denote a range. The headline is shown on the first and last day of the range, and on any dates that are displayed and fall in the range. Here is an example:

** Meeting in Amsterdam
   <2004-08-23 Mon>--<2004-08-26 Thu>
zemaye commented 4 years ago

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 calfw-org.el

(defun cfw:org-get-timerange (text)
  "Return a range object (begin end text).
If TEXT does not have a range, return nil."
  (let* ((dotime (cfw:org-tp text 'dotime)))
    (and (stringp dotime) (string-match org-ts-regexp dotime)
     (let* ((matches  (s-match-strings-all org-ts-regexp dotime))
           (start-date (nth 1 (car matches)))
           (end-date (nth 1 (nth 1 matches)))
           (extra (cfw:org-tp text 'extra)))
       (if (string-match "(\\([0-9]+\\)/\\([0-9]+\\)): " extra)
       ( list( calendar-gregorian-from-absolute
       (time-to-days
       (org-read-date nil t start-date))
       )
       (calendar-gregorian-from-absolute
       (time-to-days
       (org-read-date nil t end-date))) text)
       )))))

which is merged in my fork

Ypot commented 4 years ago

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 calfw-org.el

(defun cfw:org-get-timerange (text)
  "Return a range object (begin end text).
If TEXT does not have a range, return nil."
  (let* ((dotime (cfw:org-tp text 'dotime)))
    (and (stringp dotime) (string-match org-ts-regexp dotime)
   (let* ((matches  (s-match-strings-all org-ts-regexp dotime))
           (start-date (nth 1 (car matches)))
           (end-date (nth 1 (nth 1 matches)))
         (extra (cfw:org-tp text 'extra)))
     (if (string-match "(\\([0-9]+\\)/\\([0-9]+\\)): " extra)
       ( list( calendar-gregorian-from-absolute
       (time-to-days
       (org-read-date nil t start-date))
       )
       (calendar-gregorian-from-absolute
       (time-to-days
       (org-read-date nil t end-date))) text)
     )))))

which is merged in my fork

Tested and it works great. Thank you!!

zemaye commented 4 years ago

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 date-string contained the current date and was doing relative time calculations.

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.

Ypot commented 4 years ago

Thanks @zemaye. We are fortunate having you using calfw and optimizing it.

MaximeWack commented 3 years ago

Thanks @zemaye for the fix!

I don't know if it's only me but time ranges still span one too many day. I'd suggest replacing :

(seconds-to-time (* 3600 24 total-days)))))

with

(seconds-to-time (* 3600 24 (- total-days 1)))))

It'd be very cool if you could submit your fix as a pull request so that it's integrated in the mainline.

Thanks!

parkouss commented 3 years ago

It would be very nice to have this fixed, and a new melpa package released too! Can I help in some ways for that?

MaximeWack commented 3 years ago

Apparently I managed to create the pull request on my own forked repo… :facepalm: Now if this could be accepted, this issue could be closed! Thanks

svictor9 commented 3 years ago

Pull request #134 correctly shows time ranges but only if they don't indicate times. Consider the following time range

* Test event 3rd to 7th of June
<2021-06-03 Thu 08:00>--<2021-06-07 Mon 09:00>

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>

MaximeWack commented 3 years ago

Indeed!

The problem seems to be deeper, in the generation of the time periods from which org-get-timestamp extracts the timeranges, but a simple check in the same function prevents the creation of the second timerange in the case a timerange indicates times. I have pushed a couple commits to update this pull request.

svictor9 commented 3 years ago

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!

svictor9 commented 3 years ago

Hmm, actually I just encountered another bug with this. Consider the following events that overlap the weekend:

* Range A with hours : sat 5th to tue 8th of June
<2021-06-05 Sat 08:00>--<2021-06-08 Tue 09:00>
* Range B without hours : sat 5th to tue 8th of June
<2021-06-05 Sat>--<2021-06-08 Tue>

When viewing the monthly calendar both events are displayed fine. But in the Week view for the week that starts on 7th of june, only the event "range B" is correctly rendered as a range. The event "range A" does not appear at all on 7th of June. Only its end time is rendered, on 8th of June.

MaximeWack commented 3 years ago

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. The start date is dotime, and the last date is dotime + (totaldays - 1)

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 cfw:org-collect-schedules-period which generates all scheduled periods for every day in the visible view.

101scholar commented 2 years ago

(seconds-to-time (* 3600 24 (- total-days 1)))))

I think it should be

(seconds-to-time (* 3600 24 (- total-days calendar-week-start-day)))))