-
Notifications
You must be signed in to change notification settings - Fork 32
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
Loader: Implement existing product group picker in Group Products dialog #668
base: develop
Are you sure you want to change the base?
Loader: Implement existing product group picker in Group Products dialog #668
Conversation
Co-authored-by: Jakub Trllo <[email protected]>
…ttps://github.com/BigRoy/ayon-core into enhancement/product_group_add_existing_group_picker
Co-authored-by: Jakub Trllo <[email protected]>
…ttps://github.com/BigRoy/ayon-core into enhancement/product_group_add_existing_group_picker
self._name_line_edit.set_options(list(sorted(product_groups))) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pre-fill group field value based on selection.
self._name_line_edit.set_options(list(sorted(product_groups))) | |
product_group = "" | |
if product_groups: | |
product_group = next(iter(product_groups)) | |
self._name_line_edit.setText(product_group) | |
self._name_line_edit.set_options(list(sorted(product_groups))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The listed product groups are ALL groups under the active folders - not just the groups of the products in your selection. Just so that you can easily move it into another group outside of your selection.
So technically, this would first have to get all the product entities, get their product groups - then from those pick the first entry (or most used entry)...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, right now it's prefilled with previous value of the widget. So either we use first, or empty string, or most common, but current behavior is not correct.
self._name_line_edit.set_options(list(sorted(product_groups))) | |
self._name_line_edit.setText("") | |
self._name_line_edit.set_options(list(sorted(product_groups))) |
|
||
def set_product_ids(self, project_name, product_ids): | ||
self._project_name = project_name | ||
self._product_ids = product_ids | ||
|
||
# Update the product groups | ||
product_groups = self._controller.get_folder_product_group_names( | ||
project_name=self._controller.get_selected_project_name(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
project_name=self._controller.get_selected_project_name(), | |
project_name=project_name, |
# Update the product groups | ||
product_groups = self._controller.get_folder_product_group_names( | ||
project_name=self._controller.get_selected_project_name(), | ||
folder_ids=self._controller.get_selected_folder_ids() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW I think the method should expect product_ids
instead of folder ids, to avoid using self._controller.get_selected_folder_ids()
. Or set_product_ids
method should expect folder ids too, because the widget calling the method has access to folder ids.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See: https://github.com/ynput/ayon-core/pull/668/files#r1765457734
- We need the folder ids to query ALL the available product names under the folders - including those not in the current products selection.
- We need the product ids to know which products we actually want to change with the dialog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, both can be send to this method. The only place calling this function already have access to it, so it can just pass it in.
Changelog Description
Implement an "existing product group" picker in Group Products dialog,
Additional info
Fixes #648
Testing notes: