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

Added Support for Microsoft Busystatus #1220

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sebu06
Copy link

@sebu06 sebu06 commented Jan 26, 2023

I added support for the ics Attribute X-MICROSOFT-BUSYSTATUS. Since this is my fist contribution to the ikhal project, I would greatly appreciate feedback on this one.

Kind regards,
Sebastian

@sebu06
Copy link
Author

sebu06 commented Jan 26, 2023

this should close pull request #989 as well...

@sebu06
Copy link
Author

sebu06 commented Jan 26, 2023

I'm using this with davmail. However, I just found out, that it only displays the status correctly, and the update only sometimes works - due to davmail doing some processing on their end as well. So if this should be fully supported, a patched version of davmail is required as well... For displaying it works well though

Copy link
Member

@WhyNotHugo WhyNotHugo left a comment

Choose a reason for hiding this comment

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

I haven't had a chance to properly test this yet, but overall seems to look okay.

Comment on lines +304 to +308
return {'BUSY': 'Busy',
'FREE': 'Free',
'TENTATIVE': 'Tentative',
'WORKINGELSEWHERE': 'Work elsewhere',
'OOF': 'Out of office'}
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to define this dictionary inside a function, just make it a global dictionary. Otherwise it gets recreated each time the function is called, which is kinda pointless.

Comment on lines +356 to +357
if 'X-MICROSOFT-CDO-BUSYSTATUS' not in self._vevents[self.ref]:
return 'BUSY'
Copy link
Member

Choose a reason for hiding this comment

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

If this property is not set, I don't think we should fall back to BUSY, we should return None instead. Especially since this property is non-standard.

Comment on lines +362 to +367
if 'X-MICROSOFT-CDO-BUSYSTATUS' not in self._vevents[self.ref]:
return ''
busystatus = self._vevents[self.ref]['X-MICROSOFT-CDO-BUSYSTATUS']
if busystatus not in self.allowed_busystatus.keys():
return ''
return self.allowed_busystatus[busystatus]
Copy link
Member

Choose a reason for hiding this comment

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

Non blocking hint: You can use self._vevents[self.ref].get('X-MICROSOFT-CDO-BUSYSTATUS', None), which returns the value (if present) or None (if absent).

Suggested change
if 'X-MICROSOFT-CDO-BUSYSTATUS' not in self._vevents[self.ref]:
return ''
busystatus = self._vevents[self.ref]['X-MICROSOFT-CDO-BUSYSTATUS']
if busystatus not in self.allowed_busystatus.keys():
return ''
return self.allowed_busystatus[busystatus]
busystatus = self._vevents[self.ref].get('X-MICROSOFT-CDO-BUSYSTATUS', None)
if busystatus not in self.allowed_busystatus.keys():
return ''
return self.allowed_busystatus[busystatus]

Which can be further simplified into:

Suggested change
if 'X-MICROSOFT-CDO-BUSYSTATUS' not in self._vevents[self.ref]:
return ''
busystatus = self._vevents[self.ref]['X-MICROSOFT-CDO-BUSYSTATUS']
if busystatus not in self.allowed_busystatus.keys():
return ''
return self.allowed_busystatus[busystatus]
busystatus = self._vevents[self.ref].get('X-MICROSOFT-CDO-BUSYSTATUS', None)
return self.allowed_busystatus.get(busystatus, ""):

@WhyNotHugo
Copy link
Member

due to davmail doing some processing on their end as well

Any idea what they're doing?

@sebu06
Copy link
Author

sebu06 commented Jan 26, 2023

Any idea what they're doing?

Yes, they seem to do the following:

on update:
set the busystatus to STATUS entry in VEVENT

on event creation:

  1. set status to Tentative if STATUS:TENTATIVE
  2. otherwise set status to Busy is X-MICROSOFT-CDO-BUSYSTATUS:BUSY
  3. if both is not true, set status to Free

I'm currently thinking there are two options: only supprort setting the status to Busy / Free / Tentative (as it seems to be the standard) and only display the MS-Status for events that have been added externally or modify davmail. I'm leaning towards the first option, since the latter would most likely lead to problems with other clients (like Dav5x) when they do not follow the MS standard.

@sebu06
Copy link
Author

sebu06 commented Jan 26, 2023

Thanks for all the feedback, I will update it =)

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.

2 participants