search

getavalon / core

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

DISCUSSION, DO NOT MERGE: Separate out Models, Widgets and Delegates #414

Closed mkolar closed 5 years ago

mkolar commented 5 years ago

This PR simplifies working with individual widgets, models and delegates in GUIs across avalon.

Before this each tool had it's own widgets that were a bit randomly shared across multiple tools and it all became a know of dependencies.

We have separated out all parts that are being reused in multiple places for easier maintenance and development.

mkolar commented 5 years ago

We need to fix Hounds OCD remarks :)

Also modules need to be renamed to conform to MVC naming conventions

Model: Singular View: Plural Controller:

Route: Plural

BigRoy commented 5 years ago

We need to fix Hounds OCD remarks :)

Note that a lot of them actually seem actual issues (if they are correct) like missing imports and logger log variables. Those definitely need fixing then.

Plus having fixed all of them is going to make it all even so much neater! 💃

I'm not too fond of the _ underscore in the widgets, models folders, etc. Can we restructure this in a logical way that we can avoid that? It makes it seem almost as if no-one should touch these and that they are 100% internal only. Whereas it would be amazing if you could just grab the Asset Widget and use it in your own studio tools. What's everyone else's opinion on that?

Other than that, very nice work guys! 🥇

iLLiCiTiT commented 5 years ago

Those missing imports or variable definitions are my job. Not enough testing... :(

_ underscore was a neater solution how to split "tools" and "parts to use". It is a mess in tools folder without underscores - can be easily changed.

mkolar commented 5 years ago

How about we make a subfolder in the tools called guicomponents (or uicomponents)?

that way all the building blocks could stay together, their names could stay clean and it's quite clear that they are ok to be used in building new GUI.

It could even be one level up. so avalon would have folder with all the building blocks and a folder with all the tools that use them.

image

image

tokejepsen commented 5 years ago

Like the location of the gui components, but maybe people should interact with them through the api?

davidlatwe commented 5 years ago

How about we make a subfolder in the tools called guicomponents (or uicomponents)?

I like they live under tools, but how about naming it base ? So it will be on top via the alphabetical order and it's shorter.

Further more, maybe the style could also be pushed into that module ?

mkolar commented 5 years ago

Style could definitely be pushed to that module.

Naming it base doesn't solve the ordering properly I think. As soon as someone makes a tool called alligner or amaretto or similar, We'll be having the same conversation again. Having them with underscore would solve that and adding them to api, would double solve it :)

Like the location of the gui components, but maybe people should interact with them through the api?

do you mean inside tools or outside?

BigRoy commented 5 years ago

I like they live under tools, but how about naming it base ?

Something short, concise and clear would be perfect!

How about avalon.ui? :)

And, yes. Let's put in avalon.ui.style I suppose too.

iLLiCiTiT commented 5 years ago

I personally do not like short names because it is difficult to find where they are used. (try to find where is used avalon.io in code...)

BigRoy commented 5 years ago

(try to find where is used avalon.io in code...)

I think it's mostly imported as from avalon import io haha - shortest for the win!

Anyway, to me guicomponents sounds overly verbose and doesn't explain more than ui or gui. Fine either way, I'm just afraid of getting long imports like avalon.guicomponents.models.model_asset where to me avalon.gui.models.model_asset tells me just as much. (or avalon.gui.models.TreeModel since I noticed you imported them to the __init__.py in models)

It's personal preference so pick whatever works out best for most I guess. I will jump line once it's decided. 👍

iLLiCiTiT commented 5 years ago

In that case gui is much better then ui (one extra letter :) ). So the possible solution is move them to ~/avalon/gui/ (as well as style). Questions in my head:

or avalon.gui.models.TreeModel since I noticed you imported them to the init.py in models

Yes, they are imported in init.py (both ways are possible to use)

mkolar commented 5 years ago

I personally do not like short names because it is difficult to find where

I agree with this. Super short names are taking clarity away and replace it with a false sense of optimization :)

It's not like we're writing those imports 20 times a day. But later on they are much much easier to find and reformat if need be.

I can live with gui though. just my 2 cents

BigRoy commented 5 years ago

  1. These should be relative imports! So from ...gui.models import AssetModel

  2. A lib.py for things reused multiple times across the submodules of avalon.gui makes sense to me. Let's do it if it cleans things up, yes.

tokejepsen commented 5 years ago

do you mean inside tools or outside?

Meant inside the tools. So anywhere in avalon you would from avalon.api import AssetModel. Seems like avalon.gui could turn into a form of api just for gui components though.

Its more to with exposing the components that are ready for public use, similar to how api lets people know what is safe to use.

davidlatwe commented 5 years ago

avalon.gui +1 :D

BigRoy commented 5 years ago

Maybe somewhat unrelated:

iLLiCiTiT commented 5 years ago

Did the Travis CI fail because it had no internet connection, or what?

It looks like, Yes

So anywhere in avalon you would from avalon.api import AssetModel

Is better to import like from .gui.models import AssetModel or from .gui import AssetModel?

tokejepsen commented 5 years ago

