Implementing pixls.us ideas in darktable

Software darktable
23

Sounds like we’ll continue as we are now, and people rage quitting their PRs is acceptable collateral damage?

Frankly, that PR was very frustrating for me as well. Since we have no policies around such things, the PR submitter was able to fundamentally change the way a currently released module works and never acknowledged preserving the existing, released version of the module. Instead, I was gaslit the whole PR, with the dev saying “these are only UI changes” which they were not and “you can still get the same results” which I could not.

If we had some rules and set some expectations, it might help. Like, if you’re fundamentally modifying the way a released module works, you need to preserve the current module.

I agree that stating up front what problem you’re trying to solve would help, even if you do so in the GitHub PR. There needs to be something for other people to test against. The PR in question changed so many times, it was difficult to follow, made harder by the inconsistent answers of the PR author. The whole thing was not good.

All I wanted was for the existing Tone Equalizer to be persevered so I could continue to use it in my workflow. That has happened now, but it happened the worst possible way.

2 Likes
25

so we do have policy, where is that written down?

26

I think we probably need to go through the wiki and look at what’s there and figure out what needs updating. I’m pretty sure it hasn’t been kept up to date (one reference I saw was discussing ideas on IRC or the mailing list).

28

You’re assuming someone has to say something, but if it were written down and linked prominently in the dev wiki, you don’t think that’d help? Or make it part of the PR template as well.

You seem to be talking in the abstract, but I think the context is important here, as that is what is supposed to be framing this conversation.

I understand that for you it doesn’t matter, as you’ve been around long enough to know what is expected and know all the unwritten rules and such things. But for a first time PR submitter, do you not think giving them some context around how their PR will be evaluated would be helpful?

For me, my whole diatribe on that thread could’ve been avoided if the PR submitter had known that they’d need to preserve the current working module. When I mentioned this in the PR, it didn’t seem to carry any weight nor did @Pascal_Obry seem to reinforce it.

1 Like
30

For me that has been absolutely clear for long, if you change the working of a module it must keep old edits either by not touching code or some version bumping. Anything else would not be accepted.

I don’t think we have such a policy - or a clear agreement - about “can we change the UI” and what should be the reason to do so.

I said this elsewhere before. Indeed not a good ending. But at least as i saw it - i was in no fear the module would not be usable as before - the heat was from both sides, for me without good reason. There are definitely some issues in TE. Just today - while analysing a github issue - i only saw in the logs that TE maths curve was not stable. No indication until i changed a value by “luck”. Is that good? No.

4 Likes
31

The following can be found on Home · darktable-org/darktable Wiki · GitHub

Proposing features

While proposing new features, keep in mind that darktable:

  • tries to stay compatible with its earlier versions,
    .>…
32

I’d hope that you’d try and understand the high level issue that is being discussed here: that a PR was closed after a semi heated PR and the PR dev has decided not to even try and contribute anymore and take that context into account in your reply. I don’t think that someone has left and won’t try to contribute again is an acceptable result and so, at least to me and @wpferguson, there seems to be some improvements that can be made here. That is what we’re discussing.

To me, it sounds like there are rules/guide lines/expectations that are there but they’re not prominently documented.

I, personally, can not and do not want to enforce any set of rules. I’d like to make the process by which we provide feedback, evaluate, and communicate in the project to become better so that we don’t have these kinds of flame outs. And this isn’t even close to the first one either.

And if we can’t even discuss things in the open to try and better things, won’t we just stagnate? Or are we already as good as we can be? Honestly it is very deflating to be shot down so early in the conversation.

Yes but you’ve been around a while. So has @dterrahe. These things likely would not effect either of you at all. As long time contributors you understand the (apparently) unwritten rules and expectations of the project. Both of you seems like you have the trust and respect of Pascal and the community at large, and you’ve had a pretty lengthy tenure with many excellent contributions. But I’m willing to bet that you didn’t start out that way.

I don’t have a problem with UI change, as long as its UI change only. This particular PR was not UI change only, not matter how many times the PR dev said it was.

I agree that there should be a lot of wiggle room for the interpretation of “good UI change.” It is also hard thing to make concrete.

I am not asking for a stern set of rules that must be followed at all times and are enforced harshly.

I think it’d be beneficial as a community if we spelled out the way things generally work up front. Things like “certainly not everyone will agree with larger changes, so you should be prepared for discussion and questioning if your proposed change makes sense, is “good”, is “right” or not.” The PR dev in question was clearly not ready for any of that. It seems they thought they’d stumbled onto some universally good change that everyone would agree with what they were doing, and that just can’t be true. (That was my take on the situation at least).

I never said there were not problems. Did you test the change in that PR? The results were a different module underneath a relatively similar UI. I tested at several points and the changes between those points in time were vast and seemingly without aim.

To me, this is less about the technical merits and more about letting people new to the project know what to expect. If you want your PR to be accepted, we, as a project, should spell it out that we expect a clearly stated problem or issue and a PR to address that specific thing stated in the problem.

In this specific case of this particular PR, the problem statement was never clear, the changes themselves wandered all over the place, it was hard to follow from the beginning what exactly was changing, and it was even harder to test and provide feedback on the longer it went on.

I hope that makes some sense.

That is a good start, but I don’t think it goes far enough.

Interesting enough, another page pretty much sums up the PR in question:

Too many programmers jump on their IDE before being sure they actually understand the problem they are trying to solve.

1 Like
34

Are you even reading what I’m writing? Seems like no.

Anyway, if you want to continue as-is, just say that. Its a lot simpler and shorter.

1 Like
36

Please just bow out of the conversation.

Yes, for fuck’s sake. God forbid there is input from others before the work is actually done. You know, come to a consensus?

2 Likes
37

Right and i stopped following each iteration of work so tried it just once quite early and i simply couldn’t follow commits. Some ideas to me were technically simply wrong, two of them were really good, one of them was visualizing math problems by colors. What we did was frankly rejecting a good UI idea as hell was near because the pr rest seemed to be a mess. For me that was “our bad part in the story”.

A strict written down set of rules does not solve any problem to me.

38

Jesus. Forget it.

1 Like
39

If a dev is introducing new features with such massive ui changes it may be best practice to recommend separate smaller pr’s for each individual change, for each unique problem they aim to solve. I didn’t follow the whole conversation but I believe someone suggested that. That way, the agreed upon good bits can be merged, even if other ideas are rejected. It also allows each idea to be discussed and fleshed out on its own merits, and not be lost in discussion of all the other changes.

40

TL;DR There is a difference between rigid rules and guidelines. I understand the pushback against rigid rules, but clearly documented guidelines are useful for contributors and have no drawback that I can think of.

Let me offer a bit of perspective as someone who is at the margin and is trying to contribute with ideas and the occasional bit of code while observing the dynamics of this community.

  1. When I tried to discuss some UI aspects here on Pixls, I was told (among other things and in varying degrees of kindness) that there is no point in discussing something if one does not have already implemented the code to back it up.
  2. I have proposed a couple of features with a working (proof of concept) implementation around shortcuts here on Pixls, and in both cases I was told that it would be better to iterate on it in a PR instead of doing it on Pixls.
  3. Following that, I have created a PR for a UI change after introducing it here on Pixls and receiving positive feedback. I was told that I should have created an issue on github before preparing the PR. The PR didn’t receive any push back, but it didn’t get any attention either.

What I make of this is that (1) different devs have different standards and expectations, (2) their communications here and on github are not coordinated, and (3) the kind of response that you get from the first dev that stumbles on your proposal/idea/FR/PR by and large sets the tone moving forward.

I think that most of the disagreement in this thread arises from the fact that some folks are talking about guidelines (the term used in the OP), whereas others about strict rules.

A set of strict rules would not be good, as you don’t want to have too much red tape in a project that feeds on freedom and spontaneity, but the core dev team should be able to agree on a set of broad guidelines to help willing contributors get some clarity and find their way around. And if hard rules exist, such as “no algo changes that break modules’ backwards compatibility”, then they should be prominently stated.

@hannoschwalm Guidelines (not hard rules) may not solve any of your problems, but they would make things easier for aspiring contributors (and, I would argue, on average make things easier for the core devs).

@wpferguson and @paperdigits are right in advocating for some more clarity. It cannot hurt to have a “golden path” documented prominently (and possibly enforced at least verbally in the templates for FR and PR on github).

I for one would definitely prefer not to have to second guess what is the right next step to take to get the devs attention without pissing them off or wasting their time.

6 Likes
41

Don’t know if that’s the main point. Probably right we have to make some rules more clear. Also it’s about howto proceed with dt. And we all have different rules for sure. But keeping calm is certainly good.

