Per thread/Multiple thread actions

I definitely like the idea of ability to "batch" operations on single of multiple threads/messages within threads.

I think it should be similiar to admin/content/node.

I've already tried to implement it but wasn't successful yet.

First of all lets define what actions we wish to perform. then lets definte what information each of those actions require. then we will figure out the commonalities between them all and create API around it. then $$ :-D

The two main ones IMO are:

1. Delete conversaion.
2. Mark conversation as read.

The tagging module may want to add the ability to tag multiple conversations too.

2 should be: 2. Mark conversation as read/unread

I definitely think that tagging module should be able to apply flag to multiple messages at once.

What simply means that there needs to be a hook which allows other modules to add more actions.

(sub-)modules could add actions by returning an array like this:

Apply tag...
- tag1
- tag2
...

Those actions could be called like this:

node module already does this for the node admin page. also user module does it too on user admin page. perhaps the same method could be applied here?

We have a similar thing in the privatemsg drupal 5 version that I could try and port over? Is that a good way to go?

Will we still keep the current theme functions or will the form based approach mean that people can modify it anyway so no need?

Maybe we can copy the node admin page. I thing we probably need to go away from the theme function per row and use a theme function for the whole page. This would also make it easier to change the whole page instead of just one row.

This overlaps with my fix at #363807: Properly Theme privatemsg_list_messages() Output, which I've marked as postponed. When this issue gets fixed let's remember to close/update that issue also. I'd be happy to help implement this functionality. I also agree, we should try to forward port the existing method from the Drupal 5 version (i.e. use the same function names), but if it's not implemented well, let's not worry about any kind of similar implementation.

Ok, I've got a first working version. The whole thing is very similar to admin/content/node

Changes/Features (the important ones)
- The privatemsg_list page is now a form
- One theme function for the whole form/page instead of per row
- The theme function is now called with the #theme property of the form array
- Each thread/row has now a checkbox at the start and there is a "select all" checkbox
- There is a action select with a execute button. Actions are loaded with hook_privatemsg_thread_operations. Each Actions has a title and a callback with optional callback arguments (100% identical to hook_node_operations)
- Instead of our own hook_privatemsg_list_messages hook, the privatemsg_list can now be completely changed with hook_form_alter, so that hook is not needed anymore (Yes I know, I added it myself... )
- Currently aviable actions: mark read/unread, delete, tagging (each tag has its own action). Of course, each of them can act on single or multiple threads
- Instead of the current drupal_set_message() information that there are no messages, the table is still displayed, but with a message indicating that there are no messages.

Some killed kittens (aka: not directly related changes):
- Display/Load only authors of non-deleted messages in the filter select field. Saw that when I deleted all messages for testing...

Todo:
- Clean up code, there is still space for improvements :)
- Especially the operation functions are currently just hacked together, they need to be improved, but probably after the api is in (for example, they should trigger the hooks, or use/be used as api functions)
- Add untag actions (?)
- Other actions, probably block...
- UI Improvements (I'm open for suggestions...)
- Maybe: Configuration options. The theme function is now pretty big, and if someone only wants a different/additional field or change the amount of authors, which are displayed, he needs to understand the theme function and keep it up to date. Some ideas
- - max number of authors to display, if the user should be displayed as author or not
- - Different fields. like number of messages, including their order
- - ...
- Maybe: Get rid of the mark all as read link. This one is the only below Messages now and isn't really needed anymore when we can mark threads as read in blocks
- Maybe: A bit related to above, reproduce the following gmail feature. When Select All is clicked and there are more than the currently displayed mails in the selection, a message like the following is displayed "All 50 conversations on this page are selected. Select all 103 conversations in ". However, I have currently *no* idea how to do this

In general, I think the UI is now pretty nice and allows to do really nice things (for example, filter with any possible combination and add a tag to all those messages with only a few clicks)

Attached are two screenshots, one without any message and one with some messages selected. Ignore the mix of german and english in the UI :)

- Maybe: Get rid of the mark all as read link. This one is the only below Messages now and isn't really needed anymore when we can mark threads as read in blocks

I agree - it is no longer useful.

Also, the actions box may be a good idea to have below the messages/pager than above it. With the filter being above, it seems to be rather crowded up there. IMO

Other than that it works well, but I am not too enthused with the execute button. maybe have a different button per task? then privatemsg_filter can add a drop down on its own.

Other than that it works well, but I am not too enthused with the execute button. maybe have a different button per task? then privatemsg_filter can add a drop down on its own.

Hm. Interesting idea, but makes it a bit harder to implement additional actions. Instead of defining hook_privatemsg_thread_operations(), they need to alter the form with hook_form_alter and add their own buttons/elements.

New version and some updates/changes
- Some minor code cleanups
- removed mark all as read-link ( There are currently still multiple "mark as (un)read"-functions, I need to clean them up, too)
- Made the delete action a button, similar to google
- Added a javascript event to automatically execute the chosen action, no need to click Execute now. Execute button is however still there, if javascript isn't activated.
- Added (un)blocking action
- Moved the actions fieldset to the bottom and it is now open by default.

