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

Adding dav resource for avatars #4062

Merged
merged 7 commits into from
Mar 29, 2017
Merged

Adding dav resource for avatars #4062

merged 7 commits into from
Mar 29, 2017

Conversation

MorrisJobke
Copy link
Member

@rullzer @mario @marinofaggiana @AndyScherzinger @tobiasKaminsky Might be interesting for you. Then you don't need private APIs anymore and can also fetch it via WebDAV ;)

@MorrisJobke MorrisJobke added 3. to review Waiting for reviews downstream labels Mar 26, 2017
@MorrisJobke MorrisJobke added this to the Nextcloud 12.0 milestone Mar 26, 2017
@mention-bot
Copy link

@MorrisJobke, thanks for your PR! By analyzing the history of the files in this pull request, we identified @LukasReschke, @bartv2 and @tanghus to be potential reviewers.

@codecov-io
Copy link

codecov-io commented Mar 26, 2017

Codecov Report

Merging #4062 into master will increase coverage by 0.2%.
The diff coverage is 71.05%.

@@             Coverage Diff             @@
##             master    #4062     +/-   ##
===========================================
+ Coverage     54.06%   54.27%   +0.2%     
- Complexity    21274    21305     +31     
===========================================
  Files          1252     1313     +61     
  Lines         74088    81222   +7134     
  Branches          0     1284   +1284     
===========================================
+ Hits          40055    44081   +4026     
- Misses        34033    37141   +3108
Impacted Files Coverage Δ Complexity Δ
apps/dav/lib/Connector/Sabre/Principal.php 98.33% <ø> (-0.03%) 25 <0> (ø)
apps/dav/lib/RootCollection.php 95.65% <100%> (+0.19%) 1 <0> (ø) ⬇️
apps/dav/lib/Avatars/RootCollection.php 40% <40%> (ø) 2 <2> (?)
apps/dav/lib/Avatars/AvatarNode.php 40% <40%> (ø) 9 <9> (?)
apps/dav/lib/Avatars/AvatarHome.php 90.69% <90.69%> (ø) 20 <20> (?)
lib/private/Files/Cache/Propagator.php 94.93% <0%> (-1.27%) 16% <0%> (ø)
apps/comments/js/commentstabview.js 82.43% <0%> (ø) 0% <0%> (?)
core/js/jquery.ocdialog.js 1.57% <0%> (ø) 0% <0%> (?)
apps/systemtags/js/systemtagsinfoview.js 93.22% <0%> (ø) 0% <0%> (?)
... and 58 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0981f9a...6e537c8. Read the comment docs.

@MariusBluem
Copy link
Member

@schiessle for federated profile pictures in sharing dialog 😁

@rullzer
Copy link
Member

rullzer commented Mar 26, 2017

So, few things.

  • WHEE!
  • The extension might be problematic for clients
  • I'm not really happy this loads the image resource into memory again. We should just dump the file.

Anyway. Will look into it.

Copy link
Member

@LukasReschke LukasReschke left a comment

Choose a reason for hiding this comment

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

Will do some changes to satisfy coverage and CI style.

}

public function get() {
$image = $this->avatar->get($this->size);
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this as it gets an image. Instead of just a file...

@rullzer
Copy link
Member

rullzer commented Mar 26, 2017

Also no caching headers are set. Which would mean that if we switch over this will result in a shitload of requests to the server again

@rullzer
Copy link
Member

rullzer commented Mar 26, 2017

Altough I'm not sure how we can set caching headers....

if ($this->ext === 'png') {
imagepng($res);
} else {
impagejpeg($res);
Copy link
Member

Choose a reason for hiding this comment

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

Message: Call to undefined function OCA\DAV\Avatars\impagejpeg()

@rullzer 😉 – Also time to add tests to this 😝

@LukasReschke
Copy link
Member

Altough I'm not sure how we can set caching headers....

Probably something like \OCA\DAV\CardDAV\ImageExportPlugin?

DeepDiver1975 and others added 7 commits March 29, 2017 00:04
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer
Copy link
Member

rullzer commented Mar 29, 2017

Ok lets get this in and fixup the remaining stuff later.

@MorrisJobke
Copy link
Member Author

Tested and works 👍

@MorrisJobke MorrisJobke merged commit c1030a3 into master Mar 29, 2017
@MorrisJobke MorrisJobke deleted the downstream-26872 branch March 29, 2017 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants