Keep embedded elements under compatible containers in container UI

This would also happen when trying to delete an element.

Corrected grammatical error in title.

Here's the patch.

Hmm... it's not happening for me. Weird. EDIT: I've tried it with both the testing and standard profiles.

I presume you've applied the patch against Rules 7.x-2.1+7-dev?

Ideas on how to go about debugging that issue?

Ooh, right, sorry I got caught up in the problem.

I'll have a look at it when I get to work.

EDIT: Actually, I already have the answer. The tabledrag is built simply by specifying whether an element is leaf. There is really no robust way to dynamically configure which row can be nested under which other, specifically because Javascript won't care about the PHP class hierarchy.

However, there may be a way to get around the problem. I think the trick is for each type of embeddable plugin (UI) to declare its "classes" and possible parent "classes" in CSS form for tabledrag to understand. Then, theme_rules_elements() could add the extra relationships with drupal_add_tabledrag(). The existing plugins and default UI implementations could then reproduce the current effects, plus allowing for plugins like conditionals and branches to specify custom classes so as to restrict the "draggability" to particular types of sub-containers.

The patch above still needs fixing, though, just to ensure it works in the backend. EDIT: Or, the "delete" link could be removed for container plugins whose children can't be embedded in the new parent.

To follow up on #3, I am working on a patch based on the idea behind #7. I've posted a WIP patch.

In essence, this has become a UX issue.

Before I post my patch, could you check if an element can somehow be dragged below the operations row?

This almost seems like a bug in core. Perhaps it has already been fixed.

Turns out to be much more complicated than I first thought. I had to write a behavior to override tabledrag.js and prevent drag if the target container is incompatible.

Also, this patch adds two methods to RulesPluginUIInterface.

I'm uploading this before I work on a less radical, but sort of shoddy idea.

EDIT: Please see #13 for a successful upload.

Sorry about the commented "console.log()" lines littered in the js. Here's a clean one.

EDIT: Please skip this comment.

Okay, last try. This time I've searched for "console.log()".

EDIT: This is insane. Why isn't the the js file included?

Again, please ignore this one.

Finally got it right.

