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

Initial support for event STATUS attribute #200

Merged
merged 14 commits into from
Nov 14, 2024

Conversation

myxor
Copy link
Contributor

@myxor myxor commented Apr 3, 2024

What is it?

  • Bugfix
  • Feature
  • Codebase improvement

This works on #145.

Description of the changes in your PR

  • Loads and stores STATUS attribute in database
  • Displays STATUS in event view and allows setting it to one of the three allowed values
  • Show title of canceled events as strike-throughed in events list, week view and month view
  • .ics import and export containing STATUS attribute

Before/After Screenshots/Screen Record

tmp-1712217750920
tmp-1712217765587
tmp-1712217781436

tmp-1712225897907
tmp-1712225912213
tmp-1712225970943

(sorry for screenshots being in German)

Fixes the following issue(s)

Acknowledgement

@myxor myxor marked this pull request as draft April 3, 2024 10:24
@myxor myxor marked this pull request as ready for review April 4, 2024 10:24
@myxor myxor changed the title [WIP] Initial, basic support for event STATUS attribute Initial support for event STATUS attribute Apr 4, 2024
@Aga-C
Copy link
Member

Aga-C commented Apr 4, 2024

How it's different from the field already existing in events that I got an invitation to? If it's the same, why not reuse that field for all events and use already existing translated strings?

@akki42
Copy link

akki42 commented Apr 4, 2024

How it's different from the field already existing in events that I got an invitation to? If it's the same, why not reuse that field for all events and use already existing translated strings?

Hi Aga-C,
The existing attendance status makes use of the PARTSTAT parameter of the ATTENDEE property (see RFC 5545 #3.8.4.1, whereas the new field is to use the STATUS property (see RFC 5545 #3.8.1.11).
Please also see my comment to issue 145.
Best regards,
Akki42

@Aga-C
Copy link
Member

Aga-C commented Apr 4, 2024

From a user perspective it can be misleading, but since it's a standard then okay, let's leave it as it is. Just comparing to popular calendars like Google Calendar or Outlook, I don't see a separate setting for this, there's just attendance status. Remember that this app isn't only for professional users.

However, despite that, I still see some problems:

  • Why in ICS export STATUS is exported as 0, 1 or 2? According to RFC, it should be the string.
  • Have you tested it with CalDAV synchronization? I tested it with Google Calendar, and setting event as tentative has no effect, it's not saved. When I set an event as cancelled, it gets completely removed from Google Calendar. However, that last behavior may be intended in Google Calendar, maybe it's their way of removing events.

@myxor
Copy link
Contributor Author

myxor commented Apr 5, 2024

Thanks for your feedback @Aga-C

However, despite that, I still see some problems:

* Why in ICS export `STATUS` is exported as 0, 1 or 2? According to RFC, it should be the string.

You are right - i just fixed that in commit adb4a8b

* Have you tested it with CalDAV synchronization? I tested it with Google Calendar, and setting event as tentative has no effect, it's not saved. When I set an event as cancelled, it gets completely removed from Google Calendar. However, that last behavior may be intended in Google Calendar, maybe it's their way of removing events.

I had forgotten to handle the status attribute in CalDav sync -i fixed this in my commit bb18589.
I tested it with Nextcloud CalDav backend and DAVx5 on Android. Bidirectional syncing works now. The cancelled event is not being deleted in my tests.

It would be very nice if you can retest both functionalities and give some feedback :)

@Aga-C
Copy link
Member

Aga-C commented Apr 5, 2024

I've tested, and both export and CalDAV sync looks good. Both Confirmed and Tentative status are correctly saved in Google Calendar. Cancelled too, but it seems to be how Google Calendar works, that it removes the event automatically after a short while.

@laodzu
Copy link

laodzu commented Apr 17, 2024

As a heavy Nextcloud calendar user, the STATUS attribute has become very important for my planning. It conveniently allows me to add a "blockers" into the calendar and easily visually differentiate tentative from confirmed appointments. Trying to encourage the other users of the shared calendar to do the same, I realized that the support is missing in Fossify and that this PR tries to fix this situation. Extra cool timing and thanks for the work!

So I also tested the PR, and it works fine in both directions for my Nextcloud / DAVx5 / Fossify setup. I would really like to see this PR merged soon.

But I believe the visual aspect needs improvement, i.e. "tentative" appointments should be visually discernible in the views. Nextcloud calendar shows the tentative appointments half(?) transparent and this works very well for me.

Is it possible to add this?

Thanks in advance!

@myxor
Copy link
Contributor Author

myxor commented Apr 18, 2024

Thanks for your feedback @laodzu!

It would be possible to use the same dim as the calendar uses for past events in e.g. the month view when "dim past events" is enabled. Do you mean something like this?

@laodzu
Copy link

laodzu commented Apr 18, 2024

Thanks for the quick reply @myxor ! Yes, any form of visual difference will do. If this is the most straight forward way to do it, then I think this is fine. If you have something that I can actually test, then I should be able to quickly tell you if it works (for me) or not.

@myxor
Copy link
Contributor Author

myxor commented Apr 19, 2024

Do we want this to be configurable or not?

@laodzu
Copy link

laodzu commented Apr 19, 2024

I would vote for it to be /not/ configurable. Without the visual difference, there is (IMHO) little use of the STATUS attribute as I will surely not open every item to see its status. So for me the visual difference is essential and should thus not be configurable.

As another data point, Nextcloud Calendar also does not have an option for this but always dims the tentative entries. I vote we should do the same in correspondence with the principle of least surprise.

@myxor
Copy link
Contributor Author

myxor commented Apr 19, 2024

I pushed changes implementing the dimmed presentation of tentative events with commit c4f583d

@laodzu
Copy link

laodzu commented Apr 19, 2024

Thanks for the changes Marco! You certainly made my day. I tested it and it works just like I would have expected it to work in the first place, excellent!

I have to admit that I disable the "dimming of past events" as I do not like things changing on their own. But that's only my personal preference. So with this dimming disabled, I now have a clear and immediate representation of the tentative status.

With the dimming enabled, the color is of course ambiguous, but I think introducing yet another "attribute" next to the dimming is also not reasonable. I do have ~10 calendars with individual colors and introducing yet another attribute would push the number of "regular colors" from 20 to 30 and (IMHO) this will not ease, but complicate, the understanding.

@myxor
Copy link
Contributor Author

myxor commented Apr 24, 2024

For all others: It looks like this in the day view and similar in the other views:

tmp-1713592553261

@laodzu
Copy link

laodzu commented Apr 29, 2024

@myxor Thanks again for your work. What exactly is required for this to be merged now? As far as I can see, this feature actually puts FossifyOrg ahead of other projects. On my FP4 running eOS for example, the stock calendar app is Etar Calendar, and it does /not/ have this feature.

It would be sad if this well functioning and very useful feature keeps sitting in a PR without any attention.

@naveensingh can you shed some light on what is required for this feature to be merged? Thanks in advance!

@naveensingh
Copy link
Member

What exactly is required for this to be merged now?

Time. I'm yet to review this PR.

@myxor
Copy link
Contributor Author

myxor commented Jul 24, 2024

Any plan and time to have a look?

@naveensingh
Copy link
Member

naveensingh commented Nov 14, 2024

Looks good to me!

I made some changes myself as I want to get an update out, I reverted the "dimmed presentation of tentative events" because it interferes with the "Dim past events" preference.

In the future, we'll consider adding a "(?)" icon in front of the event names to indicate the tentative status (similar to how tasks are indicated using a checkmark icon).

@naveensingh naveensingh merged commit dfbeff2 into FossifyOrg:master Nov 14, 2024
2 of 4 checks passed
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.

[Enhancement] Handle 'STATUS' property of events
5 participants