Support from Acquia helps fund testing for Drupal Acquia logo

Comments

merlinofchaos’s picture

I'm not fully entranced by the core drag & drop for tables in D6; I don't like the small drag handles nor do I like that the dragged item doesn't really follow the mouse. This is in part because it simply may not be possible -- table rows aren't super flexible. Most drag & drop is done with lists, but nodequeue is a table and it's pretty hard to do that elegantly.

So I'm not considering this feature to be of very high importance. I'm pretty happy with the javascript click to move buttons.

delykj’s picture

Status: Active » Closed (fixed)

Thanks, Merlin.

wulff’s picture

Version: 5.x-2.0 » 6.x-2.0-rc3
Status: Closed (fixed) » Needs review
FileSize
178 bytes
8.65 KB

For a project we're doing at the moment, I created a module to let users rearrange nodes in nodequeues using the same drag-and-drop interface which they know from the taxonomy admin pages.

Would you be interested in a patch to nodequeue.module with this functionality?

I have attached the .module and .info files for people who want to try this out. Once the module is installed, go to nodequeue_dragdrop/<qid>/<sqid, optional> to try it out.

ezra-g’s picture

I think this general feature should be considered an improvement, since it standardizes the interface and can reduce the size of the nodequeue codebase.

Also, it might make it easier to incorporate the views integration built for weight module (http://drupal.org/node/314248#comment-1068126) which could potentially allow queues to be resorted from inside a View. This would make nodequeue-based photo galleries more of a possibility, and could be an improvement in the photo handling space, imo.

delykj’s picture

A Drupal 5.x compatible version would be great.

merlinofchaos’s picture

When doing the drag & drop sort in Views, I found it difficult to integrate the 'add' button as well as the types of buttons that do shuffles and the like. The drag & drop code isn't very friendly to external modification of the data.

moshe weitzman’s picture

FWIW, I think this would help a lot. Standardized UI is a good thing.

ezra-g’s picture

Having not yet looked into it in detail could this be implemented while maintaining the "move to front/back" of queue buttons? I realize that drag + drop is cooler, but a single click seems like less work for than a drag all the way to a bottom/top of a list. It would be nice to have both types of interaction.

ezra-g’s picture

Status: Needs review » Needs work

I've tested this briefly and I think it's a great addition to the module for the reasons mentioned above and because it seems like a general usability improvement. I would like to see us move towards using this as the default interface. I'm thinking of submitting a poll to see what module users prefer, or at least making announcement that work is being done towards integrating drag + drop.

One problem I noticed is that when you disable javascript, the degraded interface makes it possible to have two nodes with the same queue position (1), and we should prevent that from being saved to the database, probably with some general submit validation on the form.

ezra-g’s picture

The submit validation should probably go in #337834: Prevent bad data from being saved to nodequeue_nodes

ezra-g’s picture

Also, when trying to remove a node I get "warning: array_shift() [function.array-shift]: The argument should be an array in /Users/ezra/Developer/htdocs/dev/contrib/nq6/sites/all/modules/nodequeue/nodequeue.module on line 1760."

This interface does not delete all values for a subqueue when submitting the form -- I'd like all values to be deleted instead of just updating existing values since this could prevent invalid values from being saved.

I'm working on #337834, so that this interface can call a standard queue order submit function to submit the queue order.

ezra-g’s picture

Assigned: Unassigned » ezra-g

Assigning to me and merging #337834 into this. This could also lead us to having multiple queues on one screen and dragging a node between them.

kyle.vh’s picture

I would love to see drag and drop. Mostly because users will start expecting it since they see it at other parts of D6.x.
This module is great.

scottrigby’s picture

Since you're asking for feedback on this...

I hate to disagree with merlinofchaos on anything, but I have to take the other side on this one ;) I feel it would be helpful to standardize the interface as much as possible.

While that wasn't appropriate for an interface like the Views2 cockpit (which successfully allows tons of options in a very small amount of screen "real estate"), I think Nodequeue doesn't require such a customized solution. When possible, I personally feel there would be more continuity for drupal users if the row ordering matched the D6 core menu and taxonomy ajax draggable interface.

* That said, if improvements can be made to the ajax draggable rows (such as the size of the handle merlinofchaos mentioned), these could added as patches to core (for D7), so the interface would still remain consistent between core and other important contributed modules that make use of the same interface.

DamienMcKenna’s picture

As a lowly n00b to contributing to Drupal projects I'd have to concur with scottrigby, standardizing on the built-in functionality of D6 is the most logical step forward, then work with core to improve the core drag'n'drop system for D7 as deemed necessary. While being able to manage them from a View2 would be pretty cool, I think that should be a v3 step, after the initial work is completed and stabilized.

As for backporting to D5, the whole purpose is to use the D6 APIs so those would have to be backported first. I think the effort would be better spent in collaboratively pushing other modules to D6 compatibility first, rather than shoe-horning yet another enhancement onto D5. That's obviously purely my opinion, but with so much of the "core" 3rd party modules now at a stable D6 release, or at the very least beta or RC, all effort should be put towards pushing on D6, including our sites =-)

Damien

SeedTreeLLC’s picture

Great ideas. I like this module. It solves so much for my users. I also think the proposed patch is a great addition. Thank you for your efforts and time. Bump!

ezra-g’s picture

Status: Needs work » Needs review
FileSize
24.09 KB

This is in progress but testing and feedback are welcomed. Marking as CNR for that reason. wulff's work provided a good starting point to build off of.

@merlinofchaos: I'd like to implement this in a way that you feel good about. I preserved the current "remove" icon. Perhaps making the drag handles larger would address your concerns about their being too small. At their current core size, they _are_ larger than the nodequeue ordering buttons.

1) It Is fully degradable: Without Javascript, the remove buttons are hidden and users can use the position selectboxes to mark a node to be removed (uses the new 'r' value in the custom form element)
2) Restriping behavior works
3) I abstracted saving subqueue order from the arrange_queue form to its own api function. This makes it easier for module developers to submit new queue orders programmatically and prevents invalid data such as garbage from custom javascript or zeroes for nid or position from being saved to the database.
This preserves:
- the classic Nodequeue remove icon and behavior
- callbacks for old queue manipulation form for future use by module developers

What's left:
- shuffle works only on submit, meaning it saves the new order before the user can decide if she likes it.
- reverse doesn't work and should also use JS

drewish’s picture

subscribing.

neoliminal’s picture

I think the majority of people expect drag and drop in this module and are surprised when it's not there. If you think it's really a problem (beyond the coding changes) then perhaps allow people to toggle from one to the other. I suspect, however, that drag and drop is likely to be the proffered system for moving items in a queue.

I know my clients are asking for it.

ezra-g’s picture

Status: Needs review » Needs work

I'm marking this as needs work since technically it's not finished.

alanburke’s picture

Subscribe.

Anonymous’s picture

Subscribing. I was surprised the rows weren't draggable for reordering.

litwol’s picture

Version: 6.x-2.0-rc3 » 7.x-2.x-dev
Status: Needs work » Needs review
FileSize
23.72 KB

Patch in #17 works beautifully. Here is a patch that integrates #17 with latest 2-dev branch.

ezra-g’s picture

Status: Needs review » Needs work

Thanks for rerolling, but see my feedback in #17 under "what's left."

dixon_’s picture

subscribing

wulff’s picture

FileSize
39.95 KB

The attached patch adds JS reverse and shuffle support.

It still needs some polishing, but I thought I might as well share the code I have at this point.

@ezra-g would you be interested in a patch the splits the admin functions into a separate .admin.inc file? The current nodequeue.module is getting a little unwieldy.

ezra-g’s picture

Status: Needs work » Needs review

Awesome - Thanks ! Yes, I would be interested in an admin.inc Marking as needs review.

wulff’s picture

Updated patch attached.

With this patch, the reverse, shuffle, and clear buttons work both with and without javascript enabled.

TODO:

  • Make sure that queue operations through the API still work as expected.
  • Polish javascript.
  • Maybe rebuild node positions on node removal to avoid gaps between positions.
litwol’s picture

I'm using earlier version of this patch and there's one important feature that is missing.

While dragging its very easy to lose track of the actual order count. there should be some kind of visual aid or list the actual row numbering when you move them around.

wulff’s picture

FileSize
186.91 KB
166.31 KB

Are you thinking of a simple indication of the current node positions as in the attached screen shots?

litwol’s picture

The behavior i was after is closer to the initial one. whether the queue is in order or is reveresd you would get row count indication from 1 to n or from n to 1. The purpose is to simplify management of longer queues when the order is important (ie. i want some node foo to be 24th in the queue).

even when its "shuffled" the row count in ui would still display 1...n

ezra-g’s picture

@wulff The queue position as indicated in your screenshot would be a great addition.

ezra-g’s picture

Status: Needs review » Needs work
FileSize
53.53 KB

This re-roll is a major step forward. Thanks, wulff!

Here's a minor re-roll as a single patch. I was getting an invalid syntax sql error, which I fixed. The problem was that in nodequeue_save_subqueue_order, the INSERT part of the query was getting added twice. I also added the text, "New subqueue order not saved" to the error messages so it's clear that the subqueue order has not been changed.

@wulff, you mention in the patch that perhaps we should move some of the validation in nodequeue_save_subqueue_order to a validation function. The reason I have this code there is to prevent bad data from getting saved to the nodequeue table when passed not from a form, but from a programmatic call to this function. I think the best solution would be to replicate this validation in the form so that when dealing with forms, the validation happens in the expected place.

However, this validation doesn't really protect against invalid values in the database. Here's some weird behavior and the steps to reproduce. This was done on a clean install.

Start out with 5 nodes in a basic subqueue :

mysql> select * from nodequeue_nodes order by position asc;
+-----+------+------+----------+------------+
| qid | sqid | nid | position | timestamp |
+-----+------+------+----------+------------+
| 1 | 1 | 6 | 1 | 1242837499 |
| 1 | 1 | 5 | 2 | 1242837500 |
| 1 | 1 | 3 | 3 | 1242837502 |
| 1 | 1 | 2 | 4 | 1242837504 |
| 1 | 1 | 1 | 5 | 1242837506 |
+-----+------+------+----------+------------+

Remove node in position 4, with or without js enabled.

New display order in the form (as shown with js disabled):
1, 2, 3, 1 (last one is duplicate #1)

values in database:

+-----+------+------+----------+------------+
| qid | sqid | nid | position | timestamp |
+-----+------+------+----------+------------+
| 1 | 1 | 6 | 1 | 1242837557 |
| 1 | 1 | 5 | 2 | 1242837557 |
| 1 | 1 | 3 | 3 | 1242837557 |
| 1 | 1 | 3 | 3 | 1242837557 |
| 1 | 1 | 1 | 5 | 1242837557 |
+-----+------+------+----------+------------+

Duplicate nid 3 in in position 3.

We should prevent these duplicate values from being saved.

To review, the remaining to-do items as I see it:

- Prevent duplicate or otherwise invalid values from being saved to the database
- Show the node's position as in wulff's screenshots (@wulff, did you code that already?)
- Add form validation

nitrospectide’s picture

subscribing

wulff’s picture

I'll try and work through the remaining todos early next week. This weekend is DrupalCamp Copenhagen, so I probably won't have time to do any work.

I have the node position stuff in a patch somewhere, I'll be sure to add that.

neoliminal’s picture

subscribing

DamienMcKenna’s picture

wulff: any luck trying out the patch?

wulff’s picture

I have updated the patch for the latest version of nodequeue.module (1.84).

The javascript side of things is at a point where it is actually usable. Also, it now has better integration with tabledrag.js to avoid duplicate warning messages, etc.

I have added a node position column at the far right of the nodequeue table. I have called this column 'Pos', but would like suggestions for the name (does it even need one?).

I'm working on form validation and duplicate/invalid values in the db this week.

wulff’s picture

The problem with duplicate/invalid values has been solved. It was caused by an array that didn't get reset in nodequeue_save_subqueue_order().

I'll upload a patch once I've added the form validation stuff and done some general cleanup.

wulff’s picture

FileSize
70.66 KB
69.38 KB

I've attached a couple of screenshots showing the subqueue table without javascript enabled, and the improved error handling.

We still have the "Undefined offset: 5" error, but I was wondering if we could get rid of that by always linking to 'admin/content/nodequeue/<qid>/view/<sqid>' instead of 'admin/content/nodequeue/<qid>'?

wulff’s picture

FileSize
105.9 KB

I've attached the latest version of my patch. There are still a couple of kinks to iron out, most notably related to node positions after one or more nodes have been removed from a queue.

daniboy’s picture

subscribing

mattiasj’s picture

this would be excellent, subscribing.

loze’s picture

subscribing

wulff’s picture

Status: Needs work » Needs review
FileSize
107.14 KB

I've attached the latest version of my patch (against the current HEAD version).

This version fixes the problem with node positions after removing nodes from a queue.

ezra-g’s picture

Status: Needs review » Needs work

Wow -- This looks very good to me. In my testing both with and without JS, I wasn't able to break it! Thanks for this great work. Functionally we seem to be there. Now, we just need a bit of cleanup.

About the "FIXME"s

On line 1795 : +// FIXME: WULFF: re-add this to the nodequeue form

Is there more work that needs to be done there?

Regarding the other FIXME's, yes, we can remove nodequeue_arrange_subqueue_entry, though we should also remove the code that calls that function. The old remove node menu callbacks can be removed as well.

I noticed you made some coding style changes to various parts of the module. Some of them improve coding standards compliance, and some of them, such as making all the keys and values of an array line up, seem to violate the coding standards (though they do make things easier to read, IMO).

In any case, they bring the patch length to ~2,700 lines, slightly longer than the entire module itself. Since this feature requires such a significant change to the code, I'd prefer to commit/review it separately so that we have a clear record of the changes for this feature.

I realize you've done a ton of work on this issue so let me know if that seems possible. I could look at some of the fixme's.

wulff’s picture

Status: Needs work » Needs review
FileSize
51.86 KB

I have to agree that the patch is getting a little unwieldy. I have removed all the cosmetic changes which brings it down to a more managable 1537 lines.

This updated patch gets rid of the remaining FIXMEs and cleans up the textfield below the drag'n'drop table a bit.

The "re-add this to the nodequeue form" FIXME was related to an AJAXy way of adding nodes to the drag'n'drop table. I don't think it's necessary, so I've removed it for the time being to focus on the drag and drop functionality. We can think about adding it if we feel saucy at a later time.

I'll save the code style changes for another patch. I was thinking of incorporating them in the 'split admin pages to a separate .inc'-patch I've been talking about. That patch will mostly be moving stuff around, so cosmetic changes should be easier to incorporate there.

ezra-g’s picture

FileSize
52.22 KB

Thanks for the re-roll :).

Here's another one with a few changes:

- Hides the node author, post date fields when a node is restricted from a user.
- Changes the '#' column header to 'Position'
- Changes 'Date' header to 'Post Date'

Since the "Add" button is the only one that saves the subqueue immediately, I changed the text to "Add node and save subqueue".

This seems basically RTBC, but I'm throwing it out for one last review.

I'll turn to #512086: Can't add nodes using autocomplete textbox, titles display as restricted when manipulating queue for own nodes and once this patch has landed.

RobLoach’s picture

Priority: Normal » Critical
Status: Needs review » Reviewed & tested by the community

Very nicely done, Ezra!

  1. Checked out NodeQueue HEAD
  2. Added a Node Queue
  3. Edited and saw the drag and drop interface
  4. Moved around the elements
  5. Saved the nodequeue and saw the nodequeue was now arranged with the saved order
  6. Clapped with excitement
  7. Disabled JavaScript
  8. Refreshed and saw the position form with the "Remove" option.
  9. Chose the Remove option for an element
  10. Saved and saw the node wasn't there anymore
  11. Chose a different position for an element and clicked save
  12. It complained that I had a duplicate position
  13. I fixed the positions and clicked save
  14. It showed the updated order and I clapped again and then got coffee
  15. I drank coffee and set this to RTBC

Get this in so we can open new issues that don't depend on this patch!

ezra-g’s picture

Status: Reviewed & tested by the community » Fixed

Thanks wulff for all of the awesome work! Thanks Rob Loach for the multi-step review!

This is committed.

Status: Fixed » Closed (fixed)

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

jenlampton’s picture

*edit* whoopsie.