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

implement WebCal support #694

Merged
merged 18 commits into from Sep 15, 2016

Conversation

@georgehrke
Copy link
Contributor

commented Aug 10, 2016

No description provided.

@mention-bot

This comment has been minimized.

Copy link

commented Aug 10, 2016

@georgehrke, thanks for your PR! By analyzing the annotation information on this pull request, we identified @raghunayyar, @tcitworld and @raimund-schluessler to be potential reviewers

@georgehrke georgehrke force-pushed the feature/webcal branch from 50a96b7 to 7e34f4f Aug 10, 2016

@phsc84

This comment has been minimized.

Copy link

commented Aug 10, 2016

I'm just curious: What's the difference to #631?

@georgehrke

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2016

It uses a different approach.
#631 caches the ics files on the server. I'm not really happy with that approach.
This branch handles all of this in the user's browser.
It also reuses lots of code from #631 though.

@georgehrke georgehrke force-pushed the feature/webcal branch 6 times, most recently from e4ccb28 to 97b0f83 Aug 11, 2016

@coveralls

This comment has been minimized.

Copy link

commented Aug 11, 2016

Coverage Status

Coverage decreased (-0.4%) to 23.709% when pulling 97b0f83 on feature/webcal into 9e62f7a on master.

@georgehrke georgehrke force-pushed the feature/webcal branch from 97b0f83 to 351c6d7 Aug 11, 2016

@georgehrke

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2016

  • proper error handling
  • tests

[ ] web-based protocol handlers

@coveralls

This comment has been minimized.

Copy link

commented Aug 11, 2016

Coverage Status

Coverage decreased (-0.4%) to 23.709% when pulling 351c6d7 on feature/webcal into 9e62f7a on master.

@georgehrke georgehrke force-pushed the feature/webcal branch from 351c6d7 to efc5d21 Aug 11, 2016

@coveralls

This comment has been minimized.

Copy link

commented Aug 11, 2016

Coverage Status

Coverage decreased (-0.4%) to 23.709% when pulling efc5d21 on feature/webcal into 9e62f7a on master.

function WebCal(url, props) {
const context = {};

if (props.href.startsWith('webcal://')) {

This comment has been minimized.

Copy link
@tcitworld

tcitworld Aug 11, 2016

Member

Is this needed anymore with WebCalUtility ?

This comment has been minimized.

Copy link
@georgehrke

georgehrke Aug 11, 2016

Author Contributor

No, it is yet to be replaced

@georgehrke georgehrke force-pushed the feature/webcal branch from efc5d21 to 08fb48b Aug 11, 2016

@tcitworld

This comment has been minimized.

Copy link
Member

commented Aug 11, 2016

As of now, remote calendars are not cached, correct ?

EDIT : Inside the browser, I mean.

@coveralls

This comment has been minimized.

Copy link

commented Aug 11, 2016

Coverage Status

Coverage decreased (-0.4%) to 23.75% when pulling 08fb48b on feature/webcal into 9e62f7a on master.

@georgehrke

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2016

Exactly, it's not cached in the browser. That's what this ticket is about: #364

(We store it in the WebCalService though. When you change month it doesn't query the webcal file again)

@georgehrke georgehrke force-pushed the feature/webcal branch from 08fb48b to 6abcafd Aug 11, 2016

@coveralls

This comment has been minimized.

Copy link

commented Aug 11, 2016

Coverage Status

Coverage decreased (-0.4%) to 23.752% when pulling 6abcafd on feature/webcal into 9e62f7a on master.

@georgehrke georgehrke force-pushed the feature/webcal branch from 6abcafd to 7b76fd9 Aug 11, 2016

@georgehrke

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2016

  • fix download link to webcal calendars
@coveralls

This comment has been minimized.

Copy link

commented Aug 11, 2016

Coverage Status

Coverage decreased (-0.4%) to 23.752% when pulling 7b76fd9 on feature/webcal into 11f3f6a on master.

/**
* @param IOutput $output a small wrapper that handles output
*/
public function callback (IOutput $output) {

This comment has been minimized.

Copy link
@tcitworld

tcitworld Aug 12, 2016

Member

What about if someone provides a huge file ?

This comment has been minimized.

Copy link
@georgehrke

georgehrke Aug 12, 2016

Author Contributor

That's why I decided to use streams. 😉

@georgehrke

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2016

  • webcals is not a thing. When URL is stored as webcal try https first and fall back to http

@georgehrke georgehrke force-pushed the feature/webcal branch from 7b76fd9 to b3a9344 Aug 12, 2016

georgehrke and others added 8 commits Sep 4, 2016

@georgehrke georgehrke force-pushed the feature/webcal branch from 154ec37 to 63221bb Sep 14, 2016

@georgehrke

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2016

Let's do the web-based protocol handlers thing in a follow up pr. This pr is already huge

@georgehrke

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2016

protocol handlers: #742

@georgehrke

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2016

the 301 thing: #743

georgehrke added 2 commits Sep 15, 2016
@georgehrke

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2016

There is only one thing left:

  • fix js unit tests
georgehrke added 2 commits Sep 15, 2016
@georgehrke

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2016

@tcitworld I think this can go in. What do you think?

@coveralls

This comment has been minimized.

Copy link

commented Sep 15, 2016

Coverage Status

Coverage increased (+0.6%) to 25.782% when pulling 1094a56 on feature/webcal into fd0c66b on master.

@tcitworld

This comment has been minimized.

Copy link
Member

commented Sep 15, 2016

I didn't test your last changes, but it should be good.

@georgehrke

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2016

okay :)
Let's get this in!

@georgehrke georgehrke merged commit ba67b47 into master Sep 15, 2016

2 of 3 checks passed

Scrutinizer Running
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.6%) to 25.782%
Details

@georgehrke georgehrke deleted the feature/webcal branch Sep 15, 2016

@georgehrke georgehrke referenced this pull request Sep 15, 2016
0 of 2 tasks complete
@tcitworld

This comment has been minimized.

Copy link
Member

commented Sep 15, 2016

I'm getting php notice messages like rewind(): stream does not support seeking so this line might now be needed (see http://stackoverflow.com/a/6518288).

@georgehrke

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2016

Hm, okay. I guess we can just remove the rewind after all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.