SG-38067 hide disabled pipeline steps for the project #164
Conversation
barbara-darkshot
left a comment
barbara-darkshot
left a comment
There was a problem hiding this comment.
hey @kuldeepgudekar ,
I left some comments so you can start to make the code more readable
I'll look more into the logic once the code will be cleaned :)
thanks!
info.yml
Outdated
| # | ||
| project_visible_pipeline_steps: | ||
| type: bool | ||
| description: If True, only pipeline Steps visible for the current Project and entity type are listed. |
There was a problem hiding this comment.
could you improve the description? it's not clear what you are talking about here
pipeline steps can be used at other locations in the code, would be nice to describe it more in details (what menu is affected, what's the default behavior, ...)
There was a problem hiding this comment.
I have updated the documentation for the flag.
| return | ||
|
|
||
| bundle = sgtk.platform.current_bundle() | ||
| shotgun_api = bundle.shotgun |
There was a problem hiding this comment.
you don't need to create a new variable to store the shotgun_api object, you can directly call it from bundle.shotgun
| bundle = sgtk.platform.current_bundle() | ||
| shotgun_api = bundle.shotgun | ||
| # Use the engine's Shotgun instance for schema calls | ||
| engine_shotgun_api = sgtk.platform.current_engine().shotgun |
There was a problem hiding this comment.
same here, why you need the shotgun instance of the engine while you have the one from the current bundle?
There was a problem hiding this comment.
In sgtk.platform.current_bundle() schema_field_read method not available, that's why I need to use sgtk.platform.current_engine()shotgun instance, maybe because shotgun_schema_read method is part of the lower-level implementation?
| limit_steps_to_project_visible = bundle.get_setting( | ||
| "project_visible_pipeline_steps", False | ||
| ) | ||
| current_project = getattr(bundle.context, "project", None) |
There was a problem hiding this comment.
in this case, the context will always have a project so you don't need to store it in a variable, you can just call bundle.context.project where you need to access the context project
| :returns: List of Step dictionaries with at least 'id', 'code', | ||
| 'entity_type' and optionally 'color'. | ||
| """ | ||
| StepListWidget._cache_step_list() |
There was a problem hiding this comment.
I'm probably being picky but I really don't like non-UI code calling into UI code even if _cache_step_list is a class method on the UI class. I wonder if the step cache should be a class of it's own that both the StepListWidget and get_steps_for_entity_type make use of?
There was a problem hiding this comment.
Yeah, sure. I have updated the implementation, let me know how that looks.
pscadding
left a comment
pscadding
left a comment
There was a problem hiding this comment.
@barbara-darkshot asked me to give this a review, I just had a couple of small comments.
| return list(StepCache.get_step_map().get(entity_type, [])) | ||
|
|
||
|
|
||
| class StepCache(object): |
There was a problem hiding this comment.
@barbara-darkshot It feels to me like any logic around step caching should live in the shotgun utils?
https://github.com/shotgunsoftware/tk-framework-shotgunutils/blob/master/python/shotgun_globals/cached_schema.py
@pscadding I have migrated caching code to the cached_schema.py. Ref : shotgunsoftware/tk-framework-shotgunutils#173
There was a problem hiding this comment.
oh, that's a good catch! I was not aware of this class!
it's a great idea, especially because this class already has a method to ensure if a field is visible so we could use that => https://github.com/shotgunsoftware/tk-framework-shotgunutils/blob/master/python/shotgun_globals/cached_schema.py#L907C9-L907C25
maybe we could add a get_visible_steps_for_project method in this class as well?
@barbara-darkshot , reused the field is visible functionality for the class and added respective methods as well. Ref : shotgunsoftware/tk-framework-shotgunutils#173
| :returns: List of Step dictionaries with at least 'id', 'code', | ||
| 'entity_type' and optionally 'color'. | ||
| """ | ||
| StepCache.ensure_loaded() |
There was a problem hiding this comment.
It looks like its rebuilding the step cache everytime this function is called, can we instead reuse it if it is already built in the session?
It looks like calling get_step_map on its own would do that?
| super().__init__() | ||
| self._list_widget = list_widget | ||
| self._cache_step_list() | ||
| StepCache.ensure_loaded() |
There was a problem hiding this comment.
Similar to above do we need to call ensure_loaded here? It looks like it is called as part of get_step_map.
| return list(get_step_map().get(entity_type, [])) | ||
|
|
||
|
|
||
| def get_step_map(): |
There was a problem hiding this comment.
should we make this method private and only expose get_steps_for_entity_type?
I feel like get_step_map is only intended to be used in StepListWidget
Also, I'm not sure if the function name is enough descriptive?
There was a problem hiding this comment.
Makes sense. Updated the method name and scope.
DO NOT MERGE THIS PR, this is only to do code review for now