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

Rename tree item double-click signal to match the actual behaviour #44185

Closed

Conversation

madmiraal
Copy link
Contributor

@madmiraal madmiraal commented Dec 8, 2020

As identified here, currently a Tree emits an item_activated signal when the item's label is double-clicked, and an item_double_clicked signal when the item's icon is double-clicked.

This PR renames the Tree's item double-click signals to match the events and improves docs:

  • item_double_clicked -> item_icon_double-clicked

Part of #16863.

Bugsquad: updated to reflect current state

scene/gui/tree.cpp Outdated Show resolved Hide resolved
@Faless
Copy link
Collaborator

Faless commented Dec 28, 2020

As discussed, item_activated also fires when ui_accept is pressed (e.g. spacebar, enter, controller button, etc).
So, I'd leave the signal name as is and update the docs instead.
Renaming item_double_clicked to item_icon_double_clicked seems fine, but we should double-check that it actually behaves as documented (since I couldn't get it to fire in my 2 minute test).

@madmiraal
Copy link
Contributor Author

I've done some digging, item_double_clicked is emitted here:

godot/scene/gui/tree.cpp

Lines 2733 to 2739 in f1cb475

if (rect.has_point(mpos)) {
if (!edit_selected()) {
emit_signal("item_double_clicked");
}
} else {
emit_signal("item_double_clicked");
}

It's emitted when clicking outside of the editable portion of an editable TreeItem. For example, when clicking the icon of an editable TreeItem. The editor SceneTreeDock connects to it, where it's used to change the viewport focus to the selected item (i.e. like using the 'F' hotkey with the mouse in the viewport). Note: If select_mode is Multi, it requires the item to be clicked twice; the same way editing requires a second click.

My suggestion is to rename it item_editable_icon_clicked; as this seems to be its main purpose. The documentation needs to make it clear that:

  1. It requires the item to to be editable
  2. If the select_mode is Multi, it needs to be clicked twice.
  3. It will be emitted if any non-editable portion (not just the icon) of the item is clicked.

For those interested in the history: It was originally added as part of #5956 to solve #5791 i.e. to trigger the editor to focus on the item that was double-clicked. Note: At the time it differentiated between a fast double click (< 400 ms) and a slow double-click (between 400 and 800 ms). item_double_clicked was only emitted with a fast double-click. The dual-double-click nature was problematic (see #6452), and was changed with #7009, which both removed the need for a double-click and enabled the differentiation between clicking the editable portion and the non-editable portion (i.e. the icon) of the item. The item_double_clicked is now only emitted when the non-editable portion i.e. the icon of an already selected item is clicked. Note: The second (or rather the first i.e. after if (!edit_selected())) was added as part of #29681, but I'm not sure why, as this part of the code can only be reached if the item is editable; so checking whether edit_selected() returns false appears superfluous.

@madmiraal madmiraal force-pushed the rename-tree-item-double-clicks branch from 8898a22 to 08c7e31 Compare December 30, 2020 17:31
@madmiraal
Copy link
Contributor Author

Updated to only rename item_double_clicked to item_editable_icon_clicked and the documentation associated with the two signals.

@mhilbrunner mhilbrunner changed the title Rename tree item double-click signals to match their event Rename tree item double-click signal to match their actual behaviour Nov 25, 2021
@mhilbrunner mhilbrunner changed the title Rename tree item double-click signal to match their actual behaviour Rename tree item double-click signal to match the actual behaviour Nov 25, 2021
@YuriSizov
Copy link
Contributor

Superseded by #70290.

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

Successfully merging this pull request may close these issues.

4 participants