Remaining todo-items:
- Clean up code, there is still space for improvements :)
- Especially the operation functions are currently just hacked together, they need to be improved, but probably after the api is in (for example, they should trigger the hooks, or use/be used as api functions)
- Add untag actions (?)
- UI Improvements (I'm open for suggestions...)
- Maybe: Configuration options. The theme function is now pretty big, and if someone only wants a different/additional field or change the amount of authors, which are displayed, he needs to understand the theme function and keep it up to date. Some ideas
- - max number of authors to display, if the user should be displayed as author or not
- - Different fields. like number of messages, including their order
- - ...
- Maybe: A bit related to above, reproduce the following gmail feature. When Select All is clicked and there are more than the currently displayed mails in the selection, a message like the following is displayed "All 50 conversations on this page are selected. Select all 103 conversations in ". However, I have currently *no* idea how to do this

Btw, have you seen the new Gmail Label Drop-Down? Just wow.... but probably a bit too much of a good thing for us ;)

I am getting hunk failures when trying to apply the patch, Please reroll for 6.x-dev

Also notice you have a bug in filter module in sql where table is incorrectly wrapped along with it's alias for db prefixes: {pm_index pmi}

Also i am not sure about this:
"- Added a javascript event to automatically execute the chosen action, no need to click Execute now. Execute button is however still there, if javascript isn't activated."

while this sounds like a nice feature, imagine how often people make mistakes choosing wrong actions. From personal experience i know that i choose wrong actions on first try more often than the action i actually wanted to chose :-p.

Oh, forgot to cvs update before creating the patch. I will upload a updated patch soon.

while this sounds like a nice feature, imagine how often people make mistakes choosing wrong actions. From personal experience i know that i choose wrong actions on first try more often than the action i actually wanted to chose :-p.

I added that to make it similar to gmail, however, gmail provides a undo option after each action. Would be nice to have that too but I have to think about how to implement it.. maybe saving the last action in the session.

Perhaps we should include confirmation box with 'are you sure..........?' and yes and cancel buttons that confirms our chosen action to make sure we didnt make a mistake choosing wrong one.

Updates..

