-
Notifications
You must be signed in to change notification settings - Fork 675
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
Avatars in share dialog #4310
Conversation
API is not available on server version 9.0, moving to backlog. |
👍 @rullzer API not available is OK, it should not produce a visible error.. |
I prefer not to merge this just yet. Since the endpoint stuff might change (significantly). For now lets keep this PR here. |
ok |
@guruz Yep, I could imagine adapting this for the share dialog some time, possibly for 2.4.0. |
[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
I've rebased it on top of master and plan to fix and merge it. @rullzer fyi! |
@ckamm out of curiosity: what should we do when |
@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)
@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. |
There was a problem hiding this 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.
src/gui/shareusergroupwidget.cpp
Outdated
} | ||
|
||
const QByteArray hash = QCryptographicHash::hash(seed.toUtf8(), QCryptographicHash::Md5); | ||
int hue = ((double)hash.mid(0, 3).toHex().toInt(0, 16) / (double)0xffffff) * 255; |
There was a problem hiding this comment.
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.
@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: |
@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.
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. |
This looks good now and actually includes @SamuAlfageme's suggestion from #6176. |
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.