getavalon / core

The safe post-production pipeline - https://getavalon.github.io/2.0
MIT License
218 stars 48 forks source link

Loader GUI subset grouping #391

Closed davidlatwe closed 5 years ago

davidlatwe commented 5 years ago

Feature - Visually grouping subsets in Loader GUI

loader-subset

This grouping feature is on subset level, and the group name is from the subset document's data.subsetGroup field. So to trigger subset grouping, you need to inject subsetGroup entry to the subset document data during publish. Or maybe another little tool for it ?

Welcome to give any inputs :)

Update

See comment below

BigRoy commented 5 years ago

Visually this is great, very very nice work David! :) I will still need to check the code itself though.

Grouped icon

Anyway, first a note. The grouped entries have fa.file as icon as opposed to fa.file-o making them stand out a lot and also appear very differently then regular entries, even though they are not necessarily different. How about just toning the color a bit but keeping the same icon? (Like maybe tinted slightly darker). It's not a big issue, it's just that the full opaque folder icon is so.... full. It really pops out.

I wonder what others think about that.

The group icon however looks totally fine to me.. but, that would be one that makes more sense to me to somehow become the opaque folder. ;)

tokejepsen commented 5 years ago

How about just toning the color a bit but keeping the same icon?

Does it even need a different icon? Personally think the indentation is enough.

davidlatwe commented 5 years ago

I made a few changes and updates, including current icon restored. :)

Here's the GIF:

loader-subset-fix


Predefined groups (pretty groups)

Like families and tasks, this new element is named groups and storing in project config. A group has these attributes:

{
    "name": "Trash Bin",  # Must
    "icon": "trash-o",  # Optional
    "color": "#c4cedc",  # Optional, icon color
    "order": -99  # Optional, `float` or `int`, default 0
}

Sorting

Groups in Loader GUI are sorting by the "order" attribute, not by it's name, and only sort with other groups. Also noted that groups are always sorting in descending order and always be on top of the view.

Grouping subsets

By injecting the group name into subsetGroup entry in the subset document data, the Loader GUI will create a visual group item to grouping those subsets which has the same group name defined in the data.subsetGroup.

If the group name could not be found in the project config, it will use default icon and order. So those predefined groups are just for customizing icons and orders.

Please let me know what you think :)

BigRoy commented 5 years ago

Visually that is so clever - very nice @davidlatwe!

Like families and tasks, this new element is named groups and storing in project config.

Does this mean the project schema needs a new version/update? Or does it still pass validation with this new thing in config?


model.set_sorter()?

The set_sorter() method, is that required? Somehow Qt itself knows how to sort by that column on the SortRole even without having this sorter stored? So is there a way we could rely on that original behavior?

Like could we add the prefix of the group (0 or 1) to super(SubsetsModel, self).data(index, role)? For example:

    return prefix + super(SubsetsModel, self).data(index, role)

So that we don't have to somehow go through the "Sorter" to get the right column and query the data of it ourselves. I wonder how Qt knows what column it needs to return in that case? Or is this only the case because you are setting the Sort Role explicitly?

Somehow the assigning of the sorter feels off. Can this work simpler by avoiding it?

davidlatwe commented 5 years ago

Sorry, forgot to push the latest commit after leaving the comment 0.0

davidlatwe commented 5 years ago

So that we don't have to somehow go through the "Sorter" to get the right column and query the data of it ourselves.

Oh no, the reason why I need that was to query for which direction it's sorting (here). And to provide the right prefix for fixing groups on top also not to be affected whether it's ascending or descending.

Just pushed the update, and it's a bit more complex then the time you commented. :P

Edit:

I could not find other way to get the current sortOrder, nor to make some of the item not sortable after the model is set. Also the feature requires not to sort group with other groups by clicking the on columns. Maybe I missed some keywords ?

davidlatwe commented 5 years ago

Does this mean the project schema needs a new version/update? Or does it still pass validation with this new thing in config?

I haven't test it ! Those data were all directly write into the database currently. Will have a look tomorrow :)

BigRoy commented 5 years ago

I could not find other way to get the current sortOrder, nor to make some of the item not sortable after the model is set. Also the feature requires not to sort group with other groups by clicking the on columns. Maybe I missed some keywords ?

I see. I think the idea is to actually adjust the Sorter itself and ignore it inside the model. ;) So the Sorter model decides completely what to do. However, of course the model could include the order data so it's collected solely once from the database. (As I'm not sure how the sorter proxy would collect data like that only once - I think it should be able to do so too though)

davidlatwe commented 5 years ago

Hehe, I think I've figured out a way to implement this in a cleaner way ! I'll update shortly :)

davidlatwe commented 5 years ago

Ok, I believe this is better than previous implementation :) No weird looking code now.

BigRoy commented 5 years ago

Thanks for the update! I think we can slim it down even further, I'll try to pull your version and see if my idea could work out. Not sure if I get to it today though. :)

davidlatwe commented 5 years ago

Welcome to give it a try :D

davidlatwe commented 5 years ago

Does this mean the project schema needs a new version/update? Or does it still pass validation with this new thing in config?

Turns out the project config schema indeed requires an update. Added !

davidlatwe commented 5 years ago

Added one more commit for command avalon --save to include groups when saving to database.

davidlatwe commented 5 years ago

The test has been fixed :D @BigRoy , were you able to give it a try ?

davidlatwe commented 5 years ago

Subset grouping dialog

By pressing Ctrl + G, Loader GUI will pop-up a dialog for inputting group name and group or ungroup selected subsets.

This provides a convenient way to manage subset groups right in Loader GUI.

loader-subset-grouping

Note

BigRoy commented 5 years ago

By pressing Ctrl + G, Loader GUI will pop-up a dialog for inputting group name and group or ungroup selected subsets.

Sweet. Nice feature. Only thing I am slightly afraid of is... should anyone be allowed to do this? Or should this be a specific "admin" mode thing? I'm a bit scared of anyone on the team being able to alter the "subsets" in the database like that.

Any opinions @mottosso @tokejepsen @mkolar?

davidlatwe commented 5 years ago

Only thing I am slightly afraid of is... should anyone be allowed to do this? Or should this be a specific "admin" mode thing? I'm a bit scared of anyone on the team being able to alter the "subsets" in the database like that.

Good point.

How about adding a validation at here, and the validation will be implemented in each avalon-config.

So the code would be like:

def show_grouping_dialog(self):
    subsets = self.data["model"]["subsets"]
    selected = subsets.selected_subsets()
    if not selected:
        self.echo("No selected subset.")
        return

    config = api.registered_config()
    validate = getattr(config, "subset_grouping_validation", None)
    # Also take selected subsets into calculation
    if validate and not validate(selected):
        self.echo("Can not group/ungroup selected subsets.")
        return

    dialog = SubsetGroupingDialog(items=selected, parent=self)
    dialog.grouped.connect(self._assetschanged)
    dialog.show()
tokejepsen commented 5 years ago

I agree with @BigRoy that it needs to be an admin thing. Should also state clearly why the user cant group/ungroup. Something like User validation failed. Contact your administrator.

Also dont know how common storing variables on the config is.

BigRoy commented 5 years ago

How about adding a validation at here, and the validation will be implemented in each avalon-config.

Preferrably I think, once we go down that line it's time to implement some sort of "authentication level". Maybe Avalon core should just have something in it's api like:

avalon.api.get_user_rights()

Or get_user_level or get_user_permissions or get_user_role or get_user_authentication. Then the tools in core could just always fall back to that.

For example:

role = api.get_user_role()
if role < api.roles.ADMIN:
    # less than admin, so disallow
    return
# allowed
print("YAY!")

And we allow for some default roles, like:

roles.ADMIN = 5
roles.MANAGER = 4
roles.PRODUCER = 3
roles.ARTIST = 1

Etc?

Then any pipeline can set the current permissions, even when using an external "login"... e.g:

api.set_user_role(api.roles.ADMIN)

Of course we can complicate it as much as we want - however I do have little experience in the field of "managing permissions" and how customizable it should be for the api to be sufficient.

However, this should become it's own "Permissions/User level of control issue" - so let's make sure that's open and an active discussion. Then for now I'm fine with two ways:


Or do we see a simpler way?

tokejepsen commented 5 years ago

Either include it now (as it is) with this PR and then later patch it to allow "permissions" to be set.

I'm thinking this might be the best option.

Further to the implementation of permissions, it could also just be a simple environment variable to enable it.

davidlatwe commented 5 years ago

How about option 3:

davidlatwe commented 5 years ago

Preferrably I think, once we go down that line it's time to implement some sort of "authentication level".

Yes, I think Avalon would need this. Once we implemented this in the api, should make us a bit easier to accept other feature that is convenient but dangers.

BigRoy commented 5 years ago

I've started to give this a test run - and it's pretty sweet. Nice work @davidlatwe .

However, I did notice some issues.

Additionally, I'd love one other minor feature - the possibility to disable the grouping. I'm thinking of adding a Group checkbox to the right of the "Filter Subsets" line edit. What do you think?

Other than that, really amazing work :D

davidlatwe commented 5 years ago

Thanks for testing out @BigRoy !!

I will have a closer look on those issues tomorrow and see if I can resolve them :D

Additionally, I'd love one other minor feature - the possibility to disable the grouping. I'm thinking of adding a Group checkbox to the right of the "Filter Subsets" line edit. What do you think?

I think this would be useful, let's do this.

davidlatwe commented 5 years ago

Okay, I think I have solved them all !

davidlatwe commented 5 years ago

Here's a quick preview:

loader-subset-grouping-fix

BigRoy commented 5 years ago

That looks amazingly smooth @davidlatwe - perfect work!

BigRoy commented 5 years ago

Merge?

davidlatwe commented 5 years ago

Merge !