I strongly dislike the idea that every PR should have been discussed before, should only be about a single problem to solve … that would break ideas, progress, creativity …

Also i think, that only PR’s with

  • working code on master
  • formulated idea what the PR is about
  • possible mentioning restrictions/issues on previous code

should be expected to be reviewed, open for discussion, feedback, review as people can test.

If something is just an idea - vague or concrete - that would be an RFC issue. But the dt issue tracker is basically about issues and not vague idea.

Clearly wrong in the mentioned PR was meandering topics and the massive under-estimation of involved problems ahead including maths that were very problematic at least on one side. WE missed to spell out “do not do this” - “we would like to have this split into several parts, let’s tackle this first” , and we’d better be more cool and trusty that bad stuff will be not be included or possibly reverted.

For me we have a loss of someone willing to dev. Could we have done better, likely yes. Is the missing of guidelines/rules the underlying or major problem, i don’t think so.

If we formulate rules/guidelines those should definitely include “red-flags” for things we never want or also what users will get pissed about.

3 Likes
42

One of the major issues was “Instead of changing old modules, iterate them and keep the previous version functional.”

This hard rule would indeed have solved one of the major problems about was being discussed here.

@paperdigits is right, and there seems to be clear hurdles that a new contributor has to face just to get started, when it doesn’t need to be this way.

As @Masterpiga pointed out, he followed everything that was told him and still faced resistance in some way or another, so clearly at least Pascal or him and all major reviewers should draft something or improve the contributor docs.

I don’t know C, but if I did, just reading this thread I wouldn’t want to even attempt to contribute to dt. Just reading dterrahe’s replies is off putting and the “we had it bad so new contributors need to have it bad too” just sucks. We should strive for a better and easier world with less stress not perpetuate past problems. Walking on eggshells until the way is found doesn’t work

2 Likes
43

I wouldn’t know, he deleted all his replies…

What I get from all this, is that some guidelines about the preferred procedure would be good (perhaps even needed), with some strict rules a PR has to follow (notably the "preserve old edits).

That said, I didn’t read the remarks in @masterpiga’s PR’s as criticisms, but rather as suggestions for improvement. And the cited PRs were added late in the cycle for the next version (which may explain some of the lack of traction for new features).

1 Like
44

It is interesting to note that all core devs are saying it is good as it is and they are the ones impacted by this.

So why non core devs want to force something on us? This is beyond me! Why do you want to enforce some rules on others? Again this is beyond me.

45

I mean, if core devs don’t see how making the life of new contributors easier isn’t a necessity or a good thing I don’t know what to tell you. Maybe eventually when the core devs retire there will be no new development due to how much friction there was that no new “core devs” were formed.

I would understand if this imparted a lot of work on the team which is already overworked, but it’s literally a one page document or even less with some guidelines.

As an example, it’s not hard to see how much better darktable became processing quality wise after contributions(scene referred workflow, filmic) from a, at the time, non core dev (AP). It would be a shame to scare or send away talented developers who would otherwise bring another good push to improve the software even further.

2 Likes
46

That’s exactly the problem, it has already been said that we have guidelines and we don’t want to be more strict to avoid scaring new contributors.

1 Like
47

Please understand that I keep replying because I have the feeling that what I and others are saying is being misunderstood.

I keep advocating for guidelines and I see replies about how strict rules are bad, which I think we all agree upon.

What I and others are trying to say is that the lack of guidance (not hard rules) makes the process more arbitrary, which can be a deterrent for new contributors, not core devs.

The summary of my experience a few post above shows that different devs have different preferences, which makes contributing effectively harder than it needs to be.

Here is a potential new contributor telling you that they would like some more guidance :slight_smile: Because they have tried to engage in several ways and every time someone told them they were doing it wrong. If there had been some guidelines, I could have said “Hey, I am following the guidelines”. Without, I am left wondering whether I actually did something wrong. Should I have tagged some specific dev? Should I have raised this conversation in a different channel? Should I have not proposed this kind of change at all?

Clarity of process and flexibility are two virtues that can totally coexist.

What is the problem with writing in a doc:

| If you want to do X, the preferred way is to first do A, then B and then C.

Something a bit more structured than Hacking on darktable, touching on some of the points that have been discussed in this thread.

Again, not rules, just guidance for newcomers.

4 Likes