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

Fix "View Owners" dialog not acknowledging that some resources aren't scenes #68697

Conversation

MewPurPur
Copy link
Contributor

@MewPurPur MewPurPur commented Nov 15, 2022

image
Silly dialog!

  • Fixes resources not being openable in this dialog
  • No popup if multiple resources are selected, or if a mix of scenes and other resources is selected.
  • Misc enhancement: Deselects all items when you click on empty space, as one would expect.

This behavior matches the FileSystem dock. The whole dialog could use a glow up, but this fixes its bugs for now.

@MewPurPur MewPurPur force-pushed the dependency-editor-thinks-every-resource-is-scene branch from a1aafad to 951311b Compare November 15, 2022 22:56
@Calinou Calinou added this to the 4.0 milestone Nov 15, 2022
@MewPurPur MewPurPur force-pushed the dependency-editor-thinks-every-resource-is-scene branch 3 times, most recently from 60dc4bb to cb86eea Compare November 16, 2022 02:00
Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

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

Seems fine by me!

@MewPurPur MewPurPur force-pushed the dependency-editor-thinks-every-resource-is-scene branch from cb86eea to dca9167 Compare January 15, 2023 13:49
@MewPurPur
Copy link
Contributor Author

Pretty safe bugfix IMO, but could wait for after 4.0

@YuriSizov YuriSizov modified the milestones: 4.0, 4.1 Feb 9, 2023
Comment on lines +300 to +303
if (only_scenes_selected) {
file_options->add_icon_item(get_theme_icon(SNAME("Load"), SNAME("EditorIcons")), TTRN("Open Scene", "Open Scenes", selected_items.size()), FILE_OPEN);
} else if (selected_items.size() == 1) {
file_options->add_icon_item(get_theme_icon(SNAME("Load"), SNAME("EditorIcons")), TTR("Open"), FILE_OPEN);
Copy link
Contributor

Choose a reason for hiding this comment

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

Both the old and the new code is rather misleading, as both options are identical. Only the text is different. I think we could simplify this. And we could also rework the awkward return below in the process with an early return.

Copy link
Contributor Author

@MewPurPur MewPurPur Jun 2, 2023

Choose a reason for hiding this comment

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

Not sure I follow... You can open multiple scenes at once, unlike other resources, so they are separated.

Copy link
Contributor

@YuriSizov YuriSizov Jun 5, 2023

Choose a reason for hiding this comment

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

Both these options trigger FILE_OPEN with no additional information passed from this bit, only the text on the option is different (even the icon is the same).

Copy link
Contributor

@YuriSizov YuriSizov Jun 6, 2023

Choose a reason for hiding this comment

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

Well, in fact, this is quite a lot of checking just to display a slightly different text. Perhaps we should just call the option "Open Selected"? What's the significance of writing "Open Scenes"?

Edit: I guess we still want to know if only scenes are selected, otherwise the action should be simply disabled. Eh, let's leave it as is.

@YuriSizov
Copy link
Contributor

Regardless of the suggestion above, it's probably worth rebasing this because it's been a few months at this point.

@MewPurPur MewPurPur force-pushed the dependency-editor-thinks-every-resource-is-scene branch from dca9167 to d7eefc3 Compare June 2, 2023 18:34
@YuriSizov YuriSizov merged commit 9fbbb45 into godotengine:master Jun 6, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@MewPurPur MewPurPur deleted the dependency-editor-thinks-every-resource-is-scene branch June 6, 2023 12:31
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