Skip to content

SG-2824 Sorting my tasks #158

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

carlos-villavicencio-adsk
Copy link
Contributor

@carlos-villavicencio-adsk carlos-villavicencio-adsk commented May 13, 2025

Adds sorting for My Tasks left tab. It allows the user to provide custom fields in the info.yml file (assuming that they are valid fields to sort by).

image

Heavily inspired by:

@carlos-villavicencio-adsk carlos-villavicencio-adsk requested review from pscadding and a team May 15, 2025 13:27
@carlos-villavicencio-adsk carlos-villavicencio-adsk marked this pull request as ready for review May 15, 2025 13:27
Copy link
Contributor

@julien-lang julien-lang left a comment

Choose a reason for hiding this comment

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

Wow it's odd to see a 4-digit ticket ID ;)

I have a few initial questions but mostly trying to understand how the code works.

@@ -242,6 +242,27 @@ configuration:
description: Controls whether new tasks can be created from the app.
default_value: True

sort_fields:
type: dict
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really a dict? From looking at the lines below, it looks more like a list, no?
Or is it how tk-core handles this type of setting?

Copy link
Contributor Author

@carlos-villavicencio-adsk carlos-villavicencio-adsk May 15, 2025

Choose a reason for hiding this comment

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

If I understand correctly, it is a dict on the shape:

{
  "tasks": [
    {"field_code": "due_date", "display_name": "Due Date"},
    ...
  ]
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Reading this code: https://github.com/shotgunsoftware/tk-core/blob/v0.22.3/python/tank/platform/validation.py#L541

I believe the type should be list not dict.

Am I wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate more? I don't understand.

If you see this line, I see clearly that the setting is a dict, otherwise the .get("something") would fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the same as I implemented in the shotgunpanel.
And the core should support dict types.

I believe it makes sense to have it as a dictionary, as you say you can then look up the settings by tab name.
It's a dictionary of lists containing a dictionary.

tab name (dict)
    sort rules (list)
        sort rule details (dict)

Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to challenge the previous comment. The data type is list<dict> and not dict<list>. So the current definition of info.yml is wrong.

I think I found one similar setting that was properly implemented: https://github.com/shotgunsoftware/tk-multi-publish2/blob/v2.10.3/info.yml#L74

Copy link
Contributor

Choose a reason for hiding this comment

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

@carlos-villavicencio-adsk could you answer on this point, please?

@julien-lang julien-lang requested a review from Copilot May 15, 2025 23:18
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces sorting functionality for the My Tasks left tab, allowing users to configure custom sort fields via the info.yml file.

  • Adds a sort button in the My Tasks tab with associated setup and UI functionality.
  • Extends the data loading methods to support additional sorting parameters.
  • Updates related configurations and UI behavior in browser and proxy models.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
python/tk_multi_workfiles/my_tasks/my_tasks_form.py Imports QtGui, adds a sort button setup method for the tasks form UI.
python/tk_multi_workfiles/framework_qtwidgets.py Imports shotgun_fields to support new sorting UI elements.
python/tk_multi_workfiles/entity_tree/entity_tree_proxy_model.py Removes auto alphabetical sorting to defer sort behavior to UI selection.
python/tk_multi_workfiles/entity_models/extended_model.py Adds extra_sorting support to the load_and_refresh method.
python/tk_multi_workfiles/browser_form.py Implements new sorting setup and load_sort_data logic for task sorting via menu.
info.yml Introduces sort_fields configuration for specifying sort field options.

Copy link
Contributor

@pscadding pscadding 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.

If I'm being picky, I don't like the sort button next to the new task button, that row of widgets looks messy. I might have put it next to the search box below maybe? Feel free to ignore though.

@@ -242,6 +242,27 @@ configuration:
description: Controls whether new tasks can be created from the app.
default_value: True

sort_fields:
type: dict
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the same as I implemented in the shotgunpanel.
And the core should support dict types.

I believe it makes sense to have it as a dictionary, as you say you can then look up the settings by tab name.
It's a dictionary of lists containing a dictionary.

tab name (dict)
    sort rules (list)
        sort rule details (dict)

allows_empty: True
default_value:
tasks:
- { field_code: due_date, display_name: Due Date }
Copy link
Contributor

Choose a reason for hiding this comment

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

One difference from the panel app I note is not including an optional default property, to define which of the settings would be used by default. Was this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it was. This is a different factor from your original feature on the panel. For performance reasons, we initially display the default sorting that comes from the server, and then, the user can do the sorting.

Comment on lines +755 to +770
fields_manager = shotgun_fields.ShotgunFieldManager(
self, bg_task_manager=self._task_manager
)
fields_manager.initialized.connect(self._field_filters)
fields_manager.initialize()
self._sort_menu_actions()

def _field_filters(self):
"""
Define a few simple filter methods for use by the menu
"""
# Attach the filters
# None of the fields are included, checked, and disabled at startup
self._entity_field_menu.set_field_filter(lambda field: False)
self._entity_field_menu.set_checked_filter(lambda field: False)
self._entity_field_menu.set_disabled_filter(lambda field: False)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand what the fields manager and field filters are for?

Based on the comment it sounds like it will be filtering out any options that were configured if the user doesn't have visibility of the field in FPTR, is that correct?
I'm not sure I understand how this is working without a deeper dive.

Should the panel app be doing this as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was implemented based on shotgunsoftware/tk-multi-shotgunpanel#67.

As far as I understand, the field_manager defined like this:

        fields_manager = shotgun_fields.ShotgunFieldManager(
            self, bg_task_manager=self._task_manager
        )

Brings all possible fields from the server. Later in this method it hides everything. Then, it starts displaying the fields based on the YAML settings.

I'm not sure the main reason behind this TBH.

Copy link
Contributor

@julien-lang julien-lang left a comment

Choose a reason for hiding this comment

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

Could you add the "sort" icon please? Just like in the tk-multi-shotgunpanel?

image

Then, can we talk about improving the layout? Like Phil was saying the sort button aside from the "create a task" button is not the best.

@carlos-villavicencio-adsk
Copy link
Contributor Author

@julien-lang @pscadding After a discussion with our design consultant, we decided to go with this layout. PR is updated now.

image

@julien-lang julien-lang requested a review from pscadding May 30, 2025 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants