darktable
darktable copied to clipboard
darktable-org
Define and implement a process for large changes to the project
It is time that we define a process for larger changes to darktable, specifically if the changes are regarding UX/UI.
The process of just opening a Pull Request is starting to fail us, as people who are attempting to contribute are being turned away because they open a PR with some large change that then has to be discussed and agreed upon.
It seems like the best use of everyone's time to discuss large changes up front, come to some sort of agreement or quorum on concept and implementation before any code is written. Compromise can be had before anyone feels like their time is wasted. This should have the side effect of also simplifying implementation (it should be clear what needs to be done in code) and testing should also be more pass/fail, as all parties will have a clear idea of what to expect.
This is meant as a starting point to that discussion. Suggestions welcome.
- Open a draft PR or FR (pick one).
- add some tag so people can easily find it
- describe your proposed changed. If UX/UI, provide a mockup. explain what problem you're trying to solve or how this is an improvement if augmenting something existing.
- respond to feed back. Be humble. Be open to compromise
- Reach a conclusion
- write code
- test PR
- merge
- everyone rejoice
The problem I encountered the several times I attempted to follow this process is that nothing happened between 3 and 4, i.e. no feedback to respond to.
True. Do we conclude there was no interest or just that nobody noticed? I've also found that sometimes the only way to get noticed is to submit a PR.
I was hoping one can subscribe to a label, so you'd get notified somehow that something new has happened.
I find it a bit difficult to keep us as well. Some auxiliary infrastructure to notify people would be nice as well.
On August 13, 2022 2:41:37 PM PDT, Chris Elston @.***> wrote:
True. Do we conclude there was no interest or just that nobody noticed?
-- Reply to this email directly or view it on GitHub: https://github.com/darktable-org/darktable/issues/12319#issuecomment-1214226305 You are receiving this because you authored the thread.
Message ID: @.***>
I think such a process with mockup and proposal and feedback is good for UI stuff.
More often - in my experience - there is "something" that could be done better in dt from a technical or code maintenance point. In those situations it is probably better to develop on your own until there is good code and result and present that as a pr "open for discussion" so others can test real stuff.
If no good, it's my time spent and often the feedback did in fact lead to something better in another pr.
The problem I encountered the several times I attempted to follow this process is that nothing happened between 3 and 4, i.e. no feedback to respond to.
We can set up formal process with formula like in ancient marriage ceremony. When a ~~marriage~~ PR was impending it was announced for three consecutive Sundays. This would give all ~~parishioners~~ project contributors a chance to raise an objection to the ~~marriage~~ PR. After that we enter speak now or forever hold your peace phase.
Otherwise, the lack of sufficient motivation to discuss a certain proposal can delay the process for a very long time. It is known that the desire to discuss something theoretically possible and the desire to block the introduction of changes that are approaching are not the same thing.
Or we reverse the situation and say that if nobody says it's a good idea after three weeks it gets dropped.
I think it's dangerous to start off from the assumption that all new functionality is good. Bug fixes should not be delayed. New features almost always should be delayed, since there's no hurry for new features -- generally we've got on for more than a decade without such a feature so it can wait a bit longer.
The bigger problem recently has been features being merged before anyone noticed them (or had time to examine them in detail). Some mandatory "cooling off period" before feature merges would be a good thing IMO.
features being merged before anyone noticed them (or had time to examine them in detail).
Or because the "feedback" was that people would test after the merge (which translates into "two weeks before freeze"). Which would still be fine if there could be a feasible plan for reversal, but by definition, for "invasive" changes, there never is one.
The more invasive (large) changes are sometimes merged faster than smaller (and more easily testable) ones, for field testing. I'm not sure whether this results in more testing or less for those changes. I guess it depends on how many people are using master as a daily driver and how likely they are to touch new functionality.
more invasive changes are sometimes merged faster than smaller ones
The reason for this is that otherwise they need to be continuously rebased, because many small merges will cause conflicts with invasive PRs (again, by definition of "invasive") which would block easy testing.
Yes, and no one can assure that large invasive changes do not have side effects. We know many people are using master even on Windows as wpferguson regularly provide binaries.
Where the initial proposal fails short is that on most Open Source projects people like to hack something before proposing a PR. When the PR is created we do discuss... Well sometime as very few people care reading PR and commenting. So at this stage we are back to the current situation, I'm commenting, giving some feedback, working with the author... at some point I merge and THERE people start complaining. I'm not kidding, that's how it happens most of the time.
Look at my PR to move the wb presets into an external JSON file. It is opened since a month and got 0 comment.
I think I read most pr's before they get merged, in many cases I just don't know if it's really good. That's the case in your white balance json stuff, I have absolutely no objection but I wouldn't think my opinion is qualified enough. So keeping silent.
at some point I merge and THERE people start complaining. I'm not kidding, that's how it happens most of the time
Do we find this acceptable? This is what I'd hope to combat. In addition, 11 years into the project, we should be aiming more for refinement of our UX/UI, and not bolting on every PR that is opened.
On August 14, 2022 11:48:27 PM PDT, Pascal Obry @.***> wrote:
Where the initial proposal fails short is that on most Open Source projects people like to hack something before proposing a PR. When the PR is created we do discuss... Well sometime as very few people care reading PR and commenting. So at this stage we are back to the current situation, I'm commenting, giving some feedback, working with the author... at some point I merge and THERE people start complaining. I'm not kidding, that's how it happens most of the time.
Look at my PR to move the wb presets into an external JSON file. It is opened since a month and got 0 comment.
-- Reply to this email directly or view it on GitHub: https://github.com/darktable-org/darktable/issues/12319#issuecomment-1214677405 You are receiving this because you authored the thread.
Message ID: @.***>
I think I read most pr's before they get merged, in many cases I just don't know if it's really good.
Same here, I comment if I can help or have an opinion and actionable comment. Sometimes there are changes that I don't like and I can't find a "constructive" comment (I just don't like it) so sometimes I'm silent and probably shouldn't be. Also, once the number of comments go past 20 or 30 the level of investment required to even understand the current state of a change gets hard, and I suspect a few people bail out at that point.
I'm much much more likely to comment if a PR/FR is fully explained in the first post (including any changes made in response to comments). If we could require this be kept complete and up-to-date for all functional changes this might go a long way towards getting more feedback. It would also help when it comes to writing the documentation.
Notes on what testing has been performed (and suggestions for how others can contribute) would also be good. This is all stuff that we can do to make reviews/feedback more likely, and we can insist it's complete before merge and even on a waiting period once finished.
Do we find this acceptable? This is what I'd hope to combat. In addition, 11 years into the project, we should be aiming more for refinement of our UX/UI, and not bolting on every PR that is opened.
Any other proposal than letting a PR die and people to move away from the project because nothing happens.
My thinking is that we should strive for small incremental changes. We are not paid, and there is little chance that a big UI change will happen. We just had an example, the generic widget for color would have been a 100% identical UI as today. It would have been used in many places in dt making more code sharing which is good. It could have been improved later to get some other UI (as discussed in the issue/PR). But with the noise added into the comments the devs has just decided to stop this work and maybe move away from dt. We have lost some good improvements. Do we want more example like this?
It is is a criticism to anyone, just an example we had recently (we had other cases before). So at the end I'm not seeing a good solution. No one being paid, the project must be fun to work on. Sadly this probably hinder large rework, but I don't have a better proposal.
Now if someone want (and has the energy on the long term) to start a more formal redesign of the UI that's fine. How is willing to handle this?
Note that we would also want/need a way to surface these kinds of PRs for review. This is sort of a separate issue.
On August 15, 2022 12:07:33 PM PDT, Chris Elston @.***> wrote:
I think I read most pr's before they get merged, in many cases I just don't know if it's really good.
Same here, I comment if I can help or have an opinion and actionable comment. Sometimes there are changes that I don't like and I can't find a "constructive" comment (I just don't like it) so sometimes I'm silent and probably shouldn't be. Also, once the number of comments go past 20 or 30 the level of investment required to even understand the current state of a change gets hard, and I suspect a few people bail out at that point.
I'm much much more likely to comment if a PR/FR is fully explained in the first post (including any changes made in response to comments). If we could require this be kept complete and up-to-date for all functional changes this might go a long way towards getting more feedback. It would also help when it comes to writing the documentation.
Notes on what testing has been performed (and suggestions for how others can contribute) would also be good. This is all stuff that we can do to make reviews/feedback more likely, and we can insist it's complete before merge and even on a waiting period once finished.
-- Reply to this email directly or view it on GitHub: https://github.com/darktable-org/darktable/issues/12319#issuecomment-1215622444 You are receiving this because you authored the thread.
Message ID: @.***>
Or we reverse the situation and say that if nobody says it's a good idea after three weeks it gets dropped.
That's already the case, no? I do merge only if I'm ok (or you don't count me as somebody saying it is a good idea :) or if some people have said they liked it.
I have failed to define what a "big" UX/UI change is, but you've hit what I was thinking of, Pascal.
The color wheel UI, sigmoid module, and the collection filter changes are all examples of "big" UI changes to me. In only one case (collection filters) was an initial proposal made, and that still turned out a bit weird and obtuse to use in my opinion. In the other two cases, people just hacked away and then showed up with code and were shouted down.
Would this out come have changed if we had a process in place to evaluate these ideas and discuss them before someone starts spending time on writing code? If that fails, should we at least set the expectation with people who raise a PR that there will be scrutiny (up to and including nonconstructive "this isn't good").
I don't know if that destroys the fun of it for everyone, but I'm trying to think about how we can avoid such backlash where new contributors and new features are just shut down.
If you think what has been happening lately is the best we can do, Pascal, then I accept that and I'll close this; we can carry on in the current fashion.
On August 15, 2022 12:47:01 PM PDT, Pascal Obry @.***> wrote:
Do we find this acceptable? This is what I'd hope to combat. In addition, 11 years into the project, we should be aiming more for refinement of our UX/UI, and not bolting on every PR that is opened.
Any other proposal than letting a PR die and people to move away from the project because nothing happens.
My thinking is that we should strive for small incremental changes. We are not paid, and there is little chance that a big UI change will happen. We just had an example, the generic widget for color would have been a 100% identical UI as today. It would have been used in many places in dt making more code sharing which is good. It could have been improved later to get some other UI (as discussed in the issue/PR). But with the noise added into the comments the devs has just decided to stop this work and maybe move away from dt. We have lost some good improvements. Do we want more example like this?
It is is a criticism to anyone, just an example we had recently (we had other cases before). So at the end I'm not seeing a good solution. No one being paid, the project must be fun to work on. Sadly this probably hinder large rework, but I don't have a better proposal.
Now if someone want (and has the energy on the long term) to start a more formal redesign of the UI that's fine. How is willing to handle this?
-- Reply to this email directly or view it on GitHub: https://github.com/darktable-org/darktable/issues/12319#issuecomment-1215698266 You are receiving this because you authored the thread.
Message ID: @.***>
I think there is a simple, practical, actionable way for how this could work. Feel free to shout it down , I'm not sensitive about it.
In either 'issues' or 'pull requests' create a new subcategory (or tag) called proposal. A proposal should only be opened by a dev, who has the skill and intent to write the code. As paper digits suggests, it should detail
- what the proposal is
- why it is desirable (what problem it solves)
- optional mock up for ux and ui.
It should also be tagged with a scope (Eg, ux, ui, colour science, metadata, etc...), And Devs should subscribe themselves to whichever tag they wish, so they are notified when a proposal of interest comes up.
The default assumption should be this is a bad proposal, until people say they like it. Why? Because if people don't like something, they might be reluctant to comment so as not to cause offence. Whereas if people do like something they are more likely to say so or thumbs up. So if a proposal gets no comments, the dev knows they don't have to spend time on code. If people do like it, a certain period of time should be agreed upon before writing code begins - time for others to object or offer alternate solutions. Maybe 2 or 3 weeks as suggested above?
Of course the option for Devs to just hack code and submit a pull request will still exist. These should be treated the same as proposals, and given the proposal tag, until those 2 or 3 weeks of discussion have passed. That way there is no expectation every pr will be merged, and hopefully not such hasty closing of prs when a bit of opposition is heard, because it is expected as part of the process.
The proposal tag is removed once we know the idea has been accepted or rejected, where Pascal has the final say.
Someone wiser than me should put this process in official documentation somewhere, so when a new dev comes along and wonders why their pr has been called a proposal or met with debate, they can be pointed to the documentation, reassuring them it's part of the process.
If that fails, should we at least set the expectation with people who raise a PR that there will be scrutiny (up to and including nonconstructive "this isn't good").
I suppose people understand that. Even if it is always a bit frustrating we cannot blindly integrate something that no one want. This can destroy the fun and certainly make people move away from the project. But if they do I think they are not ready for criticisms and review, so maybe not a big deal.
That's not the case for the sigmoid dev, still working on it, have been given lot of feedback and did improve it since then. After months it has become better and better and many people support it. To me the dev here is open mind, is ok for criticisms as long as they are constructive.
What I can say finally is that PR with the UI flag should certainly get more reviews and attention not only from devs but also from users and contributors.
If you think what has been happening lately is the best we can do,
I feel that, maybe not the best way of working, but the best I can do with my allocated time on the project.