Is better to import like from .gui.models import AssetModel or from .gui import AssetModel?

I would argue that from .gui import AssetModel is best. This way it'll be very clear whether there are conflicting naming in the models/widgets/delegates. I know it does not matter about having the same name in models/widgets/delegates, but it would help clarity.

iLLiCiTiT commented 5 years ago

Can someone please check the code? This is the first version of what it might look like.

BigRoy commented 5 years ago

Is better to import like from .gui.models import AssetModel or from .gui import AssetModel?

My vote again would be .gui.models import AssetModel as it's more explicit. See why I like short names? ;) Because I tend to type them out. 😄

Plus it means importing import avalon.gui.style does not require to import all the delegates, models, widgets, etc. :) - Maybe a minor optimization, but just makes most sense to me like that. Aren't there any PEP rules for these kind things?

@mottosso usually has a very strict eye for these things. Any input?

BigRoy commented 5 years ago

@iLLiCiTiT admittedly this Github view here is quite confusing... are all the Hound messages resolved now, or are they still open? I'd love to see this PR cleaned up, tested and merged soon so I can resolve some other PRs I'd like to do.

iLLiCiTiT commented 5 years ago

Hound is ok.

My vote again would be .gui.models import AssetModel as it's more explicit.

I thought about this and true is that I want to have visible separated imports from different files so .gui.models import AssetModel won for me. If anyone disagree speak now(until afternoon) or forever hold your peace.

iLLiCiTiT commented 5 years ago

Now it seems to be everything alright

mkolar commented 5 years ago

Yes all the hounds seem to be resolved. I'm happy with the state of this and we have more PRs coming along that are dependent on this too. So merge at will

mkolar commented 5 years ago

The conflict it's seeing is a bit strange though. when we tried to merge it in the office it went smoothly without any issues. Plus the file that is supposedly in conflict has actually been deleted.

Any ideas?

BigRoy commented 5 years ago

The conflict it's seeing is a bit strange though. when we tried to merge it in the office it went smoothly without any issues. Plus the file that is supposedly in conflict has actually been deleted.

Any ideas?

Merge current master into your branch locally, then check conflicts/changes, then push to your branch. It should resolve the conflicts that Github is seeing.

Also, quickly try and run each tool to be sure it's working as expected. Including the recent "unified attributes" functionality that is merged. So for example:

Just to be sure. 👍

iLLiCiTiT commented 5 years ago

THAT'S THE ISSUE! "unified attributes" is merged already?

BigRoy commented 5 years ago

THAT'S THE ISSUE! "unified attributes" is merged already?

Yes, @mkolar pulled the trigger.

Why is the Travis CI failing on PyQt4 + PyQt5 stuff of the fontawesome stuff. @mottosso do you know how to potentially avoid that?

Ah, I see. We should be excluding them somewhere... but I'm not sure which one it is:

I think we should actually be updating both?

iLLiCiTiT commented 5 years ago

I've changed paths in both setup.py and run_maya_tests.py and have additional question:

EDIT: changed them too

BigRoy commented 5 years ago

@mottosso in particular it's the reuse of widgets and models, e.g. asset widget and tasks widget. Similarly the styling of all tools, they share the same style, etc. Also version delegate is reused across multiple tools.

With this PR it exactly makes the tools in such a way that you can delete one. ;) They just now use some reusable widgets, delegates, etc. from gui

mkolar commented 5 years ago

I just spotted this PR (have had notifications muted), and though I appreciate the initiative, this is the kind of PR we should discuss ahead of actually implementing it.

we had to implement it on our side, otherwise it wasn't manageable to keep up with some client requests. Now we're suggesting PR back, and discussing it. Sometime we have to move faster, than open source project allow unfortunately.

In short, there is far too many things going on in this PR that are subjective, in particular names and folder hierarchy.

yes. hence the discussion above.

The key to the original design is modularity - that whenever you reach outside of your own module for anything, it should be as shallow as possible. That part of the implementation could get ripped out and replaced whilst affecting as little as possible.

that's nice but it wasn't true and it is not just cb loader. Multiple tools cross reference widgets and other components from module to module, this PR is fixing that issue.

It's clear that making a change here only affects this Loader.

Our issue was on the opposite front. Every time we wanted to implement improvement or change we had to spend a lot of fixing in many places, rather fixing the widget that's responsible and have it propagated. Great example is Asset (hierarchy) browser. It it is used in project manager, loader, context switcher and should behave the same in all. Fixing bugs on that many places is not only counter productive but also introduces inconsistencies.

Separating key building blocks out and making them available to any gui tools should actually substantially lower the barrier for people wanting to make new GUIs and tools, as they can re-use more code and keep the look and functionality consistent.

mottosso commented 5 years ago

yes. hence the discussion above.

Ah, I wasn't sure, because it reads to me as though you would like this PR merged as-is. It's fine to submit PRs as topics for discussion, but next time please mention it somewhere. Updating the title to reflect this.

Next what I'd like to see if specifically what the problem is. So far it reads to me as bikeshedding. So, step 1: what's the problem?.

Next, I spot patterns I'd like us to try and avoid.

  1. One file per class, e.g. avalon.gui.models.proxy_group_filter.py, this isn't Java
  2. Single-use modules, e.g. see above, needless imports does nobody any good
  3. Duplicate namespaces, e.g. avalon.gui.models.model_subset, namespaces are good
  4. No namespace, e.g. avalon.gui.views.view_assets.py, namespaces are good
  5. Overly global access, e.g. avalon.gui is only used by tools (?), if so should be located under tools.
  6. Group by type, rather than relevance.

I've been thinking about how to articulate (6) and came up with this.

image

Sometimes, it makes sense to group things by type, like putting clothes in a cabinet. Because when you're in the process of picking clothes, it makes sense to have some oversight over all possible options, since there aren't many and there is typically only one goal, one setting, in which you pick out a set of clothes (barring any rare celebratory or home-alone occasions).

image

Other times, it makes sense to think of organisation like a flat, each room having its own cabinet, sometimes even more than one per room.

If you were looking to get dressed, it would be cumbersome to keep pants in one room, shirts in another and socks in the basement. And likewise if you were watching a movie, even though the TV is on a table, it would be cumbersome having all tables in the same place, just because they're tables.

There's a place for both, just as we've got both rooms and cabinets in our homes, and the key is relevance. When you're in the living room, you want living room-related things. When you're in the bathroom, bathroom-related things.

With that in mind, if the problem you would like to see solve is greater re-use, then let's have a look at which parts are actually re-used and whether the cost of the resulting tight coupling outweighs the benefit of reuse.

Finally, it's quite presumptious to submit a PR with "cleanup" in the title. It suggests there was a mess before, which isn't very nice and gets everyone off on the wrong foot.

mkolar commented 5 years ago

Ah, I wasn't sure, because it reads to me as though you would like this PR merged as-is. It's fine to submit PRs as topics for discussion, but next time please mention it somewhere. Updating the title to reflect this.

yes we want this to be merged and we've worked with all the direct feedback that was discussed here to come to a good result. Now we have feedback from you as well, we can keep tweaking until it's mergeable

Next, I spot patterns I'd like us to try and avoid.

good, let's discuss particular implementation.

  • One file per class, e.g. avalon.gui.models.proxy_group_filter.py, this isn't Java
  • Single-use modules, e.g. see above, needless imports does nobody any good

combining classes in less files is not a problem for me. Keeps the functionality so why not.

  • Duplicate namespaces, e.g. avalon.gui.models.model_subset, namespaces are good
  • No namespace, e.g. avalon.gui.views.view_assets.py, namespaces are good

I have no issues with getting rid o the model_ view_ and such from module names.

  • Overly global access, e.g. avalon.gui is only used by tools (?), if so should be located under tools.

No problem with that personally either. I believe that has been discussed above. We have it like that internally and its a matter of a half hour to change that.

My point is that direct discussion about implementation is good and we're happy to accommodate these comments.

mkolar commented 5 years ago

Finally, it's quite presumptious to submit a PR with "cleanup" in the title. It suggests there was a mess before, which isn't very nice and gets everyone off on the wrong foot.

Just to clarify here. The PR title didn't have cleanup in the name. The branch did and if was referring to cleaning up rubbish from our fork to submit a clean PR. ;)

mottosso commented 5 years ago

Ok, I still haven't seen mention of any problem, which leads me to believe this is in fact bikeshedding. I'm closing this in favour of a new PR or issue with clearly stated problem.

tokejepsen commented 5 years ago

@mottosso I dont think closing this is very good for development. The problem is that the widgets are becoming unmanageable, which this PR aims to solve.

mottosso commented 5 years ago

IMO if the time spent trying to sort this PR out is spent on a solvable issue, I think we will get more bang for our buck. "Unmanagable" is too broad of a problem. What specifically cannot be managed, and what do you mean by manage? Is there a model used by multiple views? That's great, let's centralise that?

tokejepsen commented 5 years ago

As @mkolar pointed out; https://github.com/getavalon/core/pull/414#issuecomment-520362793

Great example is Asset (hierarchy) browser. It it is used in project manager, loader, context switcher and should behave the same in all. Fixing bugs on that many places is not only counter productive but also introduces inconsistencies.

But instead of just looking at the Asset (hierarchy) browser, the Pype team took the initiative to collect all the GUI code. I think this is a great initiative, cause not only does it solve the issue of the Asset (hierarchy) browser being difficult to manage, but it will make future development of tools easier.

mottosso commented 5 years ago

Toke, it's too vague, you should know this. Have you looked at the code? Which part do you find has the greatest impact on making "future development of tools easier"? And is there nothing in here you think does the opposite? I wouldn't blame you for not having an answer to that, because the PR is too large. It's counterproductive to have a conversation about and is frankly wasting time we could be spending on something productive.

For future PRs, this should come as no surprise:

  1. Keep it focused, minimal
  2. State the problem to be solved and why you need it solved
  3. Illustrate the problem, preferably with a minimal example
  4. Demonstrate how the PR solves the problem

This PR fails at all of these points, hence it is closed.