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

Loader GUI subset grouping #391

Merged
merged 28 commits into from
Jul 10, 2019
Merged

Conversation

davidlatwe
Copy link
Collaborator

@davidlatwe davidlatwe commented Jun 12, 2019

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
Copy link
Collaborator

BigRoy commented Jun 12, 2019

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
Copy link
Collaborator

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
Copy link
Collaborator Author

davidlatwe commented Jun 13, 2019

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

  • Subset group could be defined and storing in project document's config field
  • Groups are sortable and only sort with other groups in descending order

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
Copy link
Collaborator

BigRoy commented Jun 13, 2019

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
Copy link
Collaborator Author

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

@davidlatwe
Copy link
Collaborator Author

davidlatwe commented Jun 13, 2019

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
Copy link
Collaborator Author

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
Copy link
Collaborator

BigRoy commented Jun 14, 2019

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
Copy link
Collaborator Author

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

@davidlatwe
Copy link
Collaborator Author

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

@BigRoy
Copy link
Collaborator

BigRoy commented Jun 14, 2019

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
Copy link
Collaborator Author

Welcome to give it a try :D

@davidlatwe
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@davidlatwe
Copy link
Collaborator Author

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

@davidlatwe
Copy link
Collaborator Author

How about option 3:

  • Use AVALON_EARLY_ADOPTER to enable/disable this group by CTRL+G feature ?

@davidlatwe
Copy link
Collaborator Author

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
Copy link
Collaborator

BigRoy commented Jul 4, 2019

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

However, I did notice some issues.

  • Whenever you "filter" a family and nothing remains inside a Group - the Group remains visible.
    • For example, by default we have all "image sequences" filtered out in Maya by default. However, if I group image sequences per renderlayer I'll always see this empty group in Maya.
  • The same happens when you type a filter at the top bar - the empty Groups always remain visible, but should be gone.
  • When pressing "CTRL+G" when having the Loader open in Maya actually also runs the shortcut in Maya interface as if the Qt interface does not handle the key event as completed but lets it pass on to Maya.
  • Pressing Enter after typing the name for the group does not work. You need to accept it by pressing the button. (A bit annoying)
  • The sorting of the groups by name doesn't seem to work for me. The groups are randomly ordered.
  • Minor UX issue, not sure how to solve. By default now when expanding the group it often happens that the subset of the instance inside of it doesn't have it's name entirely readable (because the left column is resized on initialization only to the names with the closed groups. It's a bit annoying to have to resize the column constantly. Any ideas on how to fix?

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

davidlatwe commented Jul 8, 2019

Okay, I think I have solved them all !

  • Group remain visible in view even all of it's members has been filtered out
    Firstly implemented in 98d2f85.
    Fixed for older (before 5.10) Qt version in b9914db.
    And a bit improvement in 093a0d7.

  • "CTRL+G" key event keep passing to parent App
    Fixed in 8b868ef.

  • Apply grouping by pressing Enter
    Implemented in 63ac36a.

  • The groups are randomly ordered
    Should be fixed after 1af5e8d.

  • Column not resized to fit group members' content
    Fixed in 65940c3.
    With this implementation, all columns will auto resized to fit the content. But the size is fixed, user may not change it.

  • Auto expand group on text filtering
    Implemented in 66f678c.

  • The possibility to disable the grouping
    Implemented in c8a0468.

  • Minor fixes

@davidlatwe
Copy link
Collaborator Author

Here's a quick preview:

loader-subset-grouping-fix

@BigRoy
Copy link
Collaborator

BigRoy commented Jul 8, 2019

That looks amazingly smooth @davidlatwe - perfect work!

@BigRoy
Copy link
Collaborator

BigRoy commented Jul 10, 2019

Merge?

@davidlatwe
Copy link
Collaborator Author

Merge !

@davidlatwe davidlatwe merged commit 7842b09 into getavalon:master Jul 10, 2019
@davidlatwe davidlatwe deleted the loader-subsets branch July 10, 2019 10:02
@davidlatwe davidlatwe restored the loader-subsets branch July 10, 2019 10:39
@davidlatwe davidlatwe deleted the loader-subsets branch July 10, 2019 13:13
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