- Undo support for Delete, Mark as (un)read and (un)block. I think, this eliminates the need to provide a "are you sure"-form. If you clicked wrong, you hit undo and you're good :). See http://dev.worldempire.ch/undo.ogv for an short example of how it works.
- Changed the list/list_sent code, it is now simply a parameter and is set in hook_menu. This is more flexible and would for example allow to define other url's for viewing other users' inbox without changing privatemsg_list.
- Also fixed a small bug when checking $uid (is_int doesn't work, because the uid is a string..), but that is probably still broken, especially for actions..
- The actions now use the following functions: privatemsg_thread_(delete|read|block). it should also be possible to use them directly as api functions
- started to document some of the functions..

Todo
- Improving tagging stuff, not sure how to proceed.. should that be an own select field or part of the global one?
- Configuration options
- The current delete function is a fast, single query for N threads. That's need to be changed to trigger the delete hook of each single message when the api patch is commited.

It seems that delete action doesn't remove the message from DB: db_query('UPDATE {pm_index} SET deleted = %d WHERE uid = %d AND thread_id IN ('. db_placeholders($threads) .')', $params);

I am curious at what stage will messages be really deleted from database. assume that if all recepients of these messages delete them through actions, then they are not really deleted. but if we really delete them, then it cannot be undone.

Perhaps we need a method to scan for messages where all recepients set the message status to 'deleted' and these deletions were not undone for 5+ minutes, then we can delete them safely from the database.

We could approach this from hook_cron() if we decide to implement this method at all.

subscribe

Here is an updated version. I excluded the changes from privatemsg_filter and pm_block_author for now as the patch is already more than big enough.

Changes:
- The header for the table is now built in the _privatemsg_list_headers() function. The generated array is stored in $form[#headers]. There are two possible special keys that are used by privatemsg. 'key' => 'The key in the rows array for that column' and '#weight' => 'order of the column'.
- The whole db result array for each row is stored in $form['#data'], indexed by the thread_id
- The function _privatemsg_list_thread() builds the default row array for the theme_table. However, instead of strings with span-tags, it does now use the features of theme_table to add the css classes directly to the tr and td tags. This cuts down the markup and makes it easier to theme the whole table instead of only the content. Each row is stored in $form['#rows'], indexed by the thread_id.

- Now, what the theme_privatemsg_list_messages function does is order the headers array according to #weight and then builds up each row in the same order.
- The big advantage is now, that modules can change and extend the #headers and #rows arrays with hook_form_alter. This allows to add data and change the order of the columns with that hook. Look at the two mentioned functions to find out how the data is structured.

Other changes:
- Using tablesort_sql to sort the table
- privatemsg_thread_delete now loads each message of the selected threads and deletes it, I needed to change privatemsg_sql_messages to accept an array of threads instead of only a single one.

Todo/Questio
- Currently, the two functions that generate the row and the headers are normal functions, not themes. I may have to change that too, because atleast the header is passed directly to tablesort_sql right now and there is now change to change the headers then. Not sure how bad that will influence performance, but it shouldn't be a big issue.
- I try to split up the patch to make it easer to review. But it is pretty hard because all changes are tightly related...
- Add CSS Classes to the header

Oh, forgot one thing in the Todo list

- I changed a bit the order of the order by mechanism, it is currently "is_new DESC, " . $order_by_from_tablesort. This means that new messages are *always* listed at the top, not sure if we really want that.

I think this one is now pretty ready..

Updates:
- Converted the list_header/thread functions to theme calls, so that themes can set "default" headers/rows.
- Added CSS Classes to the header
- Moved new code to the end, makes the patches easier to read.

Can't get the patch working with a new 6.x-1.-dev downloaded version. anyone else having the same trouble?

I think this patch has features that will break the filtering and tagging settins I'm working on and I need to test them. Also I would like to make a review of the 'message routing' code: AKA autotagging or something like that, and I hope the code for both (searching/filtering, current thread actions and routing (special thread actions) ) could be the same.

define *working*. can't you apply the patch or do you get errors ?

The patch should apply, but you need to reload the menu cache, because I changed how the function is called by the menu system.

Also, appart from privatemsg_thread_delete/privatemsg_thread_read, that patch is only about theming and the UI. Autotagging should imho be done in a hook_privatemsg_message_submit (Assuming we are talking about the same thing) and that is nothing that gets touched even remotely by this patch.

However, what this patch allows is easily adding data to the listing, that might be interesting for you.

this comment fixes the following stuff:

- some minor codestyle:
false should be written always as FALSE

$form['#rows'] can have no elements -> a error is displayed

here is a new patch

I meant:

And about..

Also, appart from privatemsg_thread_delete/privatemsg_thread_read, that patch is only about theming and the UI. Autotagging should imho be done in a hook_privatemsg_message_submit (Assuming we are talking about the same thing) and that is nothing that gets touched even remotely by this patch.

I know this patch is not about, but this patch is about the ui, and I'm getting obfuscated looking changes in the UI where I'm also working on. So this is the reason why I decided to include the patch in my dev system, just because I'm working in the ui (I talked about the parts I'm touching), nobody says no, but everybody is working in the ui at the same time XD

I'm going to do a write in the oppened UI thread #351542: user interface about what's going on about filtering and tagging so you can consider what's modificable and what is not if you want.

@ilo try the new patch with patch -p0 inside the privatemsg directory.

Some changes, mostly based on irc discussion.. thanks dereine for the review and the ideas!

- renamed thread functions to privatemsg_thread_change_delete() and privatemsg_thread_change_status()
- added constants PRIVATEMSG_READ and PRIVATEMSG_UNREAD and used these instead of 0/1
- removed privatemsg_set_new_status
- renamed privatemsg_mark_as_read to privatemsg_message_change_status and privatemsg_message_delete to privatemsg_message_change_delete
- added a $delete/$status parameter to both functions, so that it is possible to undelete/unread messages
- Added a $load_all parameter to privatemsg_sql_messages, this loads the deleted messages too
- As I found out, I broke the undo possibilty for delete messages, it does now work again (needed $delete parameter for message_change_delete and $load_all for privatemsg_sql_messages)

- The idea behind the function renames was to make thread/message api functions more consistent. for status and delete is it now: privatemsg_{thread|message}_change_{status|delete}({$pmid|$threads}, {$status|$delete}, $account = NULL)

Thanks berdir, but that was my first option :(

I did, (contacting support team I mean).. so ugly XD

Next week I'll try in linux. For now it's too much for me. Updated: #351542: user interface comment #41

Hm, that seems to be a bug in your patch client, no idea what happened there, never used patch on Windows.

I like it. nifty and the undo function is way cool.

However if we are doing auto apply, I do not think the execute button should show. Maybe hide it using some fancy javascript or something?

Another thing - the tagging module's list filter does not seem to appear on my test install.

This should work, but I've found some cases not working in firefox..

We need to use javascript to hide the button, because he needs to be there if javascript is not working. I tried to do it, but for some reason it didn't worked as expected. We can also not remove the button, only hide, because he is clicked with javascript so that the correct submit function is executed.

Answer to #380534: per thread(s) Actions mechanism should be refactored:

The idea and code from the operations hook and the submit/execute part is heavily borrowed from the node administration page, in fact, it is mostly the same code + undo stuff.

The big advantage of a simple operations array/select is the more or less simple processing in the _submit function. Currently, the delete button has his own submit hook. I like the idea of having more control over it, but we need to make the _submit function more flexible.

The question is if we still need the operations hook then, we could also simply use hook_form_alter with some conventions on how to name the additional data we need (like callback, undo and options for both).

@nbz
The tagging stuff is currently not part of the patch anymore, same for block_author. I thought it would be easier to get the "actions-framework" in and add tag/block actions in follow-up patches. Especially since ilo is working on filter.

I'll submit a patch to remove tags from _filter module and create new _tags module tonight. As nbz says, tagging would be handled in other ways.

about #380534: per thread(s) Actions mechanism should be refactored, I've been thiking about two different types of information in the form: one is for building a form with several available actions, and other is to manage actions and undone stuff. Why not having both? You current system works fine creating the submit code to perform any action, I don't want to remove that, just improve the UI a little by rendering more elements in the form, keeping the same submit functionality it has now.. (maybe instead of one "execute" button, each new operation array may need it's own "execute-$name" button, but all processed in the same submit function. This can be done, just think about making more flexible the ui but keeping the same 'action' array you are using right now.

It's much better to have a single point for adding options in a form than forcing other modules to attach their content (the form_alter is an option that will never dissapear, and can be used if the other things fail). As an example, I'm trying to integrate now the _tags filter with the old_filter module (not the human readable code setup in demo site), and I'm about to kill someone..

what is expected? I use _form_alter to insert the 'tags' select form element, and I setup a new [#submit][] function in the form.. and..
oh.. when the user clicks filter, this code is executed:

The drupal goto redirects completely the current form submit operation, not allowing other [#submit] hooks to execute. I need to alter the #submit order to execute mine first, but.. if I setup some filter options, and execute "privatemsg_filter_dropdown_submit" before my own submit to handle tag information what happends? The current $_SESSION filter value is overwritten completely by privatemsg_filter submit function:

So, now, to integrate both (_tags and _filter) I need to write more code to handle the filter submit case than to manage user tags..

This is poor flexibility in integration design, I'm trying to avoid that in the actions module also.

Having a hook to add or remove custom actions (not in a select list, but as elements form), may allow much more flexibility.. I hope. My experience is about new _filter implementation to manage filter options, and how easy is to create a new filter option (as explained in that long thread called "User interface").

I didn't mean to replace the generic submit, in opposite, I'd like to kill the _submit_delete function, if possible. I will think about how we could do this.

Integrating privatemsg_tags with the current privatemsg_filter is only to provide filter by tags, right? And your privatemsg_tags module provides a messages/tag/xy link to display messages of a specific tag, right? So the only thing that will not work for some time is search for one or multiple tags with the filter "widget" ?

Then I would say that there is no need to introduce legacy code from the beginning, that will be removed again as soon as your new privatemsg_filter module is ready. privatemsg_filter wasn't designed to provide flexible integration.

So, in your opinion, berdir, can I forget about patching old _filter module to integrate tag system in the search for now, and focus on the new filtering schema, as we know this could integrate well? that would help me a lot

Note about searching using the filter widget: We have no OR statement yet, it will be included soon as a feature I hope, but multiple selection can be used for now as AND. Making the OR is not dificult, just have to be implemented in multiple selection enabled selects.

I'd say so, yes. just make sure you have a working upgrade path. Atleast activate the tags module, no idea if you need to change the database schema.

Would using an "OR" give the expected results? TO me that does not seem to be awfully useable - I would expect using more terms to search by would refine the results instead of giving more options.

What is technically possible may not always add up to be the most useable option.

As for separating privatemsg_filter into two modules, I would think of that as a bad idea too, but I may be wrong. Having a load of interdependent modules would be like building a house of cards - IMO the core infrastructure stuff needs to be in privatemsg.module and every thing else build off that as much as possible.

(I fear that the (set of) modules are starting to lose their simplicity and "elegance". Something being cool does not mean people will use it.)

Yes, the upgrade is tested (works with the update.php) to add a field in the pm_tags_index table. Also I will activate the module automatically on update and that will do (this is not done yet). Thanks

@ Berdir - How about looking at the code in this module:

http://drupal.org/project/hide_submit ?

Can that code be modified to hide the button?

Just noting that I'm working on implementing some of your feedback, will take 2-3 more days.

Things I'm working on:
- Working with theme patterns which splits up each field into it's own theme function. this makes it very easy to provide more fields and add default theming for them. This is the same system as views uses. - Works, but needs to be cleaned up.
- Remove the execute button(s) - Works. Had to create a js file and register the remove call as a behavior
- Make the operations hook and submit function more flexible. Maybe we can even use the same hook for filter and actions - Nothing done yet, need to think and sleep about this.
- Re-add parts of the filter patch to display the filter "widget" again.. forgot that one - Has been written, I only need to split up the changes in privatemsg_filter

Ok, here is a updated patch, changes:

- Converted the row theme to a pattern-based system, similiar to Views2. Each field and its header can have a theme function and returns nothing by default. These theme functions are now in the file privatemsg.theme.inc. privatemsg.module provides theme functions for subject, authors, recipients, last_updated, count and thread_started.

To add a field and a default implemention, other modules need to add the field to the query and provide a field and header theme functions. See privatemsg.theme.inc for examples.

For technical reasons, the functions need to be prefixed with phptemplate_ instead of theme_. They can be overwritten by themes by prefixing them with the theme name, either as a function or as a template file.

- As an example of what this system allows, I've added a configuration setting where the user can enable/disable these fields, except subject (defaults to authors and last_updated). Currently this only affects the list query, not list_sent.

- Added a JS behavior function which removes all input fields with the privatemsg-action-button class.

- Some changes to the operations hook and the submit function. I was not yet able to include a system similiar to what ilo thought of because of the undo handling and other things. Because we have hook_form_alter, I think this is not very important right now. What I changed is the ability to add "hidden" actions. This allows other modules to add buttons and use the default submit function. "hidden" operations don't have a label key. This is currently used for the delete button. If no operation is selected, the submit function looks if there is a operation for the clicked button (name).

- Included a small patch for privatemsg_filter to re-add the filter widget.

- With all the theme functions etc. the patch has now a size of 33,7KB. I need to speek with litwol if it is possible/necessary to split the patch in several parts.

Hint: Don't forget to rebuild the theme registry after the patch has been added.

Just testing this.

While the privatemsg_filter widget shows, the filter/save filter buttons do not do anything.

Fixed.

Perfect! Patch tested and it all works!

I noticed this is mostly to perform actions on whole threads, it would be awesome if it was possible to do the same within threads, perhaps even be able to split them. We have recently imported from PhpBB, and all imported messages were merged into one HUGE big thread. If it were possible to split that into several threads, or even bulk delete within the thread, you would make 200 people VERY happy indeed... especially since all their deleted messages were somehow imported as well.

It may be a good idea to file a bug report with the phpbb2drupal module about the import going awry (I am the maintainer of it, but this topic would not be the right place to ask for details) with details of versions etc (are the thread ids being set to the same one for them all? what number?). Maybe it can be fixed?

As a quick fix, if you want to break ALL the messages into their own thread try running something via phpmyadmin along the lines of:

or something.

(the above is psuedo code and I do not expect it to work at all or break all the data in the site... but I have used something similar in the past)

As for the original patch, while I have not reviewed it properly, I can also confirm that it works well.

EDIT - I think it may need to be rerolled for the changes made in http://drupal.org/cvs?commit=179350

Thank you nbz, I have done so as you know :)

Anyway, I just wanted to report a tiny bug: if you click "Mark as read" or "Mark as unread" if you have NOT ticked any messages, you get an ugly error message:

Not a big deal, you're not supposed to do it anyway, but I thought I'd report it anyway.

Adding the same form to a single thread is a good idea, but I'd like to delay that for another issue.. that patch is already too big.

I'll have a look at the error, that should definitly not happen.

Changes
- updated privatemsg_filter to check for the correct permission.
- only execute action callback if atleast one thread is selected

Please check if that fixes your issue.

The reset button on the filter widget does not work.

(sorry about dripping things, but it is a large patch and a lot of the code does go over me).

No problem, I'm happy about any feedback I recieve :)

Forgot that, is fixed. Will upload a new patch soon.

Uh, what was that...
Double post and the text is missing...

Was: Updated to fix the reset button.

Re-uploading patch.. something really went wrong :)

Updated the patch and removed the $query_name stuff as discussed with nbz. No other changes.

Patch is actually not really smaller now, but #303087: Move "Sent Messages" to privatemsg_filter, Add inbox, improve user filtering. should not depend on this patch anymore when nbz has updated the patch. Of course, they would still need to be re-rolled if either this one or nbz's patch gets in, but that should be pretty easy to do.

I cannot seem to be able to view the authors column at all... I can select and unselect the other columns, but the authors column makes no difference.

Another thing. If the "started" column is shown, I think it would do better to come before the last updated one. Is it possible to move?

One other thing I noticed which may be hard to work around though - try marking some already marked read threads as read. When you choose undo, they will not be unread.

I can't reproduce the author thing, it's working correctly for me (but only on the list page, not sent).

Yes, I can change the default weight of the started column. The masterplan on that stuff is to allow two things:
a) use a hook, so that other modules can easily add their fields to the configuration.
b) Use a dragable table which you can use to set the weight of the field, similiar to menu/blocks for example.

