Add CONVENTION.md #437

mottosso · 2019-08-24T12:45:58Z

Related to #436

This PR contains a set of guidelines for writing graphical tools for Avalon. It's a starting point, along with an example program of what all guidelines look like together. It is meant as a reference for how to write tools that is..

Consistent (with the world, but foremost within Avalon)
Scalable (without hacks)
Resilient to user error (i.e. handle exceptions, provide messages)
Resilient to developer error (i.e. work when parts are broken, "hop on one leg")

CONVENTION.md

The example implements some of the guidelines at the moment, with the idea of being both textual and codified reference of the guidelines (it's a "guideline browser").

It's currently missing:

Multiple models, why, and how to deal with that
Asynchronousity, threading
Reference to which guidelines are implemented where (e.g. E3, S5 etc.)
External resources, like images (I noticed Avalon does not yet use any!)
CSS

python -m avalon.tools.convention \path\to\avalon-core\CONVENTION.md

More examples include..

Except these guidelines are a superset of what those were built with, an improvement.

As a guideline to the guidelines, let's refer to..

http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines

For #436, I'd like for these guidelines to apply, but they're not perfect. What I'd like you to do next is have a look at the example program and see how you can improve it. It'll be the baseline for any future application, and what we'll refer back to for guidance on updates to existing GUIs or the creation of new GUIs. It'll be a test-case for the written English, to ensure that everything actually works together and isn't just pretty words. It should be short, yet complete and functional. It doesn't need to accomplish anything useful, it's only purpose is to excercise the guidelines. Similar to how the pyblish_qml --demo flag operates, a system stress-test.

Missing

How do we deal with multiple models? As mentioned in #436, we'll try and merge the controller (in Allzpark/Pyblish terminology) with the model, making the model both the brain and data container. But the model governs things like row and column count, and different views on different sets of data may/should have different counts. In this case, it's somewhat convenient that the second "view" - the text browser - doesn't have an official "model". But if it did, how can it share a model if it only had a single item, a single row, single column? :S

davidlatwe · 2019-08-24T13:53:25Z

But the model governs things like row and column count, and different views on different sets of data may/should have different counts. In this case, it's somewhat convenient that the second "view" - the text browser - doesn't have an official "model". But if it did, how can it share a model if it only had a single item, a single row, single column?

Happens to have an example on this one :O (should fit this context I think ?)

While I was working on the Avalon Uploader for my studio, I need two QTreeView to show different perspective (staging & uploading) on one model. And I found that it actually able to let each view has different column count. Not sure if this implementation more like a hack, at least it looks and works logical to me :P

Edit:
I think the trick to share model between views was how to manage the data Role.

mottosso · 2019-08-24T14:11:11Z

I also thought of that; of making one proxy model per view. Even those that don't do anything fancy, like sorting of filtering.


     ____________________
    |                    |
    |       model        |
    |____________________|
     /        |        \
 ___/____  ___|____  ___\____
|        ||        ||        |
| proxyA || proxyB || proxyC |
|________||________||________|
    |         |         |
 ___|____  ___|____  ___|____
|        ||        ||        |
| viewA  || viewB  || viewC  |
|________||________||________|

And I think that makes sense. The proxy would be able to provide more specialised data() and setData, along with unique columns and such. They may even be able to act as more specialised business logic too? E.g.

editorModel.sortLines()
mainModel = editorModel.sourceModel()
mainModel.sortFiles()

davidlatwe · 2019-08-24T14:37:45Z

The proxy would be able to provide more specialised data() and setData, along with unique columns and such. They may even be able to act as more specialised business logic too?

Implement specialized business logic in proxies to provide processed data by that logic to view make sense to me, too.

But I am not sure about implementing the specialized setData in proxies, because isn't it just a proxy for viewing model differently ?

mottosso · 2019-08-24T15:01:55Z

But I am not sure about implementing the specialized setData

That's a good point.. doesn't feel quite right leaving this imbalance of one having data and setData, and the other only having data, but at first glance it makes sense.

BigRoy · 2019-08-24T20:21:06Z

Sorry for dropping this in here, but can we wait with merging this a day or five? I am back from vacation around Wednesday and hope to have some more time on Thursday to look into this PR and want to make sure I understand what this does and why it's there.

Being on the sidelines and reading only a glimpse of what this is doing my first hunch is to ask the question: "are we overdoing this?"

Also, the developer guide tool being in avalon.tools feels very odd to me? Can we keep that out of core or somewhere into a readme or alike? From the sidelines it feels very forced. Or do I have that feeling as I am missing some crucial part of the conversation?

To me it doesn't look like it's actually solving anything in practice but just putting in more lines of code and rules. Or will there be another PR refactoring the tools based on the ideas presented here?

Sorry of if it comes across as harsh or if I am misinterpreting things. It's definitely not intended that way, so see it as a question mostly.

mottosso · 2019-08-24T22:05:10Z

They're developer guides, like what you'd find for Blender, or Unreal, or Docker, or Go. I'd like to pass on what little knowledge I have to you, and bring out the knowledge in you in as structured of a manner as possible for all of our benefit and the benefit of the project. Avalon is growing up, and we're in a good spot to start thinking bigger than merely solving the problem right in front of us without considering the whole.

The goal is for us to both use these as reference, but also as knowledge base for when a new concept is introduced, like whether and how we should allow for and manage singletons, how we can queue tasks like in David's SFTP app, how to handle progress bars, threading and lots of concepts that run the risk of being reinvented or worse yet unconsidered for a given problem.

When we think of one idea that can be improved, we'll improve this documentation (and example). When we think of a new concept to a problem, this is where we'll document it (and implement it into the example). Then we'll have a lexicon of design patterns (and example code) to improve upon alongside the code, making Avalon scalable akin to bigger projects, like Blender, Unreal, Docker and Go.

At least, that's the dream!

davidlatwe · 2019-09-02T08:23:31Z

"Form is liberating.", so I vote "Yes" to these :)

BigRoy

Thanks for being patient and allowing me to take the time to go through this.

So I've left a review with some comments on things that stood out to me.

I think some rules here are helpful, but it does sound like it's laying out rules that I don't really see need for in practice which I hopefully all highlighted with the comments.
I'm still not a big fan of putting the convention tool in avalon.tools.convention - it's just cluttter inside there and I'd argue it's not an actual avalon tool but it is a coding example. It should not be there. It's adding lines of codes and complexity to the project that are not required. This might be a better fit for another repository, some sort of gist or maybe even a branch. It's documentation, it's a manual - not a tool. I'm seeing no reason to deploy this tool into a production environment. It's good to have it, but it's just in the wrong place. Any better ideas?

BigRoy · 2019-09-02T15:52:19Z

CONVENTION.md

+		some_button = QtWidgets.QPushButton("Hello")    # 1
+		this_layout = QtWidgets.QVBoxLayout(self)       #
+
+		thatWidget = QtWidgets.QWidget()                # 2


This seems like a bad example. Why are we using this a "Good" example for a compromise? I think the compromises should be made solely where they cannot be avoided, whereas this one easily can.

This example instead should showcase the line 79 example of mousePressEvent which must be named the same to work. That's the compromise we're making when using Qt. However, we would still force to go PEP08 for our variables, yes?

I would opt actually for Avalon tools using snake_case as that is the code guidelines for Avalon itself, PEP08. I think this mixture of mixedCase to snake_case would also just have us refactoring all sorts of things when an application becomes bigger or more elaborate and it suddenly has more "native Avalon". I think Avalon-wide we should just adhere to one standard as opposed to letting it differ based on amount of specific lines of code in a file.

This seems like a bad example. Why are we using this a "Good" example for a compromise? I think the compromises should be made solely where they cannot be avoided, whereas this one easily can.

What is a better example?

I would opt actually for Avalon tools using snake_case as that is the code guidelines for Avalon itself, PEP08.

For clarity, you are referring to "stand fast"? If not, how does your ideal case differ?

https://github.com/mottosso/avalon-core/blob/convention-guidelines/CONVENTION.md#stand-fast

Just I was referring to the Stand Fast code where all variables are PEP08 lowercase with underscores. I don't see a reason to mix the variable naming conventions inside of Avalon, if it's a "sometimes" thing it'll only make room for discussion/debating that should just have gotten a clear cut rule.

Just I was referring to the Stand Fast code where all variables are PEP08 lowercase with underscores. I don't see a reason to mix the variable naming conventions inside of Avalon, if it's a "sometimes" thing it'll only make room for discussion/debating that should just have gotten a clear cut rule.

But it's not just variables; it's methods too, and supporting functions. The example highlights that (1) if we want snake_case, we must still have mixedCase and (2) that's ugly.

Consistent use of ugly, non-standard mixedCase is greater than inconsistent use of a nice and standard use of snake_case.

But I can see how we'd get the question "what should I use where?", as "majority of code" changes over time. It could start out with mostly mixedCase and turn into mostly snake_case. Do we then refactor to one over the other? The guideline could be "anything widgets use mixedCase, everything else use snake_case".

Not 100% sure, anyone else have any thoughts here?

But it's not just variables; it's methods too, and supporting functions.

Is it? It's really just methods and functions, right? Or at least, the things you reference from other libraries? What I'm saying is that wherever you don't require to match exactly the mixedCase or any other standard, then we don't.

So yes, we do override mousePressEvent exactly as it is because otherwise it doesn't work. However, we do define our callback methods as on_accept or alike.

The rule then would be to always use snake_case unless the code wouldn't work when using snake_case.

Which means that all avalon code is snake_case but not where it triggers a command/function/method from a library that is not snake case. So it will solely be the library usage that will use its own standards which we can't influence anyway.

The good thing then is that the decision is made avalon-wide as opposed to per file.

I think we're saying the same thing.

If we use snake_case where we can, mixedCase elsewhere; that's inconsistent. That's what this bullet point addresses. The question is whether to do that, or to do something about it. What to do? That's what the three options are for; stand fast, give in, or compromise.

So the default is stand fast, and other options are just optional ?
I actually lean to all stand fast as default, since it’s most simple.

Yes, then I agree. It's stand fast that is the Good example. Also, the three examples and only one being the right one didn't stand out to me. How about just identifying them as Good, Bad and Bad similar to how you did with the others.

BigRoy · 2019-09-02T15:58:59Z

CONVENTION.md

+
+**Notes**
+
+- **S1** Widgets MUST are separated into `pages`, `panels` and `widgets`


Typo MUST are -> MUST be or are

BigRoy · 2019-09-02T15:59:35Z

CONVENTION.md

+            # Expose to CSS
+            widget.setObjectName(name)
+
+        	# Support for CSS


Indentation error?

BigRoy · 2019-09-02T16:01:00Z

CONVENTION.md

+
+    	...
+
+    	layout = QtWidgets.QHBoxLayout(panels["header"])


Are we sure it reads better to use panels["header"] than just header as a variable? It's not a standard practice I've seen before and it makes it longer and potentially harder to refactor.

Are we sure it reads better to use panels["header"] than just header as a variable?

Yeah, let's stick with that. It's something that has worked well in many differently sized UIs, and remains consistent and safe. The added bonus is that it also gives the reader a clear indication of what the name in CSS will be, and that when it's accessed from elsewhere - which you can expect it can - it'll be self._panels["header"] as opposed to.. self._header? What is that, a line of text? A widget, or what?

It's both fine for me. Just wondered whether others didn't feel this would turn into a clutter, including yourself. If not, fine with both to adapt as needed.

BigRoy · 2019-09-02T16:01:55Z

CONVENTION.md

+
+- **S10** Every widget MUST be given a unique name
+- **S11** Every widget MUST be stylable with CSS
+- **S12** Central widget MUST be a `QStackedWidget` to facilitate multiple pages 


This seems like an odd requirement? Small apps shouldn't need to have this, right? Seems to me that it's overly complicating apps that have require a single page only. Why would we force this?

This seems like an odd requirement? Small apps shouldn't need to have this, right? Seems to me that it's overly complicating apps that have require a single page only. Why would we force this?

It's only 1 extra line of code for not having to think about this choice and for encouraging additional pages being added; there are a lot of ways in which to add "pages" and without this it isn't clear which to use. Let's keep it.

BigRoy · 2019-09-02T16:04:23Z

CONVENTION.md

+
+**Notes**
+
+- **S20** Layouts MUST all be called `layout`; don't bother with unique names or maintaining reference to them


How does this work if you need to do layout.addLayout()?

You don't.

Every layout must have a widget.

Then maybe one of the notes should also be to avoid the use of layout.addLayout. Note that addLayout() is also discussed in the tips on layouting for Qt.

mottosso · 2019-09-03T06:55:56Z

Thanks Roy.

I'm still not a big fan of putting the convention tool in avalon.tools.convention - it's just cluttter inside there and I'd argue it's not an actual avalon tool but it is a coding example.

Sure it's not ideal, but I didn't see an issue with it. If you've got a better idea, now's the time.

BigRoy · 2019-09-03T08:09:08Z

Sure it's not ideal, but I didn't see an issue with it. If you've got a better idea, now's the time.

I think the best location where the code can be maintained in a single location would be another repository or a specific "contribution" branch. The last seems an odd use for a branch, so I think the first is still best. The CONTRIBUTION.md file could then just link to that repository with conventions and examples.

davidlatwe · 2019-09-03T08:50:42Z

I think the best location where the code can be maintained in a single location would be another repository

Hmmm, how about put it in core/conventions/gui ? I personally lean to not separating them in different repositories. Also I think it might still be changed, so would be better if they were all in same repository when one making a PR ?

Anyway, I think the most important is easy to maintain/access for developers. :P

BigRoy · 2019-09-03T09:05:00Z

Hmmm, how about put it in core/conventions/gui ? I personally lean to not separating them in different repositories. Also I think it might still be changed, so would be better if they were all in same repository when one making a PR ?

Imagine someone working on an older version of avalon and working with older convention examples from their local deployed version. I'd rather have it be easily distinguishable that it's the latest and greatest conventions, almost like a manual/documentation page. Anyway, I've made my arguments and I'll leave the decision open for Marcus or someone else.

mottosso · 2019-09-03T09:09:22Z

I personally lean to not separating them in different repositories.

Yes, unlike Pyblish, Avalon is a monorepo; everything goes into here. Otherwise we should have a conversation about whether tools and integrations warrant their own repos too, but had I done Pyblish all over again I would have made it a monorepo.

BigRoy · 2019-09-03T09:14:21Z

Yes, unlike Pyblish, Avalon is a monorepo; everything goes into here. Otherwise we should have a conversation about whether tools and integrations warrant their own repos too, but had I done Pyblish all over again I would have made it a monorepo.

I understand, but this particular conventions repository is not required for anything to work. It's just a code example. It should be part of documentation or something. Maybe a branch actually does make sense, similar to how gh-pages is used.

mottosso · 2019-09-03T09:33:49Z

It should be part of documentation or something

Documentation is also part of this repo, in the master branch. gh-pages is implementation detail for GitHub to publish it, it isn't something you clone or write into.

Maybe docs/ is a better place for it?

So the default is stand fast, and other options are just optional ?

I put the options there to cover all cases; to not only indicate which one to use, but also indicate what else where was and why those are less ideal. I put the little checkbox in Compromise as an indicator as to which one to use but it probably wasn't apparent enough.

Happy to stick with Stand Fast, I'll update the checkbox and point it out more explicitly.

BigRoy · 2019-09-03T10:32:54Z

Maybe docs/ is a better place for it?

To me that does make more sense, sorry if I'm making quite a bold and hard opposition. Feel free to ignore if you both feel different about it.

mottosso · 2019-09-04T13:33:31Z

Maybe docs/ is a better place for it?

Thinking about this, there is docs/ which is the api ref, but then there's https://github.com/getavalon/docs which is the user docs.

Would really like to merge these into here, so docs align with the version of Avalon, and so that PRs can be made to both together. And to make it more of a monorepo. That's separate, but means docs/ might not be the best place for this after all.

What we've got is a text document with associated executable code; if I could I would have embedded the Python into the markdown, and let us somehow run the markdown directly. But that's not possible, so the only reason for putting it elsewhere is technical mumbo jumbo. Still not 100% on where.

Add CONVENTION.md

e40eac9

mottosso added the development label Aug 24, 2019

BigRoy requested changes Sep 2, 2019

View reviewed changes

BigRoy mentioned this pull request Sep 4, 2019

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

Merged


		Notes

		- S1 Widgets MUST are separated into `pages`, `panels` and `widgets`


		Notes

		- S20 Layouts MUST all be called `layout`; don't bother with unique names or maintaining reference to them

Add CONVENTION.md #437

Are you sure you want to change the base?

Add CONVENTION.md #437

Conversation

mottosso commented Aug 24, 2019 • edited Loading

Missing

davidlatwe commented Aug 24, 2019 • edited Loading

mottosso commented Aug 24, 2019

davidlatwe commented Aug 24, 2019

mottosso commented Aug 24, 2019

BigRoy commented Aug 24, 2019

mottosso commented Aug 24, 2019

davidlatwe commented Sep 2, 2019

BigRoy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BigRoy Sep 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BigRoy Sep 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BigRoy Sep 3, 2019 • edited Loading

Choose a reason for hiding this comment

mottosso commented Sep 3, 2019

BigRoy commented Sep 3, 2019

davidlatwe commented Sep 3, 2019

BigRoy commented Sep 3, 2019

mottosso commented Sep 3, 2019

BigRoy commented Sep 3, 2019 • edited Loading

mottosso commented Sep 3, 2019

BigRoy commented Sep 3, 2019

mottosso commented Sep 4, 2019

mottosso commented Aug 24, 2019 •

edited

Loading

davidlatwe commented Aug 24, 2019 •

edited

Loading

BigRoy left a comment •

edited

Loading

BigRoy Sep 3, 2019 •

edited

Loading

BigRoy Sep 2, 2019 •

edited

Loading

BigRoy Sep 3, 2019 •

edited

Loading

BigRoy commented Sep 3, 2019 •

edited

Loading