As promised, here is the alternative approach. The differences here (compared to #13) include:

• Each constraint is for any matched element to any matched container.
• Compatibility check leverages plugin info 'embeddable' class name.
• Compatibility between elements is computed beforehand. This is somewhat optimized by grouping the computation by class name (which should reduce most of the computing load for actions).
• No change to RulesPluginUIInterface is necessary.

Basically, this approach might become expensive if there are lots of plugins with lots of different types added to a rule (i.e. minimal intersection and large number of checks). However, once we get into that scenario, this probably wouldn't be the most expensive operation.

I forgot in #14 to dispose of row wrapper object onDrop() (in Javascript).

Fix to #15 to remove the 'delete' link if child elements are not compatible with the parent of the element to be deleted.

I am compiling a testing suite now.

I am unable to reproduce the last issue you mentioned, i.e. moving an action beneath a conditional when coming from the bottom. Though it seems unlikely and rather silly for me to ask, are you sure it wasn't just the "if" statement above the action wrapping to the next line, making the action seem like it's directly under the conditional?

EDIT: Ooh, I've just found it. Will include in test suite.

Before I do anything else, I just noticed I was using class to optimize when I should be using plugin name.

Updated patch.

As an aside, one would almost think the actions were in a <tfoot> (but then, of course, #806982: Tables should take an optional footer variable and produce <tfoot> hasn't been resolved).

The following is a table of tests. Note that "Component" refers to the Rules component to test, imported from the uploaded file.

Each test is meant to be performed after refreshing.

Change log

• Updated 9 to clarify the intention of preventing embedding in incompatible container from below.
• Replaced "Else if" with "(Else) If" for the new labelling in Conditional Rules.

At present, most of the above tests are failing spectacularly (hence they are there, though some are quite funny).

Of course, if the table should include any more tests, let me know and I'll add it to the table at the end.

It seems like a row needs to be further indented while swapping to fit the necessary container.

Also, in the check for downward move, I've neglected to check for a tree group.

I'll fix up the patch in the morning and see how many tests the fix will affect.

Added actual proposal.

I just realized that I was overriding the wrong method in tableDrag.row for preventing swap. I have now correctly overridden swap() itself to swap only if a move is legal.

On another note, the tabledrag.js in Drupal works just fine. It's supposed to be swappable with non-draggable rows, in order to account for grouped rows. So, isValidSwap() is still overridden, but now with an additional check for the "rules-elements-add" class (i.e. operations row).

A tiny change to line 152 of rules.tabledrag.js:

-      var next = $(row).next('tr');
+      var next = $(row).next('tr.draggable');

So now a row can at least be dragged down to just above the operations row. This hopefully eases testing a bit.

Uploaded another WIP patch.

Cross-referencing a possible cause of #3:

#1671344: Setting parent of plugin to incompatible container removes its current parent

I've rewritten the Javascript to use a cleaner approach. This patch (based on #24) only minimally hijacks the core tabledrag.js behavior.

Summary of overrides:

• Treats tr.rules-elements-add as an invalid swap in tableDrag.row.isValidSwap().
• Trims the indent interval from tableDrag.row.validIndentInterval() to further remove incompatible containers from interval boundaries. When the interval becomes invalid (i.e. interval has negative width), the original tableDrag.findDropTargetRow() will treat the swap as invalid and abort.
• Indents a row to be embedded in the nearest compatible container when indenting a row with non-empty interval in tableDrag.row.indent().
• Change detection in overrides for the following to optimize (expensive) compatibility lookups:
  • tableDrag.findDropTargetRow()
  • tableDrag.row.validIndentInterval()
  • tableDrag.row.indent()

All of the tests in #21 pass. This is the first patch that works reliably.

Ach! Comment error on line 57 of rules.tabledrag.js. Instead of saying:

   * Overrides tableDrag.row.findDropTargetRow() to track row change.

it should say:

   * Overrides tableDrag.findDropTargetRow() to track row change.

Will re-roll patch once the above has been tested.

Quick experiment for usability. This change highlights embeddable containers when the user drags a row (i.e. presses the holds the primary mouse button).

Note that for some reason I had to go through #1671344-11: Setting parent of plugin to incompatible container removes its current parent so that PHP doesn't choke on recursive dependency.

One more go at usability enhancement. This time, the active row does not have the green highlight, and the group being dragged (i.e. children) have only pale highlights to show that although those are compatible containers they are to be ignored since a row cannot be moved into its children.

Thanks!

Some of my original tests don't make sense now that I've removed many of the original hacks.

5. Yes, it's the same as 4, just the other direction for completeness.
6. This is to ensure "If" isn't embedded in "(Else) If".
11. What originally happened was the loop outdented against the left (no indent). I suspect it had such a different treatment between swapping and indenting rows that the swap failed because it incorrectly detected an invalid swap, and yet the indent treated it as valid and removed the indent on the old row, orphaning "Add a variable".

So, what I've basically done is left Drupal.tableDrag to do what it was supposed to (e.g. prevent orphaning elements, move entire groups), and then added a compatibility matrix between rows.

This new patch disables the "delete" link, instead of removing it. The disabled link displays a tooltip that explains why the element cannot be deleted.

I have released Conditional Rules 7.x-1.0-beta1. Everything is working as expected.

Hopefully this makes it easier to test the above patch.

mitchell: Would you be able to RTBC #34?

Okay, found a bug in #34: non-recursive container UI also disables the "delete" link if the child container element is not empty.

For example, a rule containing actions inside a rule set cannot be deleted with #34.

It seems that whether deleting an element involves deleting the children is a consequence of the default argument value of the RulesContainerPlugin::delete(). Specifically, the default container plugin elects to keep children if possible, whereas Rule::delete() elects to delete children by default.

Obviously, the default argument value is what we're dealing with, because rules_ui_delete_element() invokes delete() with no arguments. So, the various container plugin UI's will have to account for the default behaviors of their respective plugins' deletion process.

fago: do you agree with this approach? I will post another patch to demonstrate what I'm talking about.

I've cleaned up #34 by moving all the fancy tabledrag stuff to its own method in RulesContainerPluginUI.

I've also tentatively changed the signature of RulesContainerPluginUI::operations() to add an optional argument for specifying $options for constructing operations links. Specifically, it checks 'delete_children' to determine whether to bypass disabling the delete link. Meanwhile, RulesRuleUI indicates 'delete_children' as TRUE.

Okay this wouldn't work either. Components declared as actions rely on RulesPluginUIInterface::operations() to modify links. The extra argument basically changed the function signature such that rules_element_invoke_component_operations() no longer gets the right arguments.

Slight compromise: skip RulesContainerPluginUI in RulesRuleUI::operations() by directly invoking RulesPluginUI::operations() since it is only supposed to show "edit" and "delete" anyway.

Well, I've tried that initially (see #2), and the difficulty comes in sizeable rules (15+ actions). When just catching exceptions, some parents are correctly set, while incompatible ones haven't, so the resulting error wouldn't actually help the user that much in the error just made because the re-ordering simply doesn't work consistently, and there are usually few clues as to what's been reset and what hasn't.

This is also inconsistent with the usual integrity check UX because with integrity errors the element is in the right place, i.e. it's as designed in Rules Core, but it wouldn't correctly evaluate, i.e. Rules Engine won't like it. When trying to place an element under an incompatible container, Rules Core will panic. Catching the exception only removes fatal errors, which only happens because it wasn't even supposed to have happened.

Calling setParent() on an incompatible container throws an evaluation exception. Not sure if that was the intended effect, but it suggests it's a more fundamental problem than an integrity error. Integrity errors tell the user that something was misconfigured, e.g. settings, variable names, etc. Evaluation exceptions thrown when placing an element under an incompatible container indicate broken states (contrary to original design). For example, with Conditional Rules, you wouldn't want to have an "If" under a "Switch".

Plus, much of the JS code exists only because the drag validity checks in tabledrag.js don't quite work the way I thought they would (e.g. it'd be nice if it had some basic targeting checks for parent and sibling classes). Not that I'm hacking, just extending it for Rules.

Okay, I now realize that, on the topic of deleting an element, I've been a massive idiot.

I've updated Conditional Rules to follow the example in Rule and delete all elements under a container whose parent cannot contain the children of the container to be deleted.

I've removed any alteration to the "delete" link and re-rolled the patch against 7.x-2.x.

EDIT: Patch (didn't get uploaded and) is in the next comment.

> couldn't you just solve it from your module and replace the js files loaded by Rules with your own ones?

That's a good point. I'll try doing that instead.

> Maybe it would be better to throw a different exception

I think the issue is more how the UI should react to such exceptions, but perhaps that's a separate issue.

If you agree, we can change this issue to Conditional Rules and create the setParent() issue for separate follow-up.