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

Show only compatible nodes in 'Select a node' window #79213

Merged
merged 1 commit into from
Aug 19, 2023

Conversation

martinboue
Copy link
Contributor

@martinboue martinboue commented Jul 8, 2023

@MewPurPur
Copy link
Contributor

I'd suggest changing the text to just "Show All" as there's nothing else it could be, and to cuddle it up on the same horizontal level as the filter input field. You're very unlikely to write something really long in it. Also, both serve the same purpose of filtering.

image

^ Example of a similar thing elsewhere in the editor.

@martinboue
Copy link
Contributor Author

Indeed, looks much better now @MewPurPur 😊

image

@martinboue martinboue force-pushed the show-only-compatible-nodes branch from 929e3f9 to 37a7fe8 Compare July 10, 2023 18:46
@martinboue martinboue requested a review from MewPurPur July 10, 2023 18:48
@martinboue martinboue force-pushed the show-only-compatible-nodes branch from 37a7fe8 to 8c5bf10 Compare July 10, 2023 19:01
Copy link
Contributor

@MewPurPur MewPurPur left a comment

Choose a reason for hiding this comment

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

LGTM. Nitpick - Our code style enforces punctuation for proper sentences.

@martinboue martinboue force-pushed the show-only-compatible-nodes branch from 8c5bf10 to dcace12 Compare July 10, 2023 19:45
@martinboue
Copy link
Contributor Author

LGTM. Nitpick - Our code style enforces punctuation for proper sentences.

Oh, didn't know about that @MewPurPur , that's no problem. Should be fixed.

(Now that you've said it, I notice these dots everywhere! 😄)

@MewPurPur
Copy link
Contributor

MewPurPur commented Jul 10, 2023

Lots of places in the codebase are missing dots, yeah. You can often date Godot's code by how its comments look :P

@martinboue
Copy link
Contributor Author

Is there anything left I need to do before merge ?

@MewPurPur
Copy link
Contributor

It's a waiting game now!

@martinboue martinboue force-pushed the show-only-compatible-nodes branch from dcace12 to 90a3891 Compare August 15, 2023 13:30
@martinboue
Copy link
Contributor Author

martinboue commented Aug 15, 2023

Update

Rebased with master and tested with #79593 / #79593 and works fine:
image

Also moved the code to create the 'Show All' CheckButton inside the newly SceneTreeDialog::set_valid_types method.

@MewPurPur
Copy link
Contributor

MewPurPur commented Aug 15, 2023

There's an erroneous tab making static checks fail. We keep empty lines really empty for consistency in code style.

@martinboue martinboue force-pushed the show-only-compatible-nodes branch from 90a3891 to c711472 Compare August 15, 2023 13:41
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 c495eb5), it works as expected.

Testing project: test_pr_79213.zip

Node tree

image

Without filter

Show All disabled Show All enabled
Screenshot_20230815_162351 Screenshot_20230815_162359

With filter

Show All disabled Show All enabled
image image

@martinboue
Copy link
Contributor Author

@Calinou I don't think it's working properly when 'Show All' is disabled. On your screenshots, the nodes in red should not appear:

image

I'll fix it, thanks for testing!

@martinboue martinboue force-pushed the show-only-compatible-nodes branch from c711472 to 1be1904 Compare August 15, 2023 16:35
@martinboue
Copy link
Contributor Author

It's fixed, here is the updated result with the test project provided by @Calinou :

Without filter

Show All disabled Show All enabled
image image

With filter

Show All disabled Show All enabled
image image

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Aug 18, 2023
@akien-mga akien-mga requested a review from KoBeWi August 18, 2023 07:11
@akien-mga
Copy link
Member

Minor nitpick, but usually we keep the title of commit messages only for the description of the change, and references to issues / proposals go in the body of the commit, e.g.:

Show only compatible nodes in 'Select a node' window

Fixes https://github.com/godotengine/godot-proposals/issues/7217

@KoBeWi
Copy link
Member

KoBeWi commented Aug 18, 2023

I think the state of Show All should be remembered between sessions. You can use EditorSettings.set_project_metadata() for that.

@martinboue martinboue force-pushed the show-only-compatible-nodes branch 2 times, most recently from 2406db4 to 200a7fa Compare August 18, 2023 11:49
@martinboue
Copy link
Contributor Author

martinboue commented Aug 18, 2023

Minor nitpick, but usually we keep the title of commit messages only for the description of the change, and references to issues / proposals go in the body of the commit, e.g.:

Show only compatible nodes in 'Select a node' window

Fixes https://github.com/godotengine/godot-proposals/issues/7217

Noted, it's updated. Thanks.

I think the state of Show All should be remembered between sessions. You can use EditorSettings.set_project_metadata() for that.

It's done. I've used "editor_metadata" as section, which I thought was the most suitable. I'm not sure if that's the correct one, please correct me if necessary, I don't know how the metadata are organised.

EditorSettings::get_singleton()->get_project_metadata("editor_metadata", "show_all_nodes", false)

@KoBeWi
Copy link
Member

KoBeWi commented Aug 18, 2023

"editor_metadata" section is fine, but the name is not very descriptive. Something like "show_all_nodes_for_node_selection" would be better.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Left some minor comments, but otherwise it's fine.

@martinboue martinboue force-pushed the show-only-compatible-nodes branch from 200a7fa to bf4cbd4 Compare August 18, 2023 16:52
@martinboue
Copy link
Contributor Author

Everything is fixed, should be ready to merge. Thanks everyone for testing and reviewing ❤️

@akien-mga akien-mga merged commit 1fbe3e1 into godotengine:master Aug 19, 2023
@akien-mga
Copy link
Member

Thanks!

@martinboue martinboue deleted the show-only-compatible-nodes branch August 23, 2023 17:55
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.

Show only compatible nodes in "Select a node" window
6 participants