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

Avatars in share dialog #4310

Merged
merged 6 commits into from
Nov 21, 2017
Merged

Avatars in share dialog #4310

merged 6 commits into from
Nov 21, 2017

Conversation

rullzer
Copy link
Contributor

@rullzer rullzer commented Dec 22, 2015

Requires owncloud/core#21311

Add avatars to the share dialog just like on the server. No avatars for federated shares yet but it is a good first step.

@guruz guruz added this to the 2.2-next milestone Jan 11, 2016
@rullzer rullzer removed this from the 2.2.0-current milestone Feb 23, 2016
@dragotin
Copy link
Contributor

API is not available on server version 9.0, moving to backlog.

@guruz
Copy link
Contributor

guruz commented Mar 1, 2016

👍

@rullzer API not available is OK, it should not produce a visible error..

@rullzer
Copy link
Contributor Author

rullzer commented Mar 1, 2016

I prefer not to merge this just yet. Since the endpoint stuff might change (significantly). For now lets keep this PR here.

@dragotin
Copy link
Contributor

dragotin commented Mar 1, 2016

ok

@guruz
Copy link
Contributor

guruz commented Apr 26, 2017

@rullzer @ckamm The Avatar code by @dragotin had been merged in master some time ago.

The share UI code has changed a lot.

Does anyone of you want to re-base this on current master so we can get it in?

@ckamm
Copy link
Contributor

ckamm commented Apr 27, 2017

@guruz Yep, I could imagine adapting this for the share dialog some time, possibly for 2.4.0.

@ckamm ckamm self-assigned this Apr 27, 2017
@ckamm ckamm added this to the 2.4.0 milestone Apr 27, 2017
@guruz guruz removed this from the 2.4.0 milestone Jun 14, 2017
[Sharing] Show placeholders for avatars

Just like on the web show placeholders for avatars in the sharing dialog

[Sharing] Show avatars!

[Sharing] Show same avatar placeholder for group/federated shares as on
web
@ckamm
Copy link
Contributor

ckamm commented Nov 20, 2017

I've rebased it on top of master and plan to fix and merge it. @rullzer fyi!

@ckamm ckamm added this to the 2.5.0 milestone Nov 20, 2017
@SamuAlfageme
Copy link
Contributor

SamuAlfageme commented Nov 20, 2017

@ckamm out of curiosity: what should we do when https://<server>/remote.php/dav/avatars/demo/128.png returns 404? same as account tab i.e. use the @ (network-native) icon; server strategy: generate an avatar with user initials in the client side? ask the core for such avatar?

@ckamm
Copy link
Contributor

ckamm commented Nov 21, 2017

@SamuAlfageme What @rullzer's code does currently is to generate a fallback avatar locally based on the first letter of the name.

* Drop AvatarJob2
* Allow AvatarJob to retrieve different sizes and users
* Make creating a circular avatar into a function
  (maybe all avatars should be made into that shape in the first place)
@ckamm
Copy link
Contributor

ckamm commented Nov 21, 2017

@ogoffart I think this is ready for review. If you agree we should always circularize all user avatars I'd be up for making that change.

Copy link
Contributor

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

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

Looks good. But see nitpick in comments.

}

const QByteArray hash = QCryptographicHash::hash(seed.toUtf8(), QCryptographicHash::Md5);
int hue = ((double)hash.mid(0, 3).toHex().toInt(0, 16) / (double)0xffffff) * 255;
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: C cast

I guess this could be rewriten as int(hash[0]), so we don't do two allocations and some non-trivial computation.

@SamuAlfageme
Copy link
Contributor

@ckamm very cool! 😍

Maybe we could consider delimiting each entry (share) using a list with alternate colors (like the one in "Sync Protocol/Not Synced") to get rid of the rectangle that encloses the checkboxes now. I remember a mockup from some time ago:

Screenshot for reference:

cc/ @michaelstingl @pmaier1

@ckamm
Copy link
Contributor

ckamm commented Nov 21, 2017

@SamuAlfageme Good idea, that'd make it nicer looking -> new issue please!

@ogoffart I've addressed the nitpick. Just using the first byte would probably break the server equivalence of the code. (I'm not 100% it's worth keeping)

The "not shared with users or groups" label only appeared if there were
no shares at all.
@ckamm
Copy link
Contributor

ckamm commented Nov 21, 2017

Actually, I tested and the colors of the fallback avatars already don't match with the server colors. I think it's good enough to just consistently color the fallback avatars somehow.

@ckamm
Copy link
Contributor

ckamm commented Nov 21, 2017

This looks good now and actually includes @SamuAlfageme's suggestion from #6176.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants