I don’t think anybody is talking about blindly applying those rules for every change. Use the common sense to see whether one point is applicable or not, as well as deciding how much into details it worth to go.
The way I see it is more of a checklist, which helps people to make sure presentation of their work is at a decent level. The goal here is to make code review more pleasant and not to be bureaucratic.
Here is an example of what I think matches my proposal: https://developer.blender.org/D6352. It’s not hard to write, but IMO gives a nice overview of the problem, the solution, alternatives and why I don’t choose them at this moment, shortcomings/limitations, and concrete points for the reviewers to discuss.
I always like to propose things for blender, and I usually work especially interface mockups, but it’s not something that programmers like to do. If any developer needs help for a concept of some feature to implement that tell me and I will help.
About the only thing I would add is that for anything that has a visual change I prefer to see both before and after captures to make it easier to compare. Not just a nice “after” image which forces me to track down that particular area in blender just to confirm how it looks now. That might seem a bit anal for otherwise really nice descriptions like in D6352 but it helps to make the proposal more self-contained.
IMO the big problem with the patch submission process is that the developers sometimes don’t see the patch or acknowledge that they have seen them even when you try contacting in other channels (blender.chat) you get no response.
It can be discouraging if you create a descriptive write up and you don’t know if anyone will look at your work.
Good design is a process and as such, I think the “think first, code later” approach has merit.
However, I feel it needs to be pointed out that no matter whether you’re an experienced hand or a fresh new face, the learning and exploration phase is about thinkering with different approaches and solutions to a problem (and may even lead to restating the problem several times).
It is only after one is well into the thinkering phase that one can begin to adopt a proper design mindset, which is to say that the design phases can be described as:
A developer who has experience with a given code base will have done lots of thinkering on it already, so their path through the “thinkering” and “consider trade-offs” phases is much quicker, because they will have already built up silent knowledge of the cul-de-sacs implicit in the project’s existing design.
In contrast, a new developer with no experience of the code base will need guidance to understand the inherent trade-offs and will need to follow the project design guidelines step-by-step before their proposals are of a sufficient standard.
Therefore, I think that what is really needed is a set of well-written design guidelines with examples of a good design process and its outcome in terms of a concrete improvement to Blender.
Once the new developer has seen and understood the outcome of these well-written design guidelines, the template you propose will make much more inherent sense and most developers will immediately understand which parts of it are useful in any given context.
In other words, I don’t think there’s a one-size-fits all solution to the problem of “good design”, but I do think that a solid project design guide will help generate consensus about which cases require going through the full blown design process (i.e. following the template) and which merely require a “Fix for broken foo” patch with a short description below the summary.
Just my €0.02
That’s why I sugest to include alternative solutions in the path description. While reviewing I also tinker in my head, to see if I can come up with something better. If the developer writes down what they already considered as an alternative and describe why they chose that particular solution from the alternatives, it makes my pondering to see if the approach is sound a lot easier.
We have a back-log of patches to review. My proposal is not aimed at making it easier to create a new patch. It is aimed at making the review process easier, more streamlined, and more straight-forward because the relevant information is proveded up-front.
I think that this is the result of having too much work for too few people. It is also one of the reasons I wrote this proposal: to make the reviewer require less brainpower to understand what the patch is about. To streamline the process by giving the relevant information in an easy-to-understand way, which I hope will help getting faster, higher quality reviews. Of course it’s not the only thing that can be improved, but I think it’s an easy one.
The focus of the development team at the Blender Institute is now 2 days per week on bugfixing, simply to get through the ever-growing pile of open bug reports. If you want to help, please jump onto the bug tracker and help fixing things. At this moment that’s much more important than to add yet another feature (note that this is not aimed at @EitanSomething in particular, just a general call for help).
And my point is that how you design and create a new patch and how easy it is to review it is inextricably intertwined and thus needs to be considered in parallel.
There’s a difference between perceiving a template as yet more red tape and having an educated understanding of why the template exists in the first place.
If the person submitting the patch understands WHY it will help the reviewer if the patch contains e.g. discarded alternative solutions (i.e. one of the steps in the design process), it will make intuitive sense for the submitter to provide these justifications and the process will ultimately be better for everyone (in the ideal case).
You’re free to disagree of course.
I fully agree, and this is also why I have the proposal here for discussion, rather than just posted on the wiki and shoved into everybody’s faces. As I said, please write concrete things I can change/add/remove so that it’s clearer/more useful for everybody.
Add worked examples on the developer wiki for the following cases:
How to create a bug fix patch
How to propose a patch with a new/modified feature
In addition, add a navigation link called “Contributing Code” pointing to the Contributing Code wiki page above the “Code Review” link in the wiki menu on left side of the wiki content under the Process header.
This should take the user to the page where they can also find (links to?) the first two worked examples.
EDIT: It might make sense to also include a link to the Blender 2.80 Design Document blog post.
IMHO, none of the above belong on the Code Review wiki page, but they will help establish the necessary context for the template you propose to add to the Code Review page.
FWIW, I would be happy to help author these pages, as they will also serve as a deep introduction to the process for me.
I’d argue not spending a bit of time to put down the things mentioned here has higher, or at least equal costs. Every day I have to dive into the history to find rationales on changes. That applies to both, code and design changes. All too often, I end up in a commit or patch with no rationales at all. I end up having to make guesses and assumptions, which tend to backfire and result in hard to predict bugs. Or I have to spend significant amount of time understanding the changes and testing corner cases.
My experience is, if you have to write down some (brief) background information and rationales for a change, it helps you make up your mind on things. So it really helps you making better, more informed decisions. Sometimes it makes you realize that benefits aren’t as great as you have thought.
This would also help a lot in communication. We need to do a better job at communicating our design rationales and thought processes.
If we are exclusively talking about UI/UX, feature design, problem solving and alternatives solutions (not code structure, quality, optimizations…) then two things can happen:
If the person who reviews the patch has experience using that area, then he/she will immediately be able to identify the problem the patch is trying to solve, its priority, design quality and come up with alternative meaningful solutions, as well as identifying potential issues that the patch may produce in other areas of the workflow. For this, there is no need for design documents and descriptions of problems, just a small explanation or video of what the changed are is enough for the reviewer to test the feature. If the person who reviews has experience with that area and he/she cannot identify the problem the patch is trying to solve, then we know that the is a design issue with the patch that needs to be fixed before continuing to code review.
If the person who reviews the patch has no experience using that area, I don’t think that having that extra information is useful enough to compensate for the time that goes into writing and discussing it.
Thinking that just a description of a patch is enough information for a developer (or for anyone without real use case experience) to make meaningful design decisions on an area that takes years to master is extremely naive and disrespectful to experts in that craft. This often leads to broken, unusable designs and workflows that are not helping artists at all while developers keep spending time maintaining and fixing bugs on them for years. By including this information in patches the way it is proposed here we are inviting people to perpetuate this mentality, making the problem worse.
This can be solved by requiring artist module members to review and accept the patches before doing anything related to code review. This way we make sure that the design, usability and behavior of the feature are useful and correct, Then, any developer of any area can review the patch without worrying about those potential problems.
In my opinion, every module that under active feature development should have an artist using it constantly on real projects while he/she reviews and accepts patches. This way we will be sure that we don’t break workflows unintentionally and all feature development is useful for real use cases and makes sense in a bigger picture. This is how Grease Pencil is being developed and it is one of the the areas of Blender with the best design for an artist.
There are artists in the studio working constantly with these modules while following the most recent development changes. Their opinion on design, roadmaps and features should be as important as reviewing the code architecture and feature implementation.
Periodically, as new workflows are created with the new functions, users’ experiences tips must be collected, papercuts are very useful in these cases, and it is the duty of UI _ UX designers to identify and propose a better reorganization, so the possible design debts or the chaos that begins to emerge is reordered and rebalanced without too much effort.
I’d throw in a ‘Do I Need a Design Task?’ subhed. A lot of design review is handled during/after code review, and there aren’t firm guidelines on when/if/why someone should get early design feedback.
I think a troubleshooting section could come in handy in the long-term (getting un-stuck in the code review process, etc), but I wouldn’t delay publication for it.
Google Summer Code projects followed here on devtalk by users with ideas and suggestions were a huge success …
Even Particle Nodes UI thread is an interesting example that the method works
I think it should be taken into consideration as a normal process to work in progress.
@pablodp606 You’re right that the patch description should fit the knowledge & experience of the reviewers. However, as @julianeisel also described, there is more to the patch description than just context for assessing the correctnes of the code in the patch. It also provides motivation for the choices made in the patch, and allows people to go back and read up on earlier decisions. There is already quite a bit of code in Blender of which nobody knows any more why it was done that way, and more importantly, whether those reasons are still valid today.
Much yes 
That could work, although I think it may be hard to quantify & describe objectively. I think it would be more beneficial to (somehow) get a culture of discussing ideas up-front with the people who are going to (potentially) review the code. Then it can be decided on a case-by-case basis how big a feature will be, and how necessary a separate design task is.
@Alberto I don’t share your grim look on things. Sure, some bad decisions were made. Nothing disastrous, though – Blender still exists, many people are happy with 2.80, and where they’re not, well, there is 2.81 and there is no end of the development in sight.
Anyway, before we keep discussing things from the past, let’s focus on the topic of this thread: to get more easy to read, more complete patch descriptions.
It’s obvious that these developments should have been more artist-driven. Instead, they were made following some “design guidelines” that existed in a void, and did not originate from discussion with artists/users.
I agree with Pablo in that in the future, every feature/fix/tweak should be used and reviewed by actual users, not judged by how well it aligns in the UI. I think everybody should remind themselves on a regular basis that “Blender is for artists”.
Sorry @sybren I felt I had to say this, I hope this isn’t too off-topic.
@sybren In my opinion, we should try to split product/feature design and code review completely.
Personally, if I’m reviewing a patch for an area I have no experience with, I would like to know that whatever is implemented in that patch is correct and it was already approved by someone expert in that area, so I can just focus on trying to make a better code that does exactly that thing. Trying to review the design of a feature, if the feature description matches the code and the code itself at the same time is a task too big for a single person to handle and a lot of things can go wrong there.
If we do a first product design review to the patches, where we evaluate and discuss the topics you mentioned in this thread, we remove a lot of work and responsibility from the code reviewer. Even if the code is a complete disaster, the person who reviews it should always know that there is an already accepted feature design behind that code that was done by someone else expert on that area, so he/she can focus only on reviewing aspects that are specific to coding.
We will also have an additional review layer to catch bugs, regressions and broken designs that are almost impossible to notice in a code review or by testing the feature in trivial use cases. This is something that happens quite often, both with bug fixing patches and feature patches.
To sum up, I don’t think we should require more information in patches because the information you are suggesting to add should be required on a step before the feature goes to a developer for code review.
Although I understand the sentiment, It is off-topic. I want to discuss a specific part of code reviews, and not development policy & practices as a whole. This discussion is derailing from its purpose, so I’m ending it here. Thanks everybody for the feedback, I’ll think it through and see if I can improve some more things, but I think I have what I wanted for now.
@pablodp606 I think what you say makes total sense, I have nothing to add to it.
I believe discussions should start from feature requests.
Currently, feature requests gather at right-click-select. Most of the requests desire #1 of the required patch description, sometimes all of them.
If they are integrated in the patch submission process, they would serve as a good starting point for design discussions.