modxcms / revolution

MODX Revolution - Content Management Framework
https://modx.com/
GNU General Public License v2.0
1.36k stars 529 forks source link

When duplicating a resource, the menuindex should be incremented instead of duplicated. #13093

Closed muzzwood closed 7 years ago

muzzwood commented 8 years ago

Summary

When duplicating a resource via the manager, the menuindex is also duplicated. This causes irregularities if sorting by menuindex.

Step to reproduce

Duplicate a resource and examine the menuindex.

Observed behavior

There were two resources with the same menuindex.

Expected behavior

When duplicating a resource, the new resource should have it's menuindex set one higher than the currently highest menuindex under that parent resource.

What does everyone think?

pixelchutes commented 8 years ago

What should happen when you're duplicating a container containing many child Resources (perhaps themselves a container with more children), etc.? For this, I personally like how the menu index gets mirrored 1:1 and is not incremented.

I could see the value of increasing the index when duplicating in a few scenarios, however, I've always looked at it like cloning, yielding an exact copy.

Jako commented 8 years ago

Maybe a system setting should be available for this.

If it is enabled: all existing resources with a higher menuindex than the duplicated resource in that container should get a +1 incremented menuindex including the duplicated resource. That way the same order is maintained and no duplicate menuindex could occur (if there was no duplicate menuindex before).

Child resources should have no issues during duplicating. Only setting a new parent (by drag&drop or by editing) has to be regarded to avoid duplicate menuindexes.

muzzwood commented 8 years ago

Yup like @jako said, if duplicating a container it would just be the container's menuindex that is incremented. There's no problem with the children keeping their current value.

A system setting could be good although I'd definitely side with making it the default behaviour ;)

muzzwood commented 8 years ago

Ha! I just went into modResource to see how I would implement it and noticed it already checks against a system setting called preserve_menuindex .... It appears this conversation has been had before!

Rather than immediately close this issue however, I still think incrementing the menuindex should be the default behaviour. (i.e. set preserve_menuindex to false as default) Are there any reasons why it shouldn't be?

Jako commented 8 years ago

Maybe in 3.0.0 because of breaking BC.

pixelchutes commented 8 years ago

For some detail, reference: #12905

Jako commented 8 years ago

in #12905 and according to the code https://github.com/modxcms/revolution/blob/8c34e7336ed05f71976e54bb24fe9488c49ab1fb/core/model/modx/modresource.class.php#L1032-L1037, the duplicated resource is appended at the end of the folder (if the menuindex of the other resources in that folder is set right - no duplicates and no holes).

Maybe it is better to sort in the duplicated resource at the same place just after the original (see https://github.com/modxcms/revolution/issues/13093#issuecomment-241572326).

SnowCreative commented 7 years ago

It's not just a problem with duplicate menu indexes. I've also experienced dropouts when shifting things around in the tree, so that for instance you get an order like 0 1 3 4 5 8 instead of 0 1 2 3 4 5 after reordering. This causes problems for instance in cases where a script is creating several different menus, basing the break points on the actual menu index numbers.

Mark-H commented 7 years ago

Closed as the original issue was fixed in #12905. If there continue to be issues, please open a new issue. Thanks!

gpsietzema commented 6 years ago

@Mark-H @digitalpenguin To me it seems that #12905 actually created this issue.

The current behaviour is like this:

ID Page Menuindex
[1] Page 0
[2] Page 1
[3] Page 2
[4] Duplicate of [2] 1
[5] New page 4

So [4] gets the menuindex of [2], but it should have menuindex 3. Or is it just me?

SnowCreative commented 6 years ago

Do you have "preserve_menuindex" set to "no" in your system settings? If not, the menuindex gets duplicated along with everything else.

gpsietzema commented 6 years ago

Ah, thanks for clearing that up. That fixes it. Will also share it with our team, thanks!