Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add buttons to reorder inspector array items without dragging #80617

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

jmb462
Copy link
Contributor

@jmb462 jmb462 commented Aug 14, 2023

Closes godotengine/godot-proposals#7495
Fix #80524

Issue fix

As @markopolojorgensen explains in issue #80524, array items in editor inspector can be reordered by dragging with mouse. But if the number of item is above 5, a pagination system is used. As a consequence, there is no way to move an item from a page to another via editor.

Fix proposal / feature enhancement

This PR implements the proposal godotengine/godot-proposals#7495 to add two buttons in order to move an item one step up or down.

If down button is used on an item at the bottom of a page, it will send it at the top of the following page.
If top button is used on an item at the top of a page, it will send it at the bottom of the previous page.

It will allow to use drag move inside a page and button move inside and across page.

The up and down button system is already used in some part of the editor (eg. autoload panel to reoreder autoload script/scenes) so it seems to be consistent to add one here.

Preview

move_array

editor/editor_inspector.h Outdated Show resolved Hide resolved
editor/editor_inspector.h Outdated Show resolved Hide resolved
@jmb462 jmb462 force-pushed the fix_80524 branch 2 times, most recently from 13450cd to be61b68 Compare August 14, 2023 14:00
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally (rebased on top of master 6758a7f), it works as expected. Code looks good to me at a glance.

There are some caveats though:

  • This is only available for built-in arrays, not exported array properties (or exported dictionary properties) as these use a different editor.
  • There's a noticeable delay after releasing the mouse button, even in a small array of 5 items. This is likely because properties aren't force-updated after the event is handled, so it's relying on editor polling to update (which occurs every 0.2 seconds by default). Watch the video at 0.5× speed by right-clicking it:
simplescreenrecorder-2023-08-25_00.55.26.mp4

If you change the Docks > Property Editor > Auto Refresh Interval editor setting, the delay will change accordingly. It would be good to figure out whether this can be improved (without changing this default setting, as it's kept at 0.2 to reduce CPU utilization).

@KoBeWi
Copy link
Member

KoBeWi commented Aug 25, 2023

Down button doesn't work.

godot.windows.editor.dev.x86_64_ll9msRhyXI.mp4

@jmb462
Copy link
Contributor Author

jmb462 commented Aug 27, 2023

@KoBeWi Fixed. I've missed that some classes use a specific function call to reorder items. (as TileMap and TileSet)

@Calinou For the delay, it seems to apply both with new buttons and with drag/drop reorder. Strangely, changing the auto refresh interval doesn't affect this delay for me (even if set to a very high or low value).

For the exported array / dict, the display is more compact and I don't know where to place new buttons without overloading the look. :(
image

Edit : the drag management is totaly different with exported arrays and allow to change page while dragging. Maybe we should just do the same for others arrays.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 28, 2023

EditorInspectorArray is for arrays of grouped properties, EditorPropertyArray is for arrays of values. They serve a bit different purposes and the latter is much more compact as a result and arrows aren't really feasible to add there.

Edit : the drag management is totaly different with exported arrays and allow to change page while dragging. Maybe we should just do the same for others arrays.

Makes sense, doesn't need to be done in this PR.

@akien-mga akien-mga requested a review from Calinou August 28, 2023 17:22
@akien-mga akien-mga modified the milestones: 4.x, 4.2 Aug 28, 2023
@akien-mga akien-mga merged commit c1e85c6 into godotengine:master Aug 29, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants