While reviewing PR #33909, we found potential issue with items re-ordering using drag and drop in Joomla 4. Main issue are:
And to further thicken the plot: in J3 ordering is using negative numbers, so e.g. when you set field one below field 2 then field one gets ordering of 1 and field 2 gets ordering of -1.
It should also be tested how ordering of fields will handle these when upgrading to 4.0 and then reordering n 4. Will it correctly handle the negative numbers etc. as the logic of ordering has apparently changed.
Labels |
Added:
?
|
Yes, I think only client part was changed, so should not be hard to fix :). As mentioned, page 2, page 3 restart item ordering from 1 and it causes the issue. I guess instead of re-start with 1, it should be started with min ordering of the items on that page and it should be fine.
Labels |
Added:
?
|
Set release blocker label as discussed with other maintainers.
okay, so it is JS, in J3 the order is recalculated before submit in
joomla-cms/media/jui/js/sortablelist.js
Lines 215 to 255 in 4172b68
in J4 it always start fro 1
That's right. And keep in might that in J3, we do not change ordering of all items like we are doing in J4. Change ordering of all items could cause performance issue during saving process (note that we also have row in same group, and ordering value of row from different groups should be be changed)
It changes the order for the active page, if I right understood.
But need to check how it doing the calculation, it not that easy to get what there is written
Yes, but as I understand, it does not change ordering value of every items on the page. Only certain items (especially in case there are items from different groups - for example fields from different fields group)
But need to check how it doing the calculation, it not that easy to get what there is written
Yes, not easy to understand. I will said that it is JS code and we will ask you @Fedik and @dgrammatiko for this difficult task, lol
I think it's a miracle that this thing even works for some cases...
I will revisit the code once I'm feeling a bit better (kinda sick the last few days, so the brain is not there 100%) unless @Fedik is faster. The idea is simple you have an array of all the rows before the d&d and another after and you diff the changes
as a reminder for testing / debugging: there are two scenarios: first sort (where all fields get an order set because all fields order = 0) and second order (where all fields already have an order and only changed fields get an order set)
When all fields have an order set, then ordering on a sub page should work correct, the issue here is that when no ordering is set and thus the ordering on the other pages is set to 0.
This could be solved by NOT setting the default sort order to 0 but to e.g. the primary key (server side)
In theory that is :)
Actually, not that simple if you look at the script which calculate the order value of each record :) which @Fedik mentioned earlier #33917 (comment)
In theory:
As I understand, it is complicated like that for performance issue (think about change ordering of items from a table with 100K records as you mentioned). The process not only update ordering value of the records but also recalculate ordering value for other related records (for example, from same category) https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/MVC/Model/AdminModel.php#L1548-L1625
Only the record of the dragging element and the records between it's original position and it's dragend position should have the ordering value update
Ok, so this is the difference, in J3 the reordering is basically swapping the dragged element with the one in the drop position but with the current script the dragged element is inserted before the drop position (ie real reordering, not swapping)
If we want to keep the swapping behaviour it should be straightforward:
// in the `drag` event get the original array of rows and the position of the dragged element
let rows = [];
let draggedElementIndex = 5;
// A simple swap function
function swap(input, index_A, index_B) {
let temp = input[index_A];
input[index_A] = input[index_B];
input[index_B] = temp;
}
// On drop get the drop element position
let droppedElementIndex = 5;
// Do the swap
swap(rows, droppedElementIndex, draggedElementIndex);
//Apply the changes to the DOM
// Do the ajax...
Guess I don't understand JS code or it is not that simple :(. Let's say we are on the page with 20 records ordered in ASC order and you want to move a record from position 2 to position 5 (so basically, the record is moved down). Here is how I understand the ordering value should be changed:
If the item is moved up, the logic would be different. That's how I see it works in J3. We have to care about the case items are ordered in DESC order, too. That's why I said we should not change ordering value of all records on the current page.
The rearrange code in J3 is
joomla-cms/media/jui/js/sortablelist.js
Lines 204 to 256 in f1f10a2
We could just copy this over (I don't remember why I skipped that and rewrote the logic)
* There records from position 6 to 20 will have it ordering value un-changed.
True only on second+ ordering, on initial order these all have vaue 0 and will break the order
That's why I said we should not change ordering value of all records on the current page.
you have to.
example on first page:
1
2
3
4
5
You move 1 to the end:
2
3
4
5
1
So all previous items need to be shift:
2 = 1
3 = 2
4 = 3
5 = 4
1 = 5 actual index
In the resulting DIFF an every item will be changed.
Then on second page:
6
7
8
9
10
You move 9 up:
6
9
7
8
10
You again recalculate all:
6 = 6
9 = 7
7 = 8
8 = 9
10 = 10
Here in the diff will be changed only 3 element.
That basically what @dgrammatiko wrote.
Of course we have to skip non related groups, but the idea is the same.
The thing getting more fun when you have a filter enabled.
So example your first page become:
1
3
4
7
10
And you move 1 item to the end:
3
4
7
10
1
You recalculate all:
3 = 1
4 = 2
7 = 3
10 = 4
1 = 5
All element changes.
You disable filter and you have totaly different ordering with doubled elements:
1
1
2
2
3
3
4
4
....
~But joomla 3 also ignores this case I guess ~
Hm not really, joomla 3 seems just doing +1/-1 depend from direction.
That's why I said we should not change ordering value of all records on the current page
More correct would be: we change ordering only for "changed elements". Because amount of changed elements may be equal to whole page ;)
Somehow, we would have to find the items between the original position of the element and it's dropped position and apply the ordering change (+1, -1 depends on move up, move down and how items are sorted in ASC or DESC order)
The thing getting more fun when you have a filter enabled.
That is why (imo) the initial ordering should be set to the primary key AND the new order should not be 'recalculated' (read: new values) client side, but should be based on the existing order ids. That way, sub pages and filtering have no negative impact
what is shown (subpage or filter) is pushed as an array to the server, new order is done on INDEX (of that array) and not on values. That way the values will not get doubled / 'corrupted'
the initial ordering should be set to the primary key
It will not help much, you still get doubled items, after reorder of filtered data.
Filtered items where Order is set from primary key:
11
14
15
32
You move 11 to end:
14
15
32
11
Recalculate:
14
15
32
11 = 33
In the end you have 2 (or more) items at position 33.
Additionally it may be harder to do calculation
Somehow, we would have to find the items between the original position of the element and it's dropped position and apply the ordering change (+1, -1 depends on move up, move down and how items are sorted in ASC or DESC order)
The data exists there, you have the original array of the elements on drag start and the final one on drag end. So basically it's all about array diffing and then a forEach to apply the changes
subpage and filter is a different things, you always will get doubled row for "filtered", unless you will not apply more complex algorithm, to resort whole data in database.
But that is not big problem, I guess. More important that sorting works correctly "per page".
subpage and filter is a different things, you always will get doubled row for "filtered", unless you will not apply more complex algorithm, to resort whole data in database.
But that is not big problem, I guess. More important that sorting works correctly "per page".
No, you will only get double rows if you assign NEW values to sort order, that is why I propose to use index as pointers to existing values as source for order instead of new values.
I'm thinking that we might have to find index of drag position (should be easy) and then index of the dropped position
Actually Draggula is very helpful here as each event ('drag', 'drop') provide us the element, eg:
.on('drag', (draggedElement) => {
// Get the index of draggedElement from the rows array
})
.on('drop', (dropElement) => {
// Get the index of dropElement from the (new? Idk if the event is fired before or after the DOM update) rows array
// Diff the arrays
// Do the ajax
})
I don't get how array diff would help in this case.
Check my example for Second Page with 3 changed elements.
before [6, 7, 8, 9, 10]
after [6, 9, 7, 8, 10]
diff [9, 7, 8]
which will be re-indexed to [7,8,9]
and send
@Fedik I think he means instead of using 0 indexed array to have an array of the actual id numbers
What we have
// First element
<input type="text" name="order[]" size="5" value="" class="width-20 text-area-order hidden" data-order="1">
Proposed:
// First element's id = 100
<input type="text" name="order[]" size="5" value="" class="width-20 text-area-order hidden" data-order="100">
correct :) that way you will never have to create a new value with the risk of double etc.
Only issue then left is the ordering value of 0 which in J3 is worked around by having negative ordering values (e.g. -1).
As said that could be 'fixed' by initially setting the order value not to 0 but to the primary key value. Not sure what that would mean for upgrading from 3.x though
Not sure what that would mean for upgrading from 3.x though
This attribute is created client-side (js) so it shouldn't break anything, but getting the ID number might be tricky?
Edit:
if there is a check input then it's easy, just get the value from there: <input class="form-check-input" autocomplete="off" type="checkbox" id="cb0" name="cid[]" value="5" onclick="Joomla.isChecked(this.checked);">
that way you will never have to create a new value with the risk of double etc.
Check my example above #33917 (comment)
Where element 11 moved at the end, and changed to 33,
however on next page may already exist element with order 33 (which set from ID).
So you get 2 element at position 33.
That not should happen: order value is set on item creation, so when item with ID is created there cannot be another item that has order value of 33, unless you upgrade from 3.x where order values could be anything...
because it is based on the (primary) key, even deleting e.g. 33 will never use 33 again
Not sure what that would mean for upgrading from 3.x though
This attribute is created client-side (js) so it shouldn't break anything, but getting the ID number might be tricky?
Edit:
if there is a check input then it's easy, just get the value from there:<input class="form-check-input" autocomplete="off" type="checkbox" id="cb0" name="cid[]" value="5" onclick="Joomla.isChecked(this.checked);">
or just echo it in server side?
I think for now, the priority should be making client side works in the same way with Joomla 3. Please note that after the ordering is saved, the server side also have logic to re-calculate ordering of the related items , see https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/MVC/Model/AdminModel.php#L1615-L1619
So the issue you are discussing about might be invalid. Or even it is valid, it should be discussed in separate issue. For now, just please find a way to make it works in the same way with Joomla 3.
or just echo it in server side?
Yes, that's easier but also a B/C break. We can have some logic there like, get data-id
if exists if not check for the checkbox input in the same row and disable d&d if it doesn't;t exist. So no B/C break
so when item with ID is created there cannot be another item that has order value of 33
okay, and how then ordering should work?
you see that after moving/sorting the element around this number will changes (many times back and forth), and I already show how you will get a double.
@dgrammatiko ignore id, they will be picked from row cid[]
field, we just have to set correct order[]
I would say +1/-1.
Setting ID as default may help avoid negative numbers, but this changes behavior, where each new Item will be at end of the list. While current behavior is when each new Item is at top of the list. Or I am wrong?
In the end a negative number still a number ;)
Status | New | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2021-05-18 09:27:49 |
Closed_By | ⇒ | joomdonation | |
Labels |
Added:
?
Removed: ? |
Labels |
Removed:
?
|
hm, it seems PHP part for saveOrder is not changed,
on quick look it the same in J3 and J4 for
AdminController::saveOrderAjax()
,AdminController::saveorder()
,AdminModel::saveorder()