Skip to:
Content

BuddyPress.org

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#6331 closed enhancement (fixed)

Star Private Messages

Reported by: r-a-y's profile r-a-y Owned by: r-a-y's profile r-a-y
Milestone: 2.3 Priority: normal
Severity: normal Version:
Component: Messages Keywords: has-patch
Cc:

Description

It would be nice for users to star important private messages so they can return to them at a later date.

I've written this out as a plugin at the moment:
https://github.com/r-a-y/bp-pm-starred/archive/master.zip

The plugin is very much influenced by GMail's UI. It adds:

  • A "Starred" box under the main "Messages" user nav
  • A star column in the message thread loop. (Add this patch to fix the table header: https://buddypress.trac.wordpress.org/ticket/6328)
  • A star icon in the messages loop so users can star / unstar a message.
  • An "Expand" button in the message thread if a starred message is present. When this button is present, all unstarred messages are collapsed by default. Clicking on the "Expand" button expands all unstarred messages and then turns into a "Collapse" button, which collapses all unstarred messages when clicked.

GIF:
http://i.imgur.com/cRvJ0nf.gif

My question is whether this should be part of core. If it shouldn't, I don't mind since it's already a plugin :)

Attachments (4)

6331.01.patch (19.5 KB) - added by r-a-y 9 years ago.
Refreshed for trunk and fixes an issue with AJAX mass unstarring.
6331.02.patch (31.6 KB) - added by r-a-y 9 years ago.
6331.suggestions.patch (5.3 KB) - added by imath 9 years ago.
6331.03.patch (39.8 KB) - added by r-a-y 9 years ago.

Download all attachments as: .zip

Change History (36)

#1 @boonebgorges
9 years ago

Cool! I like it. I don't see a reason not to put it in core - it doesn't add a ton of overhead (and by the looks of it, a core patch would be smaller than the plugin itself, because of some workarounds you have to do.). +1 from me.

#2 @DJPaul
9 years ago

An "Expand" button in the message thread if a starred message is present. When this button is present, all unstarred messages are collapsed by default. Clicking on the "Expand" button expands all unstarred messages and then turns into a "Collapse" button, which collapses all unstarred messages when clicked.

Is this different to how regular messages appear? I haven't used Messages for quite a while.

#3 @imath
9 years ago

Very nice :) It's giving the messages component a second youth.
Like boonebgorges, i'm adding my +1 !

#4 @hnla
9 years ago

Ah that's why you needed to patch for additional column insertion :)

+1 for core, rather than a plugin, it's nice additional functionality for messages. Not 100% sure I get the expand/collapse button in a actual message thread, first thought is that would just show fragmented parts of a general thread of a conversation, but maybe that doesn't matter, just provides a means to sort those parts of a conversation as important ones.

#5 @DJPaul
9 years ago

  • Milestone changed from Under Consideration to 2.3

@r-a-y are you able to get this done for 2.3? If not, let's update the milestone appropriately.

Only minor issue is what @hnla and I have mentioned -- the collapse/expand buttons -- but probably just need to see it in action. I'll make time to test your patch this week.

#6 @r-a-y
9 years ago

Yup, will be able to do it for 2.3.

Not 100% sure I get the expand/collapse button in a actual message thread, first thought is that would just show fragmented parts of a general thread of a conversation, but maybe that doesn't matter, just provides a means to sort those parts of a conversation as important ones.

I copied this functionality from GMail. I'm kind of agnostic as to whether this should be included or not.

Here's a GIF of the Expand / Collapse button in action for those of you who haven't tested the plugin yet:

http://i.imgur.com/rfzqdi4.gif

If we were to remove this functionality, I would still keep the per-message toggling. In the GIF, this would be where I toggle the "Nice work" message.

#7 @r-a-y
9 years ago

  • Owner set to r-a-y
  • Status changed from new to assigned

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


9 years ago

#9 @r-a-y
9 years ago

  • Keywords has-patch added

01.patch adds support for starring private messages. It tries to do so in a less-intrusive manner.

Let's say you don't want this feature enabled. Add this code snippet:

add_filter( 'bp_messages_is_starred_enabled', '__return_false' );

And this prevents the inclusion of the bp-messages-starred.php file. I'd preferably like all new functionality to follow a similar approach for upcoming BP features. Thoughts?

Unlike the proof-of-concept plugin, I added a template tag function and added it into the messages-loop.php and single.php templates because theme developers might be interested as to what the code is so they can move it around in their custom templates if desired.

What I'd like some feedback on is the naming convention of some of the functions.

  • bp_messages_is_starred_enabled()
  • bp_messages_starred_action_link() - The messages-loop.php template uses function names like bp_message_thread_delete_link() and bp_message_thread_view_link(), but also bp_the_message_thread_mark_unread_url(). Should this function be named bp_messages_the_starred_action_link()? Keep in mind that this function can be used in both /members/single/messages/messages-loop.php and /members/messages/single.php.

A couple of things I did not do:

  • Add the 'starred' action into /members/single/messages.php.
  • Add the JS for expanding / collapsing messages from comment:6.
Last edited 9 years ago by r-a-y (previous) (diff)

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


9 years ago

@r-a-y
9 years ago

Refreshed for trunk and fixes an issue with AJAX mass unstarring.

#11 @imath
9 years ago

Hi r-a-y

Very nice work and very interesting feature! So great to give some new functionalities to the messages component.

I'd preferably like all new functionality to follow a similar approach for upcoming BP features

imho, i'm not sure. I see the interest of the approach, maybe i'm not used to it, but i'd say if it's a core feature, then it should use the available files eg: put the templates thing in bp-messages-templates. Else it's something hibryd between a plugin and a core feature, and i can imagine the number of specific files that would be added for each functionality.
Or if we are to do this, then maybe a folder should be used to organize them a bit like DJPaul did with the components classes eg: /classes/class-bp-messages-message.php, and to be consistent, we would then need to do this for all existing functionalities eg: activity mentions.... So it looks like a huge reorganization to me :)

Some feedbacks :

  • Is there a specific reason not to hook bp_enqueue_scripts to fire bp_messages_starred_enqueue_styles(), i see you are using wp_enqueue_scripts ?
  • i can see an interest to allow filtering the registered icons stylesheet. If people want to use the icon stylesheet in use for their active theme. So it may be interesting to do an apply_filters( 'bp_messages_icons_styles', 'dashicons' )
  • Maybe this add_action( 'wp_ajax_messages_starred', 'bp_messages_starred_ajax_handler' ); should live in bp-templates/bp-legacy/buddypress/buddypress-functions.php :
    $actions = array(
    
    	// Directory filters
    
    	// Friends
    
    	// Activity
    
    	// Groups
    
    	// Messages
    	/* other ajax actions about the messages component */
    	'messages_toggle_star'              => 'bp_legacy_theme_ajax_messages_star_toggle',
    );
    
  • When you require the bp-messages-starred.php file is there a specific reason not to do that :
    if ( bp_messages_is_starred_enabled() ) {
    	$includes[] = 'starred';
    }
    
    parent::includes( $includes );
    
  • There's a problem with your filter to disable the functionality. I think you should either use add_filter( 'bp_messages_enable_starred', '__return_false' ); or edit the function bp_messages_is_starred_enabled() so that the apply_filter is 'bp_messages_is_starred_enabled'. Imho this should be the opposite as it's a core feature, it's on, by default, so people might want to disable it, maybe bp_messages_is_starred_disabled() ?
  • Your gif was giving me the impression the star feature was another messages action just like read, unread, or delete, viewing it in twentyfifteen i see it's a very particular action requiring it's own column. I personaly think it's complicating the process. Why not use the actions column like this :
    <td class="thread-options">
    	<?php 
    	/* since BuddyPress 2.3.0 */
    	bp_message_thread_star_item() ;?>
    
    	<?php if ( bp_message_thread_has_unread() ) : ?>
    		<a class="read" href="<?php bp_the_message_thread_mark_read_url();?>"><?php _e( 'Read', 'buddypress' ); ?></a>
    	<?php else : ?>
    		<a class="unread" href="<?php bp_the_message_thread_mark_unread_url();?>"><?php _e( 'Unread', 'buddypress' ); ?></a>
    		<?php endif; ?>
    			 |
    		<a class="delete" href="<?php bp_message_thread_delete_link(); ?>"><?php _e( 'Delete', 'buddypress' ); ?></a>
    </td>
    

You would make sure it's the first action, do the checks whether to display the star or not based on the avaibility of the feature, and it would be a lot more easy for theme designers to edit their templates by simply adding the new template tag.

  • As explained earlier, my personal preference would be to find bp_messages_starred_enqueue_styles() in bp-messages-cssjs.php just like it's the case for bp_activity_mentions_script() for instance, to find the code in the function bp_messages_starred_message_css_class() inside bp_get_the_thread_message_css_class() the screen function inside bp-messages-screens.php etc.. Because as a plugin developer wishing to extend BuddyPress i'm used to this organization :)
  • It may be interesting in the future to add new actions/message status like "read it later, important.." Maybe the star feature could be use to build a more generic message status API ?

As a user

  • i would expect to be able to bulk unstar/star. As you made this feature a particular action, i understand why it's not in the bulk select box, but imho, as a user i think it's an action just like the others.
  • When viewing my starred tab (Inbox | Starred | Sent | Compose) i would expect to see a list of the item i've starred, not a list of the thread containing one or more item i starred. If it's too complex, i guess it might be better to only allow the thread to be starred at the begining ? Because in the case of an item, i need to open the thread and look for my starred item. The star item can be hard to find, so maybe it would be interestring to stick them to the top :)

#12 @imath
9 years ago

I just realized why the star feature has his own column (although the star could prepend the subject title) and has no bulk actions in sent/inbox. Correct me if i'm wrong: the column means : this thread contains one or more starred items. If it's the case i guess some other people will make this confusion so maybe the th could be more detailled than a simple star.
+ I see now that the bulk action is in this particular column, when you click on the star if one or more items are starred it removes all starred item in the thread. (maybe a title attribute into the star link to inform "Delete all starred items" ?)
But if you click on the star when no items are starred it only stars the first item.. So in first case action is concerning all items, in the second only one.

imho if the target is the messages item, the column in inbox/sent should only be informative, and the bulk action should live in the starred tab which should only display the starred items (so not the parent thread), the link could still be the thread with an anchor to arrive on the starred item. At the same time, i don't know if it's easy to loop in messages item without looping on threads. first.

Last edited 9 years ago by imath (previous) (diff)

#13 follow-up: @boonebgorges
9 years ago

Regarding file organization. I see imath's point. But I would argue that if our organizational rules lead to files like the 6000-line bp-groups-template.php, then we should probably reconsider our rules. One thing I definitely like about r-a-y's approach is that disabling the feature will result in the file's not being loaded; this seems like a big improvement from the point of view of performance and modularity.

Regarding naming. bp_messages_is_starred_enabled() is a bit awkward when spoken. Something like "is starring enabled" or even the ungrammatical "is stars enabled" sounds more natural to me. I probably would've chosen bp_messages_stars_ as the default prefix, and adjusted it to starred when grammatically appropriate. But this is a small point :)

Should this function be named bp_messages_the_starred_action_link()?

I think so. It sounds like the unread function is incorrectly named.

#14 in reply to: ↑ 13 @imath
9 years ago

Replying to boonebgorges:

Regarding file organization. I see imath's point. But I would argue that if our organizational rules lead to files like the 6000-line bp-groups-template.php, then we should probably reconsider our rules.

Good point :)

But, in this case, i think what DJPaul did with classes and the subdirectory should then be considered for this kind of organization.

Sorry r-a-y. I won't argue more on this part as the most important is the feature itself :)

#15 @r-a-y
9 years ago

Thanks for everyone's feedback, particularly imath's!

Is there a specific reason not to hook bp_enqueue_scripts to fire bp_messages_starred_enqueue_styles()

Good spot! Will make this change.

Maybe this add_action( 'wp_ajax_messages_starred', 'bp_messages_starred_ajax_handler' ); should live in bp-templates/bp-legacy/buddypress/buddypress-functions.php :

I was thinking about that as well. I'm happy to go with what everyone else thinks here.

When you require the bp-messages-starred.php file is there a specific reason not to do that :
if ( bp_messages_is_starred_enabled() ) {

$includes[] = 'starred';

}

This is what I wanted to do originally, but we need to run the parent::includes() function first since the bp_messages_is_starred_enabled() function itself lives in bp-messages-functions.php.

Unless we move bp_messages_is_starred_enabled() into bp-messages-loader.php. Not sure if we want to do that.

There's a problem with your filter to disable the functionality. I think you should either use add_filter( 'bp_messages_enable_starred', '__return_false' );

Happy to change the filter to 'bp_messages_enable_starred', but if we change the filter name, doesn't the function name have to be the same name since we usually have the filter and function name be the same?

bp_messages_is_starred_disabled()

I think we do not want to have negative-sounding functions. I think we had a similar discussion regarding the "disable activity stream commenting on blog and forum posts" option in the BP admin area and how we switched that over to the opposite.

Your gif was giving me the impression the star feature was another messages action just like read, unread, or delete, viewing it in twentyfifteen i see it's a very particular action requiring it's own column.

Another good call here, imath.

I kind of wanted to move the "Star" column near the checkbox, but since the "Unread / Read / Delete" links are at the end of the table, I left the Star column by the existing actions.

Personally, I would love to change the "Unread / Read" and "Delete" links into icons, so the "Star" icon could go alongside them. Or maybe even remove the "Actions" column altogether and just leave those actions as a bulk action. This is what GMail does.

Would love to hear what everyone thinks about this.


my personal preference would be to find bp_messages_starred_enqueue_styles() in bp-messages-cssjs.php just like it's the case for bp_activity_mentions_script() for instance, to find the code in the function bp_messages_starred_message_css_class() inside bp_get_the_thread_message_css_class() the screen function inside bp-messages-screens.php etc.. Because as a plugin developer wishing to extend BuddyPress i'm used to this organization :)

I'm not opposed to moving the CSS class and enqueue styles stuff into the core BP messages component. I'll move it in just for you, imath :)

When viewing my starred tab (Inbox | Starred | Sent | Compose) i would expect to see a list of the item i've starred, not a list of the thread containing one or more item i starred.

imho if the target is the messages item, the column in inbox/sent should only be informative, and the bulk action should live in the starred tab which should only display the starred items (so not the parent thread),

I copied this functionality directly from GMail, but as I was implementing this I can see your point.

One change I made from GMail's functionality is if you star a thread, it stars the first message in the thread. In GMail, if you star a thread, it stars the last message in the thread. I found this awkward, so I made it star the first message.

GMail also has an option to toggle conversation view. When conversation view is disabled, all loops change to display individual messages. However, when conversation view is enabled, all loops change to threads. Since BP's mode is effectively GMail's enabled conversation view, this is what I followed.

It might be cool to have a "Conversation view disabled" mode, then we can do what you propose. However, this would probably require changing up the main /members/single/messages.php template and also changes to how we fetch messages.

i understand why it's not in the bulk select box, but imho, as a user i think it's an action just like the others.

I'll look into adding the star actions into the bulk actions dropdown menu. This was an oversight on my part!

(maybe a title attribute into the star link to inform "Delete all starred items" ?)

Will implement this.


Or if we are to do this, then maybe a folder should be used to organize them a bit like DJPaul did with the components classes eg: /classes/class-bp-messages-message.php

.

disabling the feature will result in the file's not being loaded; this seems like a big improvement from the point of view of performance and modularity.

This is what I wanted feedback on. Thanks for chiming in!

Just to be clear, I do not want to reorganize all existing functionality to this new method. Only new features going forward and when it makes sense to do so. Since the BP codebase continues to grow with awesome new features, I just want to make sure we do not follow the route of WP and including everything on each page load. If a dev wants to turn something off, then they should be able to do so without the extra code being loaded.

That being said, should the file live in a /modules/ subdirectory or something similarly named? I'd love to get the opinion of all core devs on this because this is a new approach to development.

I was thinking about leaving the code I wrote as a plugin into classes so it could live in /classes/, but this goes against how all BP code is written. So that's a no-go.

Regarding naming. bp_messages_is_starred_enabled() is a bit awkward when spoken.

Thanks for this. I was using "starred" because of the slug name, but I can switch to "stars" instead of "starred" for everything.

Thanks again for the feedback everyone.

#16 @imath
9 years ago

About the filter to disable, in your comment announcing the patch you shared
add_filter( 'bp_messages_is_starred_enabled', '__return_false' );

If i do that having the patch applied, the star functionality is still loading, because your function is :

392  function bp_messages_is_starred_enabled() { 
393	  return (bool) apply_filters( 'bp_messages_enable_starred', true ); 
394 } 

That's why i suggested to change the filter to 'bp_messages_is_starred_enabled' or change the function name to the filter as you like :)

I'm not opposed to moving the CSS class and enqueue styles stuff into the core BP messages component. I'll move it in just for you, imath :)

Thank you, but it was related to the file organization, so if everyone is fine with having all in one file, i'm fine with it :)

#17 @boonebgorges
9 years ago

That being said, should the file live in a /modules/ subdirectory or something similarly named? I'd love to get the opinion of all core devs on this because this is a new approach to development.

This seems like a good idea in the long run, but I don't see the need for it at the moment. I can only think of a small handful of toggleable component-specific features: message starring, activity favoriting, activity mentions, maybe that's it? By all means, let's break them out into separate files that are loaded conditionally. But let's not introduce new directory structure until it's actually needed - that is, until we've chopped up existing functionality to be truly modular, or until we've got so many neat geegaws that we need the extra folder :)

#18 follow-up: @imath
9 years ago

About parent::includes/require we could have a core function :

function bp_is_feature_active( $component_id = '', $feature_id = '' ) {
   return (bool) apply_filters( "bp_{$component_id}_is_{$feature_id}_active", true );
}

#19 in reply to: ↑ 18 @boonebgorges
9 years ago

Replying to imath:

About parent::includes/require we could have a core function :

function bp_is_feature_active( $component_id = '', $feature_id = '' ) {
   return (bool) apply_filters( "bp_{$component_id}_is_{$feature_id}_active", true );
}

Or bp_is_active( 'messages', 'stars' ).

#20 @imath
9 years ago

Absolutely +1 :)

#21 @r-a-y
9 years ago

02.patch now includes:

  • An extra parameter for the bp_is_active() function. Thanks for the suggestions, boonebgorges and imath! You can now deactivate the PM star component with this snippet: add_filter( 'bp_is_messages_star_active', '__return_false' );.
  • Star bulk manage support. Needed to add an extra hook into the bp_messages_bulk_management_dropdown() function. Also added some JS so the "Add star" / "Remove star" dropdown options are only shown depending on the checked item (this emulates GMail).
  • The title attribute on the star link icon (thanks for the UX feedback, imath)

I also moved the AJAX handler into bp-legacy's buddypress-functions.php since the localization of the title attribute ties things to bp-legacy. Or maybe this could be a separate JS file and enqueued on message pages only?

Function and filename prefix now uses star instead of starred or stars.

Let me know what you think.

Last edited 9 years ago by r-a-y (previous) (diff)

@r-a-y
9 years ago

#22 @imath
9 years ago

@r-a-y, just tested it and it shapes really great :)

I like the bp_is_active() thing, i find it pretty elegant actually. The only thing that disturbs me a little, but i must be overthinking it is that the feature is not really registered so doing this bp_is_active( 'messages', 'foobar' ) would return true :) At the same time, i see no risk about it.

Some suggestions :

  • Maybe we should add some unit tests about bp_is_active() just to sleep safe :)
  • I would also sleep better, if there were some kind of capability checks before starring an item :) Sorry i haven't seen this in my previous comments.
  • I really think we should let people use a different icon stylesheet than dashicons if their theme is registering one.
  • we should use bp_enqueue_scripts instead of wp_enqueue_scripts.

All these suggestions are in 6331.suggestions.patch, you'll need to apply 6331.02.patch first.

#23 @imath
9 years ago

Oops! forget about my suggestion about filtering the dashicons style, i just realized you were not using a specific stylesheet for the feature but buddypress.css.
I guess people will be able to deal with it directly in their stylesheet, and by eventually filtering 'bp_get_the_thread_message_css_class' at 11

#24 @r-a-y
9 years ago

  • Keywords dev-feedback removed

Thanks again for your feedback, imath!

03.patch includes:

  • The ability to register features in BP_Component::start() and updated unit tests to bp_is_active()
  • Switching the capability checks to bp_core_can_edit_settings() as that includes checks bp_is_my_profile() and 'bp_moderate'.
  • Using 'bp_enqueue_scripts' as the enqueue script hook (forgot to include this in 02.patch!)

Disregard the PHPDoc changes to BP_Component::setup_globals(); I'll commit that separately.

@r-a-y
9 years ago

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


9 years ago

#26 @r-a-y
9 years ago

In 9844:

Core: Introduce ability for components to register new features.

This commit allows plugin developers to register a feature in the
'BP_Component' class. To BuddyPress, a feature is merely an internal
marker. It is up to the developer to implement the feature.

Developers can check if a feature is registered with this snippet:

bp_is_active( $component, $feature_name )

See #6331 where this idea came about.

Props boonebgorges, imath, r-a-y.

#27 @r-a-y
9 years ago

In 9845:

Messages: Introduce messages_get_message_thread_id() function.

Allows developers to get the thread ID from a single message ID.

See #6331.

#28 @r-a-y
9 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 9846:

Messages: Introduce starring private messages feature.

This commit:

  • Registers the 'star' feature into the BP Messages component and dynamically loads the bp-messages-star.php file if the feature is active. By default, the feature is active, but can be disabled with the following snippet: 'bp_is_messages_star_active', '__return_false' );
  • Adds a 'Starred' box to the messages user subnav
  • Adds an icon to star or unstar a message in a message or message thread loop
  • Adds the 'Add star' and 'Remove star' options to the message thread 'Bulk Actions' dropdown menu
  • Adds accompanying CSS and JS to the bp-legacy template pack.

Fixes #6331.

#29 @r-a-y
9 years ago

In 9855:

bp-legacy: Set style precedence for message stars.

Ensures message stars show up consistently without wonky styles across
different themes.

See #6331.

#30 @hnla
9 years ago

@r-a-y if you want to avoid wonky styles, check how I've handled positioning of stars on the single message view in twentyfifteen, positioning absolute will likely have more problems than perhaps floating the parent due to it's inherent fragility.

#31 @r-a-y
9 years ago

The wonkiness I was experiencing wasn't really anything to do with positioning, but rather the star icon getting underlined when it shouldn't.

Happy for you to clean the positioning up, hnla, if you get a chance to look at it.

#32 @r-a-y
9 years ago

In 9881:

Message star: Fix usage of single form when using _n() function.

Props netweb.

See #6368, #6331.

Note: See TracTickets for help on using tickets.