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

Sections #58

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

Sections #58

wants to merge 5 commits into from

Conversation

BigRoy
Copy link
Member

@BigRoy BigRoy commented Aug 19, 2016

Experimental implementation for creating section headers as described in #21.

This implements a GroupByProxy that can group a (flat list) Model by a data role into a tree of sections and the original items.
Additionally this changes the QListView over to a QTreeView to retrieve the nesting functionality for the visuals.

@mottosso
Copy link
Member

Awesome!

Some things I found to think about, some obvious, some not.

  1. Instance sections show a list, when they should show the first ("primary") family.
  2. All was collapsed at start, with no hint about what is available; we'll need an indicator to communicate that sections are collapsible
  3. Which should be collapsed by default, if any? Why? Should we/how do we customise?
  4. Selecting tree items look odd, so we'll need to have a go with the CSS and likely the delegate
  5. Should each section feature a main progress indicator
  6. Should this indicator also be something one could tick to tick all optional items on/off?
  7. Ticking an item (via the space-key) causes a delay on some items, and throws an exception

@BigRoy
Copy link
Member Author

BigRoy commented Aug 19, 2016

Thanks @mottosso

Most sound obvious and they are good ideas. I was thinking also at the very least have something like a counter of amount of plugins in the header. Like Collectors (12), Validator(3) and something that visually would show the success/failure states, maybe even like a progress bar in the header.

  1. All was collapsed at start, with no hint about what is available; we'll need an indicator to communicate that sections are collapsible

A visual indicator (the toggle triangle) can be enabled by having setRootIsDecorated to True on the tree view. Yet I'm expecting delegates and styling will help along the way to visualize it the way we want.

The collapsed state is a tricky one. Basically you'll want to "expand" at certain processing times, because you can't expand by default (as far as I know). Since the data isn't present from the very beginning one can't just view.expandAll(). Setting the expanded states should be called at the times the data is present.

  1. Which should be collapsed by default, if any? Why? Should we/how do we customise?

With that information know I believe we know how far we need to push with 2.

  1. Ticking an item (via the space-key) causes a delay on some items, and throws an exception

This sounds related to that the instances can't be toggled yet, as that isn't implemented correctly from the change between List to Tree.

@mottosso
Copy link
Member

Basically you'll want to "expand" at certain processing times

This can happen as soon as reset() finishes.

Add FamilyGroupProxy to list only first family for instances
Fix checkbox updating by implementing setData on proxy
Always expandAll (for now)
Fix Proxy.is_header method
@BigRoy
Copy link
Member Author

BigRoy commented Aug 23, 2016

Some slight changes to get closer to our wanted result. Currently after collection and validation it looks like this:

pyblish_proxy

Changes:

  • Fix styling by implementing Section header delegate and a ItemAndSection delegate that draws both either depending on whether it's a header
  • Removing indentation in TreeView so clicking checkbox actually is in the right location
  • Implemented Proxy.setData which means now toggling checkboxes seems to be woking.
  • Always expands all after reset

Todo:

  • Find a way to connect signals from the source model to the proxy to update "automatically" when source model gets updated. I'd expected that to be done automatically when building a proxy, but it doesn't seem to do so.
  • Implement better way to handle expanding of the view
  • When resetting with the tree views open it will give many errors on retrieving data from indices in source that don't exit anymore, up until the point the model finishes resetting. The Proxy basically updates to late. Finding the right signal to connect to (preferrably to signals on the source model) should fix this completely.

@mottosso
Copy link
Member

Awesome!!

At first I was looking at the screenshot thinking it was pyblish-qml, and that you wanted to contrast the two.

@BigRoy
Copy link
Member Author

BigRoy commented Aug 23, 2016

At first I was looking at the screenshot thinking it was pyblish-qml, and that you wanted to contrast the two.

That's great. I just took the Item delegate and built a Section delegate using that as template and looked Qml to see what the result was there and tried to make it look similar. I guess so far so good.

Still need to fix up some things, just so few hours in a day especially aside of work. :)

@mkolar
Copy link
Member

mkolar commented Aug 31, 2016

So it seems like we might be running into an issue here. I've tested @BigRoy's code and it's only running with pyside which as we all know by now, has been dropped in maya 2017 in favour of PySide2.

From a quick look it might just be some scrabmled classes that were moved to different modules in PySide2, however I can't find proper documentation on these changes. I managed to get the UI to show in maya 2017, but it's not liking the QAbstractProxyModel class and keeps erroring out on that.

@mottosso
Copy link
Member

mottosso commented Sep 1, 2016

I've tested @BigRoy's code and it's only running with pyside

Have you also given PyQt5 a try? It should work with that as well. Though, like with Maya pre-2017, it will need to be a PyQt5 compiled for the version of Maya you are using.

I managed to get the UI to show in maya 2017, but it's not liking the QAbstractProxyModel class and keeps erroring out on that.

Looks like you've found another misplaced class in PySide2! I'm helping the guys getting these sorted out, luckily there are (1) less misplacement than PySide1 and (2) developers available to actually do something about it.

Till then, can you go here, and add this remapping section, along with this:

QtCore.QAbstractProxyModel = QtGui.QAbstractProxyModel

That should solve it.

@mkolar
Copy link
Member

mkolar commented Sep 1, 2016

My bad. Problem is somewhere else. QAbsractProxyModel is correctly in QtCore after multiple checks. However the treeview in pyblish-lite still doesn't get populated.

This time due to:

# Traceback (most recent call last):
#   File "K:\.core\dev\pyblish\pyblish-lite\pyblish_lite\tree.py", line 131, in rebuild
#     self.reset()
# AttributeError: 'PluginOrderGroupProxy' object has no attribute 'reset'

@mottosso
Copy link
Member

mottosso commented Sep 1, 2016

Linking to related issue in Qt.py

Looks like there's some incompatibility elsewhere. @BigRoy, are you able to take a look?

@BigRoy
Copy link
Member Author

BigRoy commented Sep 1, 2016

Sounds like there's a left-over call to reset() somewhere which might have needed to get refactored to rebuild(). @mkolar can you confirm that it is working correctly in 2016 for you?

It was in a somewhat early-test stage as I noticed some errors still popping up, but I think with the latest fixes to the proxy models those all moved away as it started working like any of the original models.

@BigRoy, are you able to take a look?

Not to investigate on short-term, but I'm keeping an eye on this conversation. ;) I'll try to investigate somewhere this weekend or early next week if I have the time.

@mottosso
Copy link
Member

mottosso commented Sep 1, 2016

Looking at the docs for QAbstractProxyModel, there doesn't seem to be a reset() method anymore.

In Qt 4 on the other hand, there is. So it's something we'll need to work around here.

@BigRoy
Copy link
Member Author

BigRoy commented Sep 1, 2016

In Qt 4 on the other hand, there is. So it's something we'll need to work around here.

Sounds like that's the root of this. Will have to investigate a bit on how this operates in newer Qt versions. Thanks for investigating!

@mkolar
Copy link
Member

mkolar commented Sep 1, 2016

Yeah. It works fine in maya 2016, 2016ext2 and nuke. Certainly the issue that @mottosso found with missing reset()

@BigRoy
Copy link
Member Author

BigRoy commented Sep 1, 2016

Looking at Qt5 docs I think it's the modelReset() method we want instead of reset() (actually also for Qt4, think I just took the wrong one there).

The self.reset() that is at the beginning of the rebuild method on the Proxy should be replaced with a call to self.modelReset() at the end of the rebuild method. (Note that modelReset was added in Qt 4.6, is the PySide version based on that one or older?)


Or actually no. I think it's beginResetModel() at the beginning and endResetModel() at the end of that method. Haha, I'll have a look whenever I have some time. (Don't have Maya 2017 on my hands, or PySide 2, so it's a bit hard to test actually)

As far as I can tell both of the above should work. Do you know the difference between those methods?

@mkolar
Copy link
Member

mkolar commented Sep 1, 2016

@BigRoy BINGO. I've tried using beginResetModel() and endResetModel() and it works in both maya 2017 and 2016.

@BigRoy
Copy link
Member Author

BigRoy commented Sep 1, 2016

Thank @mkolar!

Feel free to comment on the rest of the functionality and please test overall stability. Hope to get the goals straight to have this working as a stable implementation so we can start having a look at actually getting a good pull request going here, instead of the messy WIP state I started. 👍

@mottosso
Copy link
Member

mottosso commented Sep 1, 2016

instead of the messy WIP state I started.

Nonsense! This is an excellent way to do PR's. We'll simply merge once it's pretty, and discuss along the way.

Wip away!

@mottosso
Copy link
Member

I've tried digging into this, but found myself overwhelmed by the complexity at the moment.

@BigRoy do you think it would be possible to either (1) explain the core components of what makes this work so that I or someone could continue from where you left off or (2) simplify the code slightly to make it easier to understand?

@BigRoy
Copy link
Member Author

BigRoy commented Oct 11, 2016

I'll have another look at cleaning and simplifying this to the bare necessities! Or at least have a look at what is unclear in the current state.

@mottosso
Copy link
Member

Or at least have a look at what is unclear in the current state.

If it helps, I think the main struggle I had was to understand the relationship of the generated tree in the proxy and the original items. How the name is forwarded from this item to the view, for example.

@tokejepsen
Copy link
Member

Had another request for this from new users. Any progress on it?

Seems like this is close to a merge.

@mottosso
Copy link
Member

This ball is in @BigRoy's court.

@BigRoy
Copy link
Member Author

BigRoy commented May 22, 2017

Had another request for this from new users. Any progress on it?

Unfortunately I personally haven't found the time to continue working on this. The code should be close to a working version, albeit slightly quick and dirty as I recall it. If you've more time on your hands feel free to pull the PR to your own repo and refactor some of it... otherwise I hope to find some time soon, though looking at how long this has been open I'm not too confident I'll get into it soon, again.

@davidlatwe davidlatwe mentioned this pull request Nov 12, 2018
@BigRoy
Copy link
Member Author

BigRoy commented Feb 11, 2021

@mkolar are you using this feature? :) Just wondering.

@hannesdelbeke
Copy link
Contributor

sections are used in openpype's version of pyblish lite, the PR is still open #109
original vs openpype
image

moqodow pushed a commit to submarine-studio/pyblish-lite that referenced this pull request Feb 10, 2022
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.

5 participants