But I just wanted to have something working to show what the new system can do.

When I mark something already read as read and click undo, it is then unread. The question is, is that the intended behavior ? Probably not, but that's very hard to, I think.

The first problems may be something to do with my wampserver install - it seemed to stop working soon after that (cannot access anything on my localhost now).

As for the undo behaviour, it thought it would be hard to fix and leaving it as is is no major problem for me.

I am tempted to set this to rtbc, but I would like someone else to confirm that it works (thus it not simply being my setup (where I have alternated with participants and other stuff too while developing other patches) that has gone awry).

I say get this in as is and if there are problems, they can be fixed instead of keeping on rolling and rerolling this patch.

When I mark something already read as read and click undo, it is then unread. The question is, is that the intended behavior ? Probably not, but that's very hard to, I think.

Maybe we should only use the Undo feature on deletions?

Tried to reproduce the problem in #48. Mark as read/mark as unread and delete, with no messages selected, does not generate any errors.

As for #58, I can reproduce this by switching off the 'Last updated' column. It's like the list 'falls back' to the pre-patch output. I also see this error:

Seems ORDER BY is the problem. Presumably switching off the 'Last updated' field also removes that field from the ORDER BY, and that a check is needed to see if the fields to order by are blank.

Maybe we should only use the Undo feature on deletions?

That would not really help imho. If the filter module or something else would add a Trash-View, we would have the same problem there.

The problem is that the undo feature doesn't undo the action it has actually done, instead it simply does the reverse of the previous action. And it is hard to actually undo what it has been done, because we would need to select all the affected data first and save what is going to change. But I don't think it's a big problem.

However, good catch with last_updated, it is needed because of the default sort. For now, I just made it a required field, maybe I can make the default sort configurable later on. Test the updated patch, please.

should we make a new issue about how to handle the undo feature? I guess there's the need of save the current thread action, and that has not been considered yet.. I guess it deserves a brainstorm, and this way we can commit this patch (removing the undo information)

Just got my local server working again and the patch works. (an update from microsoft had overwritten the previous hosts file...)

There were questions on irc where there was some disagreement over the undo feature and whether it should go in as a separate patch, but I think it should go in as is and any changes can then be addressed through smaller patches - it gets hard trying to find out what changes between patch revisions atm and I do think this is ready to go in.

Another thing to note - this patch only adds the selectable columns to the all messages page - the "sent messages" page is still sort of hard coded with tis columns, but that should be fixed in my follow up patch that removes the current sent messages anyway.

Setting to rtbc.

litwol did say in IRC that the undo behaviour should set the messages back to their previous state, not just reverse the last operation. I'll let him chime in on that though.

FWIW, my opinion is we shouldn't bother offering an undo operation for read/unread messages, only for delete (no quirks that way). Then, when the deleted items filter is in place, undo could be removed entirely.

Possible usability issue: when trying to mark threads as Unread, one thread kept its Read status. Further investigation showed the thread was a single message that had been sent by the current user. Well, duh, it can't be marked as unread, which is correct behaviour (I believe), but we shouldn't say '2 threads marked unread' -- or whatever -- when they weren't. Small issue, but it's unexpected.

subscribe

Making the undo feature more "intelligent" is not easy. Each change method would need to store and return every affected message. Exactly, message, not thread (atleast for mark as read, delete and blocking actions). This means that the message_change_xy functions need to be changed to be able to work on multiple messages and the thread_change_xy functions would need to specify the message_change_xy functions as their undo action.

I can try to make it work as described above but that should not be part of that patch imho. My suggestion would be to commit that patch either as it is or remove the feature for now. However, If removed, we need to provide an confirmation screen, because it is very easy to click the wrong action, tag, whatever.

And even if there would be a Trash and you could go there and restore the deleted messages, a simple undo link is imho much faster and easier to use if the wrong button was clicked.

We should probably discuss this in IRC when I'm there :)

@ nbz overwritten hosts file is nasty, had that recently on ubuntu too. FYI, I always use unique names for my patches, It should be possible to diff the patch files. That's what I do often to check if I haven't introduced any unwanted changes. However, that only works well with a small amount of different lines.

Today I'm going to continue my review from the weekend. Berdir, if you have some time today, come over to IRC :)

Updated patch with some minor fixes, changes

- fixed a bug, it was not possible to mark the message/thread with mid 1 as read/unread
- removed last_updated default value from privatemsg_display_fields
- removed trailing slash from path definitions
- reformated privatemsg_sql_list a bit, moved all required fields to the top
- Improved api docs a bit and converted to the correct format

