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 conflicted indeterminate state in asset importer #84953

Merged
merged 1 commit into from
Feb 14, 2024

Conversation

rakkarage
Copy link
Contributor

@rakkarage rakkarage commented Nov 16, 2023

This pull request fixes an issue in the asset importer where the indeterminate state of conflicted items was not being properly resolved. It adds a new signal propagate_check_finished to the Tree class and a new method _fix_conflicted_indeterminate_state to the EditorAssetInstaller class to handle this.

Closes: #84021

Currently in Godot, in the asset import dialog (editor_asset_installer) it deals with indeterminate like this.

void EditorAssetInstaller::_check_propagated_to_item(Object *p_obj, int p_column) {
	TreeItem *affected_item = Object::cast_to<TreeItem>(p_obj);
	if (!affected_item) {
		return;
	}

	Dictionary item_meta = affected_item->get_metadata(0);
	bool is_conflict = item_meta.get("is_conflict", false);
	if (is_conflict) {
		affected_item->set_checked(0, false);
		affected_item->propagate_check(0, false);
	}
}

This is a problem because normally a indeterminate parent when clicked will become checked, but because some of the children are is_conflict they are unchecked again? which causes parent to go indeterminate again and repeat. So it prevents toggling from folder.

@rakkarage rakkarage force-pushed the asset-install-conflict branch 3 times, most recently from d0e110a to f86833d Compare November 16, 2023 01:35
@rakkarage rakkarage marked this pull request as ready for review November 16, 2023 01:57
@rakkarage rakkarage requested review from a team as code owners November 16, 2023 01:57
@AThousandShips AThousandShips added this to the 4.3 milestone Nov 16, 2023
@KoBeWi
Copy link
Member

KoBeWi commented Nov 16, 2023

Why is the new signal needed? Can't you just

tree->propagate_check();
_fix_conflicted_indeterminate_state()

?

@rakkarage
Copy link
Contributor Author

rakkarage commented Nov 16, 2023

Thanks. I tried it like that first but _check_propagated_to_item is already being called multiple times during a:

void TreeItem::propagate_check(int p_column, bool p_emit_signal) {
	bool ch = cells[p_column].checked;

	if (p_emit_signal) {
		tree->emit_signal(SNAME("check_propagated_to_item"), this, p_column);
	}
	_propagate_check_through_children(p_column, ch, p_emit_signal);
	_propagate_check_through_parents(p_column, p_emit_signal);
}

Which is how it gets to _check_propagated_to_item so modifying the tree while it was in the process of propagating a check did not seem to work even when limiting it to only calling when affected_item == root.

Seems much easier to let it finish propagating then fix. Idk.

Thanks.

@KoBeWi
Copy link
Member

KoBeWi commented Nov 16, 2023

I mean you can fix the conflicts after calling propagate_check() in EditorAssetInstaller. The function is not async, so right after the call is when it has already finished. The signal is redundant.

@rakkarage
Copy link
Contributor Author

rakkarage commented Nov 16, 2023

I (dialog) don't call propagate_check tree does, when something gets checked or unchecked or added or...
The existing code just uses this handler to uncheck conflict when checked...
I need a new handler to know when tree has finished propagating checks and dialog has finished flipping them back off again. propagate_check_finished idk

void EditorAssetInstaller::_check_propagated_to_item(Object *p_obj, int p_column) {
	TreeItem *affected_item = Object::cast_to<TreeItem>(p_obj);
	if (!affected_item) {
		return;
	}

	Dictionary item_meta = affected_item->get_metadata(0);
	bool is_conflict = item_meta.get("is_conflict", false);
	if (is_conflict) {
		affected_item->set_checked(0, false);
		affected_item->propagate_check(0, false);
	}
}

I am probably just misunderstanding. Thanks, I will take another look at it.

@KoBeWi
Copy link
Member

KoBeWi commented Nov 16, 2023

But I do see propagate_check() calls in EditorAssetInstaller. It's in item checked callback, but it's the installer calling it, not the Tree itself. And your new signals is emitted right at the end of this method, which is the same as if the method's caller did something after the call.

I (dialog) don't call propagate_check tree does

Where? Maybe I'm missing something.

@rakkarage rakkarage force-pushed the asset-install-conflict branch 2 times, most recently from 2c2c11a to d5d2982 Compare November 17, 2023 03:12
@rakkarage
Copy link
Contributor Author

I apologize for the confusion. :)
I was focused (tunnel vision) on the one propagate_check call inside _check_propagated_to_item which is signaled/called from TreeItem::propagate_check, but ya adding fix after all other propagate_check calls works.
Thanks.

@rakkarage rakkarage force-pushed the asset-install-conflict branch from d5d2982 to 3142206 Compare November 17, 2023 03:16
@fire fire requested a review from a team November 17, 2023 05:56
@KoBeWi
Copy link
Member

KoBeWi commented Nov 17, 2023

Shouldn't the conflict check in _check_propagated_to_item() be removed now?

Also the branch with conflicts appears as checked, while I think it should be indeterminate
image
Other than that it seems to work correctly.

EDIT:
I see the new behavior is intended. Not sure which behavior would be better tbh.

@rakkarage rakkarage force-pushed the asset-install-conflict branch from 3142206 to cc95658 Compare November 17, 2023 15:47
@rakkarage rakkarage force-pushed the asset-install-conflict branch from cc95658 to 58c9e34 Compare November 18, 2023 01:00
@akien-mga akien-mga requested review from YuriSizov and removed request for a team January 9, 2024 16:51
@akien-mga akien-mga merged commit a33a87d into godotengine:master Feb 14, 2024
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
Development

Successfully merging this pull request may close these issues.

When importing an asset, you can't deselect a partially selected folder.
4 participants