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 "Import Defaults" selector not being initialized incorrectly #80914

Merged

Conversation

jsjtxietian
Copy link
Contributor

In ImportDefaultsEditor, when first call the function, last_selected can be the first one(Animation Library) and directly show the settings for "Animation Library", fix #80867

However, there is another option: show the <Select Importer> option, I decided to go with the first option but this one is ok too. Can change my code to impl this one.

@dalexeev
Copy link
Member

However, there is another option: show the <Select Importer> option

In my opinion, <Select Importer> doesn't serve any purpose, we can remove it:

diff --git a/editor/import_defaults_editor.cpp b/editor/import_defaults_editor.cpp
index 98a9bfe9dc..a0ee2c2b66 100644
--- a/editor/import_defaults_editor.cpp
+++ b/editor/import_defaults_editor.cpp
@@ -169,17 +169,15 @@ void ImportDefaultsEditor::_importer_selected(int p_index) {
 
 void ImportDefaultsEditor::clear() {
 	String last_selected;
-	if (importers->get_selected() > 0) {
+	if (importers->get_selected() >= 0) {
 		last_selected = importers->get_item_text(importers->get_selected());
 	}
 
 	importers->clear();
 
-	importers->add_item("<" + TTR("Select Importer") + ">");
-	importers->set_item_disabled(0, true);
-
 	List<Ref<ResourceImporter>> importer_list;
 	ResourceFormatImporter::get_singleton()->get_importers(&importer_list);
+
 	Vector<String> names;
 	for (const Ref<ResourceImporter> &E : importer_list) {
 		String vn = E->get_visible_name();
@@ -190,8 +188,9 @@ void ImportDefaultsEditor::clear() {
 	for (int i = 0; i < names.size(); i++) {
 		importers->add_item(names[i]);
 
-		if (names[i] == last_selected) {
-			importers->select(i + 1);
+		if (names[i] == last_selected || (i == 0 && last_selected.is_empty())) {
+			importers->select(i);
+			_update_importer();
 		}
 	}
 }

@jsjtxietian
Copy link
Contributor Author

jsjtxietian commented Aug 23, 2023

In my opinion, <Select Importer> doesn't serve any purpose, we can remove it:

Agreed. The OptionButton itself is quite clear, thus maybe we don't need a placeholder there to tell the user to select one importer.

@jsjtxietian jsjtxietian force-pushed the init-importer-default-correctly branch from 572ea51 to 10ed170 Compare August 23, 2023 09:50
@jsjtxietian jsjtxietian force-pushed the init-importer-default-correctly branch from 10ed170 to b795abc Compare August 24, 2023 02:11
Copy link
Member

@dalexeev dalexeev left a comment

Choose a reason for hiding this comment

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

Looks good to me.

editor/import_defaults_editor.cpp Outdated Show resolved Hide resolved
@jsjtxietian jsjtxietian force-pushed the init-importer-default-correctly branch from b795abc to 61b56ac Compare August 24, 2023 07:03
@YuriSizov YuriSizov changed the title Fix selection list 'Importer' initialized incorrectly Fix "Import Defaults" selector not being initialized incorrectly Aug 25, 2023
@YuriSizov
Copy link
Contributor

The solution makes sense to me, but could you please amend your commit message to follow the preferred style of the project? Your first line needs concisely describe the change, so you can use the title of the PR for this purpose. You can keep the rest, just put it into commit's description instead.

In ImportDefaultsEditor, delete 'Select Importer';
when first call the function, last_selected should
be the first one, directly show the settings for
"Animation Library"
@jsjtxietian jsjtxietian force-pushed the init-importer-default-correctly branch from 61b56ac to f997fee Compare August 25, 2023 16:27
@jsjtxietian
Copy link
Contributor Author

Thank you for your help, I missed the commit message requirement mentioned in CONTRIBUTING.md, sorry for that!

@YuriSizov YuriSizov merged commit 3c71214 into godotengine:master Aug 25, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@jsjtxietian jsjtxietian deleted the init-importer-default-correctly branch November 1, 2023 07:35
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.

Project -> Project Settings -> Import Defaults -> The selection list 'Importer' is initialized incorrectly
3 participants