Opened 3 years ago
Closed 3 years ago
#8581 closed feature request (fixed)
Extending no content activities with images and call of actions
Reported by: | vapvarun | Owned by: | imath |
---|---|---|---|
Milestone: | 10.0.0 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Activity | Keywords: | has-patch |
Cc: |
Description
We have certain activities like
1- Changed Avatar
2- Added friend
3- Created Groups
4- Joined Groups
5- Changing Cover image for profile or Groups
these activities can be improved with image-rich features which may give a glimpse about that specific group or members with using a cover image as background.
Attachments (10)
Change History (49)
#2
@
3 years ago
Hi @imath, will need the input of saving earlier upload avatar by a member, we can use the same folder which keeps the current avatar of a member or link avatar with wp attachment to display within activity content.
like if I have changed my avatar yesterday that activity will display yesterday's uploaded avatar and if I changed my avatar today again the that will display what's uploaded today and it will not impact yesterday's avatar change activity.
#3
@
3 years ago
- Keywords dev-feedback added
I see, we don't keep previously uploaded avatars. I'll think about it and I'll be back!
This ticket was mentioned in Slack in #buddypress by vapvarun. View the logs.
3 years ago
#5
@
3 years ago
Hi @vapvarun
First of all thanks for working on this! I'm not sure is it the right time to give feedbacks about the patch but I have a few. Please ignore if you're still working on any of them.
- Storing image data as base64 in the database feels me is not the best way. It'll increase the storage requirement by 33% [ 1 ]. It will cause extra load to MySQL and object cache storage. I understand the reason of storing images in database, we want to show previous images on activities but we really need that? I'm not sure but we may can handle this situation with BP Attachments?
- Why do we need storing image file names as meta value? If we want to add this value to alt attribute of <img> tag or anywhere we want, can't we use title or name of activity item?
If we decide to go with base64,
- I think we need to move fetching and saving image process into a new function. We should check remote request (like is it WP_Error object? and HTTP response code. For example
wp_remote_get
returns aWP_Error
object when timeout). We also should check response data is empty at least. - We should provide a filter for passing arguments to remote request. I believe this could useful for developers when trying to send custom header or user-agent. For example, our CDN is waiting for user agent to return the images.
Thanks again!
[ 1 ]: https://developer.mozilla.org/en-US/docs/Glossary/Base64
#6
@
3 years ago
@oztaser Thanks for the feedback.
My initial approach was using wp attachment as bp attachment but it's still in development.
I had discussed with @imath during the team meeting he suggested going with base64.
"Using base64 images, we make sure they are only deleted when the activity is deleted"
It will definitely increase the database storage. We are open to suggestions to update the patch.
for fetching and displaying these images, we can easily wrap them using a new function with filters.
#7
@
3 years ago
- Keywords needs-refresh added; needs-patch removed
Hi @vapvarun
First thanks a lot for your initial patch. I can read from it you put a lot of energy into it. I already saw some possible improvements, but first I'm going to test and play a bit with it.
Second, thanks a lot @oztaser for alerting us about the base64 db overheat warning! My initial thought was: I absolutely wanted to avoid creating a WordPress Attachments to store the images. Because:
- in this case it can be deleted outside of the activity and we have a risk of an error due to a vanished image
- we shouldn't use WordPress attachments for community members generated media imho.
So the base64 is a bad idea, I'll try to think about it to suggest an alternative, as I've been playing with revisions on the BP Attachments feature as a plugin (I really need to find time to progress on it!!) & I have something in mind :)
Finally looking at the patch, we might be too ambitious about it. I saw you were creating new activity actions @vapvarun, I believe we should focus on existing ones first, make sure we have something robust and then create the new ones.
Important things about the feature are:
- avoid generating too much activity when the user is changing his profile image many times in a short amount of time because, he's not happy with the ones he chose...
- show the version of the avatar when the activity was created.
Let's try to work on a. and b. :)
@oztaser could you look at a. 🙏. I think it's already in BP Core, the goal is too only keep the latest activity about a profile image change. The idea is, before saving a new activity having the 'new_avatar'
type check if the user already created one of this type like in less than an hour ago and if so delete this activity and then publish the new activity.
I'll look at b. 👌
@vapvarun could you narrow the patch to existing no-content activities only ?
#8
@
3 years ago
Hi there!
Thanks for the explanation of the situation about storing images.
@imath, I've created the patch for avoiding multiple new_avatar
activities per hour. I've also added a filter to the time period.
I'm open to any suggestions about the patch.
P.S: I've created the patch from master branch. I thought this is most clear way because I was not sure about how the function I've edited would change.
#9
@
3 years ago
@oztaser your patch looks pretty nice and thanks a lot for the PHPUnit tests 💪. I'll commit it as is for now, and we'll always be able to create a specific function out of it when we'll add support for cover image and group activities.
#10
@
3 years ago
- Keywords dev-feedback removed
@vapvarun I've just tested your patch, generated activities are looking awesome!
I have a few suggestions I'd like you to work on 🙏. Let's focus on existing activity types first.
the new_avatar
activity type
- Including the cover image into the generated activity is problematic imho. It introduces a new question: Which one should we use? The one that was available at the time the avatar was changed or the latest one, in your patch it's the latest one. I believe, we shouldn't include it at all to focus on what has changed: the avatar.
- We should probably use another class than
.avatar
otherwise the theme may resize it. In the screen-capture, TwentyNineteen is doing so. I'd suggest.profile-image
instead. - The avatar should be full, maybe centered? Only the
@username
should be added and the link could be the activity directory permalink +?r=username
this way it add@username
into the activitywhats-new
textarea. Well actually what about this layout:
Avatar | |
Mention @username | View Profile |
The friendship activity type
I think this one can stay the way it is, or we could simply replace the .avatar
class by a .profile-image
one.
#11
@
3 years ago
Thanks, @imath @oztaser
working on it to improve further and will include the above suggestion, removing new cover image activities, for existing avatar change activities, I am using current cover images, I think that will be a good approach for now as we will not have to look for what was the cover image at that time, we can also add a filter there that will help developers to replace the current image with more generic activity type image.
#13
@
3 years ago
@oztaser we can use file_get_contents in case we are getting timeout from wp_remote_get.
as on local system or controlled environment, we may get empty output from it.
is it okay to go with the following approach? I was trying to avoid calling file_get_contents directly inside code but after checking inside bp core codes, it's already been used 2 times
$data = wp_remote_get( $avatar_url);
if ( is_array( $result ) && ! is_wp_error( $result ) ) {
$data = wp_remote_retrieve_body( $data );
} else {
$data = file_get_contents( $avatar_url );
}
#14
@
3 years ago
@vapvarun We probably get the same error if we try to fetching file from URL by file_get_contents
. I also prefer use wp_remote_get
for all HTTP requests but I think it makes sense if use absolute file path instead of file URL with file_get_contents
.
I don't see any problem using file_get_contents
with absolute path but I'm not sure. Maybe @imath has an any knowledge.
#16
@
3 years ago
I have an alternative to using base64 encoded images. In short it consists in adding an history
subdirectory to the user avatar one. I'll submit a patch tomorrow. @vapvarun don't worry about using file_get_contents()
or wp_remote_get()
we won't need it, I believe 😉.
NB: Old new_avatar
activities won't have the activity meta, so we'll need to only display the rich content if we have this meta.
#17
@
3 years ago
- Keywords has-patch added; needs-refresh removed
Here’s the patch I’m suggesting about the need to use the profile image file the way it was at the time when the activity was generated (the b.
point of my previous comment).
- Improve the BP Attachments API so that it can manage file revisions (It will be useful for the BP Attachments feature as a plugin) and avatar/cover images history.
- Instead of deleting an avatar when it’s being changed, move it to an
history
subdirectory. - Avoid using an activity meta to store the previous avatar. Instead, use the date the activity was created on and edit the way we name avatars so that it uses a timestamp instead of
uniqid()
. To achieve this I had to edit a bunch of functions to transport the exact same timestamp the avatars were created inside thebp_members_new_avatar_activity
function to use this timestamp as the activity date. - Introduce 2 new core functions:
bp_attachments_list_directory_files()
andbp_attachments_list_directory_files_recursively()
. The second one help use to return the current and historical avatars inside a flat array. - Introduce the
bp_avatar_get_version_src()
function to get the user avatar according to the date it was created. - Force
bp_activity_has_content()
to be true fornew_avatar
activity having an existing avatar for the activity date. See the code here. - Add a late filter to
’bp_get_activity_content_body’
to generate the rich content. See the code here. For now it simply outputs the avatar.
@vapvarun you’ll be mostly interested in 6 & 7 to generate your rich content.
NB: when possible we should always try to avoid template actions such as 'bp_activity_entry_content’
as these kind of actions might miss inside some overrides.
#18
@
3 years ago
@imath, thanks for the help; it will work; I tested the patch, the previous avatar is getting saved; just a trailing slash is missing when displaying the previous avatar. I will submit the patch shortly, including the above suggestion.
#19
@
3 years ago
Hi there!
I've read the patch and test it locally. It works very well. Thanks for working on that, @imath. I've just noticed a few code style problem :) :
- There is a
var_dump
onbp_activity_has_content
function :) - I think there is no need to create empty
$avatar_id
variable onbp_avatar_get_version_src
- There is no need
else
block onadd_revision
line 592. We can just move the line out side of theelse
block.
I have one last concern about the new_avatar
activity feature and other new activities we store old file. Should we remove avatars from history directory when new_avatar
activity is removed? What you think about that? I think it can be important for user privacy.
#20
@
3 years ago
Hi @oztaser thanks for these catches, I believe @vapvarun will include them on the patch he’s working on.
You’re absolutely right about deleting avatars inside the history directory when the corresponding activity is deleted.
We’ll include it as well.
In a future release, users should also be able to delete it from the Avatar UI.
#21
@
3 years ago
Profile Update: https://prnt.sc/20xaefu
Group Join: https://prnt.sc/20xaff4
Friend Connection: https://prnt.sc/20xafmj
#22
@
3 years ago
Thanks a lot for the revised patch @vapvarun That’s my priority of the week-end! I’ll test it asap, your screenshots looks very promising 🤩
#23
@
3 years ago
I've tested your patch @vapvarun and it works great. Thanks a lot for your work on it.
I first want to say sorry for what I'm about to write. I know both of you already invested a lot of time into this ticket, and I must say I also did!
But I think it's not ready yet. We need to work a little more on it.
- I've changed the logic to be more consistent with what I believe is the BuddyPress way of doing things.
Instead of using a filter, I'm kind of introducing a new template into our template hierarchy. The activity type one. It's useful here and I believe it can also be useful for other activity types.
I also think we need the $activities_template
global to contain the generated content parts.
Here's how it's working now:
- use specific template parts located into the
buddypress/activity/type-parts/
of the BP Ttemplate Pack - Perfom a template part look up in
bp_activity_content_body()
, if a template is found load it instead of getting the content body usingbp_activity_get_content_body()
: this way we don't need the activity filters anymore. I think it's also best because we were sanitizing empty content otherwise because all thebp_activity_get_content_body
filters were used for nothing. You'll also see thebp_activity_content_body()
function is now using a new$context
parameter so that we can control where these template loads are occuring. I believe it should only be done inside a Theme. - introduce new template tags:
bp_activity_the_content_body()
to bypass the activity types template parts look up or to use this tag inside an activity types template part.bp_activity_has_generated_content_part()
is checking if the property of a generated content existsbp_activity_generated_content_part()
is outputing the the property of a generated contentbp_activity_get_generated_content_part()
is returning the output after making sure it has been sanitized.
- Add the generated content from
bp_activity_has_content()
making sure to avoid as much as possible code duplication.
- Sometimes the cover image or the avatar are disabled...
I've added checks about this and had to edit the styles a bit in case the user has no cover image for instance. In this case including the grey background is not great imho. So I'm just outputting the avatar.
I haven't tested, but there should be adaptations to add to styles in case the avatar is disabled.
- Are we covering all existing no content activities? Should we be more creative about the layouts?
As you will see I've found at least one we were not covering so far: the created_group
one. We need to review the activities to find the ones that are not covered yet.
You will also see the friendship_created
output is the same as the new_avatar
one, and I did the same for created_group
and joined_group
(I was a bit lazy or a bit tired).
When looking at the activity stream, we basically just outputting the user profile header or the group profile header everytime. I believe we should try to make these layouts different.
- As @oztaser noticed, we need to allow users to manage "historical avatars". I don't think we should delete an avatar when the corresponding activity has been deleted. I think we should do the other way around. Delete the activity when an historical avatar has been deleted. This means we need a new tab inside the Avatar UI to pick an old avatar or delete old avatars.
- Let's drop the embed activities from this feature for now.
That being said, I think the 8581.2.patch is becoming a very huge patch. So I'd like us to keep it the way it is (unless it can be improved) and to add the needed steps from different patches. This will require us to apply 8581.2.patch, commit it locally before creating new patches. We can alternatively use this branch of my GH copy of trunk.
I'll try to work on adding the new Avatar UI tab in the coming days. @vapvarun can you bring us some creativity about the layouts and check if we're still missing some no content activities?
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
3 years ago
#26
@
3 years ago
Join Group: https://nimb.ws/YoRltZ
Friends: https://nimb.ws/FqR4hn
Created Group: https://nimb.ws/3OdVfo
Changed Avatar: https://nimb.ws/VyW4qU
Profile Update: https://nimb.ws/f89pR0
New Register: https://nimb.ws/Z5c0Mm
with center align the view with activity type template structure like this
https://nimb.ws/TwYEQl
@imath
#27
@
3 years ago
Thanks for your layout suggestions @vapvarun I think my explanations were not very clear 😬. It’s ok to have the activity look like a profile header for one type of activity. What I think is each type should have a different layout. I think it’s minor, we’ll be able to improve them during the beta period, no worries 😉. I’ll start committing what we already have: which is really great 👍.
I really like the idea, looking forward to test the patch 😉