Add drag and drop sorting for list display fields

Looks great, always wanted to do this too but never did :)

Maybe we even have an old issue about this open.

Will look into this soon.

My suggestion would be to introduce a simple hook for adding new fields instead of using hook_form_alter(). That hook would allow modules to return array of field, each consisting of a title/label, default value, and locked flag (can't be changed in the UI). Maybe more.

I'd suggest to do that in the D7 info style. Meaning, a hook_privatemsg_fields_info() and a hook_privatemsg_fields_alter($fields). Both inside a simple API functions that first collects the fields, then calls the alter hook and then returns it. Maybe with an option to either return all or only enabled fields (all for admin UI, only enabled everywhere else where the variable is currently used).

Oh, and please always set issues to needs review when you have a patch, then the testbot can run it.

The reason you are not seeing these errors is that E_NOTICES are hidden by default in Drupal 6, but they are enabled on the testbot. Looks like you're messing around with the form a bit too much :)

Also, 6.x-2.x is the current development branch, API changes are fine there. In contrast, the current 7.x version, 7.x-1.x, is stable and no API/string changes are possible. 7.x-2.x *will* of course be opened, but this has not happened yet.

I doubt any other module is doing the hook_form_alter trick except privatemsg_filter and privatemsg_attachments, which are part of this project so they can be patches at the same time.

+++ privatemsg.admin.inc	11 Jan 2011 18:57:22 -0000
@@ -130,7 +130,8 @@
+    '#process' => array('privatemsg_process_display_fields'),

As discussed, when changing to a registration hook, we can do the processing directly in the form function and don't need #process. That should make the whole thing a bit simpler.

+++ privatemsg.module	11 Jan 2011 18:57:22 -0000
@@ -2325,6 +2331,21 @@
+  $column_weights = variable_get('privatemsg_display_fields_weights', array(
+    'subject' => -20,
+    'participants' => -15,
+    'thread_started' => -10,
+    'count' => -5,
+    'last_updated' => 20,
+  ));

I guess that we want to add the default weight to the registration hook too. Then privatemsg can register the default fields by implementing the hook itself.

Overall, this looks very good.

Powered by Dreditor.

Instead of Locked, why not simply 'required' as that is what you used yourself to explain it :) It's not like locking it to being disabled woud make much sense imho :)

Additionally, we need an "Enabled", probably default to false.

More thinking, since you mentioned the theme callback stuff, we could go as far as replacing the header theme stuff completely with this hook. It's not actually a theme function anyway because it doesn't return HTML. The only drawback would be that themes can't easily override it. And for the Drupal 7 version, they could, because they can implement alter hooks. On the plus side, this would be a performance improvement.

If we do that, we also need to add keys for everything that those theme functions can return, including sorting column, sorting direction and so on.

Just noticed that hook_node_info() uses 'locked', so it might be a good idea to use that too.

See http://api.drupal.org/api/drupal/modules--node--node.api.php/function/ho...

This needs work :)

I might give it a try myself when I find the time. I think you already did the heavy lifting with your form mangling. Defining an info/info_alter() hook including helper functions is quite a repetitive task that I've done a few times already.

Ok :)

Here is a patch, haven't checked that the tests still work but manual testing looks good.

- Replaces the header templates with a info hook
- Integrate the dnd directly into the form, requires much less code.
- The info hook basically uses the same arguments as before, aka those who are required for table headers/sorting. Additionally, the following are defined at the moment: #title (defaults to data), #weight (defaults to 0), #enabled (enable by default, defaults to FALSE), #locked (disables the enabled checkbox, does not allow to change it).

This has a few additional advantages that we can build upon. One example is that we could make the default sorting configurable, although I'm not sure if that makes much sense. Also, the displayed columns are not based on the select SQL columns anymore, so we could technically change it that subject and last_updated are required, but again, useful?

There is also an issue open about implementing the same dnd/enabled handling for the listing types (inbox, sent, all). That would allow to make it easier to define more of those, for example Trash or expose specific tags as separate tabs.

There is also a duplicate issue about this laying around, will close it when I stumble upon it ;)

Forgot to add a default value for #locked. Also missed one property, that is #access, which works just like it does for form elements and it can be used to hide the column based on the permissions of the current user (only show tags columns if he can see them)

Subscribing to help with testing (per Berdir's request). Will report back soon...

Tested the patch in #19... it's working great. I've been enabling, disabling, and re-ordering with ease. No problems at all.

One small change could be renaming the "Enable" column header as "Display?" (with a question mark included). But that's not essential.

So as far as I'm concerned, this is RTBC. :-)

--Ben

Ok, here is a new patch. No visual changes...

- Updated documentation to be correct again. Including hook and theming documentation.
- Actually using the enabled headers to theme each row.
- Headers can now override the default theme, it defaults but is not hardcoded to the key anymore.

Just tested the latest patch. It's working well except for one thing:

If you uncheck the "Participants" field and click save, when you reload the page Participants will be checked. Because of this it is not possible to remove the Participants column from the message list page. Either that checkbox should work properly or else it should be grayed out.

--Ben

New patch that should fix Participants thing.

Forgot some debug statements.

Re-uploading patch to trigger testbot.

Commited to 6.x-2.x, will need to be ported once we have 7.x-2.x

Ported the patch to 7.x-2.x. All tests should pass, please test.

Committed

Powered by Dreditor (triage sandbox) and Triage transitions