- added a comment which hopefully explains the array_slice in _privatemsg_generate_user_array(). Except that, I haven't refactored that function in that patch yet. I think that should be done in another patch, because it isn't easy (While it would be possible with the PGSQL query, mysql doesn't support GROUP_CONCAT in combination with LIMIT, so I need the array_slice in that function). I will probably create a patch which moves that subquery to an simple query which is executed for each line, to keep the cost curve flat. This would also eliminate the need of a different query for pgsql and mysql. Another thing to consider is that D7 now supports user_load_multiple() with included static cache. this means that the whole function can be replaced with a single user_load_multiple() call in D7.

I will also add a diff which shows the changes in the patch. It is a bit hard to read but might help to see which parts have been changed.

hi,

just tested the patch with the .dev version.

I've got this:

warning: Missing argument 1 for privatemsg_list() in /home/pariscin/www/sites/all/modules/privatemsg/privatemsg.module on line 367.
warning: include_once(sites/all/modules/privatemsg/privatemsg.theme.inc) [function.include-once]: failed to open stream: No such file or directory in /home/pariscin/www/sites/all/modules/privatemsg/privatemsg.module on line 1475.
warning: include_once() [function.include]: Failed opening 'sites/all/modules/privatemsg/privatemsg.theme.inc' for inclusion (include_path='.:/usr/share/php5') in /home/pariscin/www/sites/all/modules/privatemsg/privatemsg.module on line 1475.
user warning: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'LIMIT 0, 25' at line 1 query: SELECT pmi.thread_id, MIN(pm.subject) as subject, MAX(pm.timestamp) as last_updated, MAX(pmi.is_new) as is_new, GROUP_CONCAT(DISTINCT author SEPARATOR ",") as author FROM pm_message pm INNER JOIN pm_index pmi ON pm.mid = pmi.mid WHERE (pmi.uid = 7) AND (pmi.deleted = 0) GROUP BY pmi.thread_id ORDER BY LIMIT 0, 25 in /home/pariscin/www/sites/all/modules/privatemsg/privatemsg.module on line 391.
warning: include_once(sites/all/modules/privatemsg/privatemsg.theme.inc) [function.include-once]: failed to open stream: No such file or directory in /home/pariscin/www/sites/all/modules/privatemsg/privatemsg.module on line 1475.
warning: include_once() [function.include]: Failed opening 'sites/all/modules/privatemsg/privatemsg.theme.inc' for inclusion (include_path='.:/usr/share/php5') in /home/pariscin/www/sites/all/modules/privatemsg/privatemsg.module on line 1475.

@junro
Clear the menu cache.

I cleared the cache in /admin/settings/performance, and got this after in the same page.

warning: include_once(sites/all/modules/privatemsg/privatemsg.theme.inc) [function.include-once]: failed to open stream: No such file or directory in /home/pariscin/www/includes/theme.inc on line 286.
warning: include_once() [function.include]: Failed opening 'sites/all/modules/privatemsg/privatemsg.theme.inc' for inclusion (include_path='.:/usr/share/php5') in /home/pariscin/www/includes/theme.inc on line 286.
warning: include_once(sites/all/modules/privatemsg/privatemsg.theme.inc) [function.include-once]: failed to open stream: No such file or directory in /home/pariscin/www/includes/theme.inc on line 286.
warning: include_once() [function.include]: Failed opening 'sites/all/modules/privatemsg/privatemsg.theme.inc' for inclusion (include_path='.:/usr/share/php5') in /home/pariscin/www/includes/theme.inc on line 286.
warning: include_once(sites/all/modules/privatemsg/privatemsg.theme.inc) [function.include-once]: failed to open stream: No such file or directory in /home/pariscin/www/includes/theme.inc on line 286.
warning: include_once() [function.include]: Failed opening 'sites/all/modules/privatemsg/privatemsg.theme.inc' for inclusion (include_path='.:/usr/share/php5') in /home/pariscin/www/includes/theme.inc on line 286.
warning: include_once(sites/all/modules/privatemsg/privatemsg.theme.inc) [function.include-once]: failed to open stream: No such file or directory in /home/pariscin/www/includes/theme.inc on line 286.
warning: include_once() [function.include]: Failed opening 'sites/all/modules/privatemsg/privatemsg.theme.inc' for inclusion (include_path='.:/usr/share/php5') in /home/pariscin/www/includes/theme.inc on line 286.

and got this in /messages:

warning: include_once(sites/all/modules/privatemsg/privatemsg.theme.inc) [function.include-once]: failed to open stream: No such file or directory in /home/pariscin/www/sites/all/modules/privatemsg/privatemsg.module on line 1475.
warning: include_once() [function.include]: Failed opening 'sites/all/modules/privatemsg/privatemsg.theme.inc' for inclusion (include_path='.:/usr/share/php5') in /home/pariscin/www/sites/all/modules/privatemsg/privatemsg.module on line 1475.
warning: include_once(sites/all/modules/privatemsg/privatemsg.theme.inc) [function.include-once]: failed to open stream: No such file or directory in /home/pariscin/www/includes/theme.inc on line 593.
warning: include_once() [function.include]: Failed opening 'sites/all/modules/privatemsg/privatemsg.theme.inc' for inclusion (include_path='.:/usr/share/php5') in /home/pariscin/www/includes/theme.inc on line 593.
warning: call_user_func_array() [function.call-user-func-array]: First argument is expected to be a valid callback, 'theme_privatemsg_list_header' was given in /home/pariscin/www/includes/theme.inc on line 597.
warning: include_once(sites/all/modules/privatemsg/privatemsg.theme.inc) [function.include-once]: failed to open stream: No such file or directory in /home/pariscin/www/includes/theme.inc on line 593.
warning: include_once() [function.include]: Failed opening 'sites/all/modules/privatemsg/privatemsg.theme.inc' for inclusion (include_path='.:/usr/share/php5') in /home/pariscin/www/includes/theme.inc on line 593.
warning: call_user_func_array() [function.call-user-func-array]: First argument is expected to be a valid callback, 'theme_privatemsg_list_header' was given in /home/pariscin/www/includes/theme.inc on line 597.
user warning: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'LIMIT 0, 25' at line 1 query: SELECT pmi.thread_id, MIN(pm.subject) as subject, MAX(pm.timestamp) as last_updated, MAX(pmi.is_new) as is_new, GROUP_CONCAT(DISTINCT author SEPARATOR ",") as author FROM pm_message pm INNER JOIN pm_index pmi ON pm.mid = pmi.mid WHERE (pmi.uid = 7) AND (pmi.deleted = 0) GROUP BY pmi.thread_id ORDER BY LIMIT 0, 25 in /home/pariscin/www/sites/all/modules/privatemsg/privatemsg.module on line 391.
warning: include_once(sites/all/modules/privatemsg/privatemsg.theme.inc) [function.include-once]: failed to open stream: No such file or directory in /home/pariscin/www/sites/all/modules/privatemsg/privatemsg.module on line 1475.
warning: include_once() [function.include]: Failed opening 'sites/all/modules/privatemsg/privatemsg.theme.inc' for inclusion (include_path='.:/usr/share/php5') in /home/pariscin/www/sites/all/modules/privatemsg/privatemsg.module on line 1475.
warning: include_once(sites/all/modules/privatemsg/privatemsg.theme.inc) [function.include-once]: failed to open stream: No such file or directory in /home/pariscin/www/includes/theme.inc on line 593.
warning: include_once() [function.include]: Failed opening 'sites/all/modules/privatemsg/privatemsg.theme.inc' for inclusion (include_path='.:/usr/share/php5') in /home/pariscin/www/includes/theme.inc on line 593.
warning: call_user_func_array() [function.call-user-func-array]: First argument is expected to be a valid callback, 'theme_privatemsg_list_header' was given in /home/pariscin/www/includes/theme.inc on line 597.
warning: include_once(sites/all/modules/privatemsg/privatemsg.theme.inc) [function.include-once]: failed to open stream: No such file or directory in /home/pariscin/www/includes/theme.inc on line 593.
warning: include_once() [function.include]: Failed opening 'sites/all/modules/privatemsg/privatemsg.theme.inc' for inclusion (include_path='.:/usr/share/php5') in /home/pariscin/www/includes/theme.inc on line 593.
warning: call_user_func_array() [function.call-user-func-array]: First argument is expected to be a valid callback, 'theme_privatemsg_list_header' was given in /home/pariscin/www/includes/theme.inc on line 597.
warning: include_once(sites/all/modules/privatemsg/privatemsg.theme.inc) [function.include-once]: failed to open stream: No such file or directory in /home/pariscin/www/includes/theme.inc on line 593.
warning: include_once() [function.include]: Failed opening 'sites/all/modules/privatemsg/privatemsg.theme.inc' for inclusion (include_path='.:/usr/share/php5') in /home/pariscin/www/includes/theme.inc on line 593.
warning: call_user_func_array() [function.call-user-func-array]: First argument is expected to be a valid callback, 'theme_privatemsg_list_header' was given in /home/pariscin/www/includes/theme.inc on line 597.
warning: include_once(sites/all/modules/privatemsg/privatemsg.theme.inc) [function.include-once]: failed to open stream: No such file or directory in /home/pariscin/www/includes/theme.inc on line 593.
warning: include_once() [function.include]: Failed opening 'sites/all/modules/privatemsg/privatemsg.theme.inc' for inclusion (include_path='.:/usr/share/php5') in /home/pariscin/www/includes/theme.inc on line 593.
warning: call_user_func_array() [function.call-user-func-array]: First argument is expected to be a valid callback, 'theme_privatemsg_list' was given in /home/pariscin/www/includes/theme.inc on line 597.

Check if that file is really missing in the privatemsg folder. It is part of the patch, I can't see why it is missing for you, I assume something went wrong when you applied the patch.

I don't have such file: privatemsg.theme.inc, not with the dev. version, or the rc2 version.

Where is this file? Without using the patch, the module don't need this file.
Maybe you have to attach this file with the patch.

oh ok, soory, i've got it :)

The patch works great for me! :)

Thanks ^^

We have to change:

.privatemsg-list-subject.privatemsg-unread {
font-weight: bold;
}

to

.privatemsg-list .privatemsg-unread {
font-weight: bold;
}

The patch does already change it to:
tr.privatemsg-unread td.privatemsg-list-subject

This is consistent with the old behavior, your change would make the whole row bold, not just the subject, because the privatemsg-unread class is now in the tr-tag.

subscribe, thanx!

Applied the same array_merge() call to change_delete and fixed a minor bug with execute buttons.

No other code changes, only some inline comments for litwol!

I've completed a code review on this patch and posted any problems as issues, using the tag 348907.

There's very little to hold this patch back, the only confusing thing is theme patterns, if we can get those properly documented everything will be fine. There's nothing technically wrong with this patch and it should be committed!

I'm going to write patches for those follow-ups when this one has been commited. Can't really do it now :)

Wonderful patch, opens heaps of possibilities for usability improvements. Thank you everyone involved. Thank you Berdir.

Now we have to concentrate on resolving the followup patches (click on this issue tag)

Automatically closed -- issue fixed for 2 weeks with no activity.