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

use oauth2 lib and remove deferred dependency #14

Closed
wants to merge 16 commits into from
Closed

use oauth2 lib and remove deferred dependency #14

wants to merge 16 commits into from

Conversation

kidd
Copy link
Owner

@kidd kidd commented Jul 22, 2018

Use oauth2 library instead of manual managing of tokens. http calls
are done through it so we don't use deferred.el anymore.

Use oauth2 library instead of manual managing of tokens. http calls
are done through it so we don't use deferred.el anymore.
@telotortium
Copy link
Collaborator

It's probably worth noting somewhere that oauth2 uses plstore, and thus you now need to install gpg (or else configure plstore somehow).

org-gcal.el Outdated

(defun org-gcal-end-date (time)
(if (< 11 (length time))
`("end" ("dateTime" . nil) ("date" . nil))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should have ("dateTime" . ,time), just like org-gcal-start-date above.

Copy link
Owner Author

Choose a reason for hiding this comment

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

ah, yes, nice catch.
fixed

kidd added 2 commits July 22, 2018 19:47
Entrypoint to creating an event in a google calendar without relying
on current-file, location or anything
@telotortium
Copy link
Collaborator

telotortium commented Jul 22, 2018

I've been testing this out a little - it seems that this code doesn't refresh the token properly when the access token expires - I haven't investigated to figure out why yet. It's possible that you need to manually reauthorize whenever you receive a 401 from the Google Calendar API. See also https://developers.google.com/calendar/v3/errors.

@kidd
Copy link
Owner Author

kidd commented Jul 22, 2018

yes, I'm working on that also. It looks like oauth2.el defadvice is not triggering.

Things I'm about to try are:

@kidd
Copy link
Owner Author

kidd commented Jul 22, 2018

it looks like using oauth2-url-retrieve-synchronously fixes it, but it's not ideal.
apparently oauth--url-advice is not retained correctly in the async version. Still looking into it.

@kidd
Copy link
Owner Author

kidd commented Jul 23, 2018

from url-retrieve docstring:

The variables ‘url-request-data’, ‘url-request-method’ and
‘url-request-extra-headers’ can be dynamically bound around the
request; dynamic binding of other variables doesn’t necessarily
take effect.

And yes, the async version doesn't get any extra var than the ones defined in url-http method.

It's not easy to write a hack for this to work either.

I'll be looking for alternative ways to keep asynchronicity and code reuse (thinking of make-thread or async). Until we find something that works, this is on hold.

telotortium pushed a commit to telotortium/emacs.d that referenced this pull request Jul 23, 2018
On kidd/org-gcal.el#14, some difficulties have been
encountered with the oauth2 branch, and refresh tokens don't work properly on
this branch, so I'm reverting back to the rebind branch for now.
kidd added 4 commits July 24, 2018 10:08
oauth automatic token-refreshing does not work with asynconous
url-retrieve.  That happens because dynamic bindings are not held
through the request (except a few chosen ones `url-http').

This commit comes over the synchronous + refresh problem by using
multiple threads in a synchronous way.
org-gcal.el Outdated
(org-gcal--add-time)))
'rgc-cb
(list cal skip-export)))))))
(make-thread (lambda ()
Copy link

Choose a reason for hiding this comment

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

Emacs 26.1 only ?

Just an idea, but how about carrying extra variables as
symbol-plist of calendar-id or something ?

Copy link
Owner Author

@kidd kidd Jul 25, 2018

Choose a reason for hiding this comment

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

nah, the make-thread was just a crazy test. I guess this branch is turning into an 'experimental' branch. sorry for the confusion.

I don't fully get you on the plist for the calendar-id. What would it solve?

The remaining problem here is that:

  • oauth2.el doesn't refresh the token automatically if used asynchronously.
  • using synchronous requests works, but for multiple calendars it can become a little sluggish.
  • as an "extra" annoyance, oauth2 uses plstore, and it looks like you have to enter the passphrase to decrypt the data of the tokens way to often and way too many times. This gives a shitty experience also.

I've been looking at google-contacts.el, calfw.el, and other similar packages and all of them have hacks around those. I'll revert the thread thing, but not sure what's the next thing to try :(. Feel free to throw ideas in.

@rprimus
Copy link

rprimus commented Jul 26, 2018

Thu Jul 26 18:03:24 BST 2018

Hi,

Will the move to oath2 stop events being written to .org-gcal-token whenever org-gcal-refresh-token is called?

Looking forward to this updated version being pushed to MELPA.

Copy link

@rprimus rprimus left a comment

Choose a reason for hiding this comment

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

Thu Jul 26 18:10:34 BST 2018

In org-gcal-sync, optional arg silent has been removed, however, it is still mentioned in the DOCSTRING.

@kidd
Copy link
Owner Author

kidd commented Jul 26, 2018

@rprimus , when using oauth2.el, tokens are stored using plstore.
There are some issues with this branch that still are not solved (#14 (comment) for more info).

Until we find a suitable solution of each of those, this branch won't be merged.

Lifting the call to `org-gcal-auth' was a bad idea because when the
token expired, we were sending the expired token for each calendar we
want to synchronize. Now the first one that gets the expired token
asks for renewal, and next ones use the renewed token.
@bobberb
Copy link

bobberb commented Aug 9, 2018

When this is mature I would appreciate a buzz to test this branch on windows.

@telotortium
Copy link
Collaborator

This is obsolete - see #191 for continuing progress here.

@telotortium telotortium closed this Jul 2, 2022
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.

5 participants