DISCUSSION, DO NOT MERGE: Separate out Models, Widgets and Delegates #414
Conversation
…m tools and necessary parts
…from tools and necessary parts
…om tools and necessary parts
| mode = selection_model.Select | selection_model.Rows | ||
| for index in lib._iter_model_rows(self.proxy, | ||
| column=0, | ||
| include_root=False): |
There was a problem hiding this comment.
continuation line under-indented for visual indent
| # Select | ||
| mode = selection_model.Select | selection_model.Rows | ||
| for index in lib._iter_model_rows(self.proxy, | ||
| column=0, |
There was a problem hiding this comment.
continuation line under-indented for visual indent
| role=self.model.ObjectIdRole): | ||
| with lib.preserve_selection(self.view, | ||
| column=0, | ||
| role=self.model.ObjectIdRole): |
There was a problem hiding this comment.
continuation line under-indented for visual indent
| column=0, | ||
| role=self.model.ObjectIdRole): | ||
| with lib.preserve_selection(self.view, | ||
| column=0, |
There was a problem hiding this comment.
continuation line under-indented for visual indent
| silo = self.get_current_silo() | ||
| with lib.preserve_expanded_rows(self.view, | ||
| column=0, | ||
| role=self.model.ObjectIdRole): |
There was a problem hiding this comment.
continuation line under-indented for visual indent
avalon/tools/_models/model_tasks.py
Outdated
| super(TasksModel, self).__init__() | ||
| self._num_assets = 0 | ||
| self._icons = { | ||
| "__default__": qtawesome.icon("fa.male", color=style.colors.default) |
avalon/tools/_models/model_assets.py
Outdated
|
|
||
| if role == QtCore.Qt.ForegroundRole: # font color | ||
| if "deprecated" in node.get("tags", []): | ||
| return QtGui.QColor(style.colors.light).darker(250) |
avalon/tools/_models/model_assets.py
Outdated
| except Exception as exception: | ||
| # Log an error message instead of erroring out completely | ||
| # when the icon couldn't be created (e.g. invalid name) | ||
| log.error(exception) |
avalon/tools/_models/model_assets.py
Outdated
|
|
||
| # Make the color darker when the asset is deprecated | ||
| if node.get("deprecated", False): | ||
| color = QtGui.QColor(color).darker(250) |
There was a problem hiding this comment.
This sounds dangerous, are we missing an import?
avalon/tools/_models/__init__.py
Outdated
| @@ -0,0 +1,50 @@ | |||
| from ...vendor import Qt | |||
| from Qt import QtCore, QtWidgets, QtGui | |||
mkolar
changed the title
|
We need to fix Hounds OCD remarks :) Also modules need to be renamed to conform to MVC naming conventions Model: Singular
Route: Plural |
Note that a lot of them actually seem actual issues (if they are correct) like missing imports and logger Plus having fixed all of them is going to make it all even so much neater! 💃 I'm not too fond of the Other than that, very nice work guys! 🥇 |
|
Those missing imports or variable definitions are my job. Not enough testing... :(
|
| mode = selection_model.Select | selection_model.Rows | ||
| for index in lib._iter_model_rows(self.proxy, | ||
| column=0, | ||
| include_root=False): |
There was a problem hiding this comment.
continuation line under-indented for visual indent
| # Select | ||
| mode = selection_model.Select | selection_model.Rows | ||
| for index in lib._iter_model_rows(self.proxy, | ||
| column=0, |
There was a problem hiding this comment.
continuation line under-indented for visual indent
|
How about we make a subfolder in the 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.
|
|
Like the location of the gui components, but maybe people should interact with them through the api? |
I like they live under Further more, maybe the |
|
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
do you mean inside tools or outside? |
Something short, concise and clear would be perfect! How about And, yes. Let's put in |
|
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...) |
I think it's mostly imported as Anyway, to me It's personal preference so pick whatever works out best for most I guess. I will jump line once it's decided. 👍 |
|
In that case
Yes, they are imported in init.py (both ways are possible to use) |
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 |
|
Meant inside the tools. So anywhere in avalon you would Its more to with exposing the components that are ready for public use, similar to how |
|
@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. |
|
Hound is ok.
I thought about this and true is that I want to have visible separated imports from different files so |
|
Now it seems to be everything alright |
|
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 |
|
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. 👍 |
|
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 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? |
|
I've changed paths in both
EDIT: changed them too |
…e_data paths in setup.py
There was a problem hiding this comment.
Hi all,
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. Such that..
- We can clearly identify the problem
- We can discuss alternatives
- If no alternative solves the identified problem, then have a look at whether it can be implemented
In short, there is far too many things going on in this PR that are subjective, in particular names and folder hierarchy.
If I may, I'd like to go back to the introduction and tackle the problems, before diving deeper into the 100+ (!) file changes.
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 kno[t] of dependencies.
For completeness, here's the current layout.
| Folders | Files |
|---|---|
 |
 |
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.
One of the things that broke this design initially, and what may have been cause for this PR to begin with (let me know if so) was cbloader and friends, that did things like this. I remember we talked about that during its inception and that it was intended to change, but not like this.
I'd like to bring our attention to the old Loader. If we put ourselves in the shoes of someone inspecting our code or thinking about contributing, or being an actual user and having some issue with that Loader, it's immediately clear where to look and where to make changes. It's clear that making a change here only affects this Loader. If a user wanted a second Loader, he could copy this file and make modifications to an entirely new one.
That's modularity.
Now, based on the existence of this PR, I take it that there are parts of the codebase you find a need to duplicate and that it complicates maintenance.
Which part have you found most problematic? What initially inspired this refactoring? Preferably I'd like us to find a common ground before we move forwards.
|
@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 |
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.
yes. hence the discussion above.
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.
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. |
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.
I've been thinking about how to articulate (6) and came up with this.
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).
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. |
mottosso
changed the title
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
good, let's discuss particular implementation.
combining classes in less files is not a problem for me. Keeps the functionality so why not.
I have no issues with getting rid o the
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. |
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. ;) |
|
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. |
|
@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. |
|
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? |
|
As @mkolar pointed out; #414 (comment)
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. |
|
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:
This PR fails at all of these points, hence it is closed. |
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.