getavalon / core

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

Refactor avalon.tools to avoid cross-referencing imports between tools #440

Closed BigRoy closed 5 years ago

BigRoy commented 5 years ago

Note: This is a big PR as it refactors locations of models, views, delegates and functions. It's this big because unfortunately it needs to be.

Problems

  1. Different tools cross-referenced each other with imports from the other tools.
  2. The tree model's item is called Node, which is wrong terminology as Qt refers to QAbstractItemModel where it is an Item
  3. The family and group configs that are "refreshed" in loader.lib are actually caches reused across multiple tools. This is unclear from the variable and function naming. (This "global state" is a problem of its own, but this PR will not try to solve that as it would require even more changes.) It only solves the cross-referencing across tools.
  4. Some variables are mixed case like lowerCamelCase where they should be snake_case as per #437
  5. An AssetView class exists which purely exists for one-time use and only sets 3 small view tweaks - it's redundant complexity.
  6. The TreeModel defines an attribute called COLUMNS which should be lowercase columns.
  7. There is an unused model ExactMatchesFilterProxyModel which is redundant lines of code.

What's changed?

  1. All shared tool models, views, delegates, widgets and functions have been respectively refactored to avalon.tools.models, avalon.tools.views, avalon.tools.delegates, avalon.tools.widgets and avalon.tools.lib
  2. The Node is refactored to Item, including variables node and alike that I could find are refactored to item.
  3. This is still a global state, however the variables and functions now are clearly referring to it as a cache and simplified to the only way they are used. It's up to a following PR to move the caching to inside a model as opposed to a global state.
  4. Refactored to snake_case.
  5. The AssetView class has been removed and the related lines of code can now directly be found in the AssetWidget.
  6. TreeModel.COLUMNS refactored to TreeModel.Columns
  7. Remove ExactMatchesFilterProxyModel.

This is a work in progress PR and has not been tested yet. However I'm opening this to kickstart the discussion about any faulty choices I'm making. Plus, since it's a lengthy PR it gives others time to go through what is being changed.

I'll clearly state in a comment when I have been able to thoroughly test changes and feel it's in a good state for others to test too.


This PR acts as a replacement to #414 trying to do some similar things yet now along the new Contribution guidelines. Additionally it tries to describe the problems that we faced before and how they have been resolved.

BigRoy commented 5 years ago

With this last commit I checked out the pipeline features in Maya and ran through:

This now all worked so I consider this testable for others.

davidlatwe commented 5 years ago

Ah, backward compatibility, but do we need to do that for this kind of low level refactoring ?

I have some script/tools that, for example, used projectmanager.model.TreeModel. But since these doesn't count as part of api nor other higher level things, so maybe we won't need to support this ?

BigRoy commented 5 years ago

I have some script/tools that, for example, used projectmanager.model.TreeModel. But since these doesn't count as part of api nor other higher level things, so maybe we won't need to support this ?

Exactly, as those have never been exposed to any public facing API these changes are in the realm of "allowed to change". So, those don't need to be backwards compatible and you'll have to refactor in-house code that used it to change along. We had some internal tools referencing things too. ;)

Anyway, anything not exposed in the API is allowed to change without being fully backwards compatible. (e.g. a function name may behave differently, be renamed, moved, etc.). And the recommendation is not to reference those methods, but of course... you can if you really need to at your own risk, like now.

mottosso commented 5 years ago

I suppose that "Approved" should have been "Requesting changed", as I think we should address my comments before merging. I'll let @davidlatwe hit the Merge button for this PR on my behalf. Great work guys.

BigRoy commented 5 years ago

This should be ready to go if I understood @mottosso correctly here then there are now no open comments anymore as I refactored the columns to Columns which was the last remark.

I'd say, give it one other test run and feel free to merge @davidlatwe 🚀

Thanks all!

davidlatwe commented 5 years ago

Ah, I believe we should bump the version to 5.4.0, :wave: @BigRoy :) I will merge this PR and draft a ~pre-release~ release after verison bumped !

BigRoy commented 5 years ago

@davidlatwe this hasn't been merged yet. Do you want me to quickly bump version? :)

davidlatwe commented 5 years ago

Yes, please do ! It seems that I could not bump it myself into this PR :P