The “patches welcome” defense is killing constructive criticism

[mschumacher69]

Just throwing this here to continue the conversation that was started here...
https://www.howtogeek.com/please-stop-lowering-your-standards-for-open-source-software/

[BenMcLean]

Patches apparently aren't welcome.

Does Jellyfin need a larger team instead of apparently just one guy?

[BenMcLean]

Making which player to use a global setting on the Android client that's hidden several levels deep is real bad though. Even if the particular PR has some technical or maintainability problem, the project should be addressing the underlying issue in some way. Burying player selection under several levels of menu, somewhere between a dozen to two dozen button presses away (to get all the way out of a video to reach the player setting, then all the way back to the video, then to watch it, then to go all the way back out of the video and into the player setting to set it back) is obviously ridiculous. It would make more sense to drop the feature of alternate players altogether (thus sending a clear signal that somebody who wants this should make their own app for it) than to make the means of reaching it this actively hostile to users. If you don't expect people to be using it then just don't include it at all, but do not normalize this kind of ass backwards design.

[BotBlake]

@BenMcLean
I am not deeply involved in this specific Android TV situation, so I do not want to speak for the maintainers there directly. But this is exactly why discussing changes with the maintainers before investing major work into a PR is often valuable.

I also want to point out that the tone of feedback matters a lot in open source discussions. Statements like:

"do not normalize this kind of ass backwards design"

usually do not come across as constructive criticism. They tend to frame the current implementation as incompetent while presenting the proposed solution as obviously correct. That can quickly turn a technical discussion into a confrontational one, even when there may actually be valid feedback underneath.

Maintainers are generally much more open to discussing shortcomings, alternatives, and tradeoffs when the conversation stays focused on the problem itself instead of attacking the existing implementation.

Regarding the actual feedback: the underlying point I am hearing is that external player selection may currently be too difficult to access for users who switch between players frequently. That is a completely reasonable usability discussion to have.

But there are also tradeoffs involved. Settings like alternate/external players are often considered advanced functionality. Making them highly visible and easily accessible can also lead to inexperienced users changing playback backends without understanding the side effects, which then creates additional support and UX problems.

So there are multiple possible conclusions here:

• the current placement is acceptable because the feature is considered advanced,
• the settings structure could be improved,
• the workflow for temporary player switching could be redesigned,
• or the feature itself may need reevaluation.

Those are all legitimate design discussions. But they are broader product decisions, not automatically solved just because somebody opened a PR implementing one specific approach.

If we look at the PR you linked, it appears that the feature was implemented primarily based on issues where the maintainer team had already questioned whether this functionality should exist in the first place.

That is exactly why discussing larger changes with maintainers beforehand can be important. Was there any broader discussion about the feature and its implementation in chat rooms or on an issue before the PR was opened, or was the implementation work started immediately?

When maintainers have already expressed uncertainty about a feature’s direction or necessity, opening a PR without prior alignment can easily lead to situations where significant work is invested into something that ultimately does not match the project’s goals.

[Albermonte]

@BotBlake
I agree in principle, but a private chat room isn't the right place for this. Discussions about new features and the reasons for rejecting them should be public, with clear reasoning attached.

If the chat room stays as the first point of contact, there should at least be a CONTRIBUTING.md (or similar) outlining that this is the first step to contributing, what kinds of features are accepted, how decisions are made, and on what grounds something gets rejected.

For context:
I'm not involved in any of this. I stumbled onto this repo looking for a fix to a Jellyfin issue and started poking around. Here's what I found:

A user reported on removed features, with several others asking for them back:
#4261
13 interested users. Reason given: "not user-friendly and an annoyance to maintain with the ongoing refactoring of playback-related code."

A user proposed a solution (#4465), 26 interested users. A PR followed (#4487) but was stalled after feedback. Someone else picked it up and addressed that feedback (#4990 now, suddenly, "there is currently no intention to add this feature."

From my point of view of somebody that just landed here:
The decision of a feature of being accepted or not is only if the maintainer thinks it will be used or not, 20+ users requesting the same feature to be added are not enough, a PR with everything ready and all feedback addressed is not enough.
A lot of comments were minimized and marked as off topic or spam (even though from my perspective, they were simple comments, no reason to minimize) and that leads me to believe that the maintainer is not open to new features and that anyone who disagrees and points it out will be silenced.

[BenMcLean]

From my point of view of somebody that just landed here:
The decision of a feature of being accepted or not is only if the maintainer thinks it will be used or not, 20+ users requesting the same feature to be added are not enough, a PR with everything ready and all feedback addressed is not enough.

That is what this looks like yes.

The reason for using alternate players is often that particular videos sometimes have quirks which mess up the official player in some situations on some devices. Those quirks should probably also be addressed directly somehow, but even if they are, you're never going to fix them all and keep them all fixed all the time, so alternate players as an option really does need to be made a permanent first class feature. Another use case is when an alternate player offers a feature, like stereoscopic 3D display interlacing support, which the default player absolutely cannot and probably should not provide, but which only applies to specific videos (like only 3D videos or some other edge case) while the default player should still stay the default for most videos.

That's why I think alternate players should be made a first class permanent feature on the Android client. Conceptually, which player to use is an individual video setting, not a global setting, so the UI should reflect this. If it's desired to hide the option for non-advanced users then maybe a "Show Advanced Controls" option should be added for this, but the existing design is way, way too much menu diving just to switch players and, worse, to switch back. (nobody remembers to do that which messes things up next time)

I struggle to understand why it would be a big technical problem just to change where the exact same option is configured. This isn't even a new feature: it's just making an existing feature reachable where it's actually used.

Alternate players are primarily for individual videos that are special exceptions to general use on the same system, almost always because they're in some weird, non-standard edge-case format. But, when the feature is needed at all, it's needed often and fast and it's needed to not screw up how all the rest of the videos open just because somebody forgot to set the alternate player back to the default last time.

I think long-pressing to open a player selection modal would be an OK UX solution but if that's not desirable then there's many other ways this could be addressed. But it needs to be addressed somehow.

(I've criticized long pressing as non-intuitive in design discussions of other unrelated projects elsewhere in the past and I'd actually prefer a separate button over a long-press but either would be better than what we've got)

[BenMcLean]

See, this is all we get, which is what @Albermonte was talking about.

I'm not personally prepared to maintain my own fork all on my own right now because of other commitments in my life and I understand how devs of open source projects like this are generally volunteers for whom users can seem really entitled on requests, but it's not like we're demanding they create a feature from thin air: we had a perfectly good PR for this. This kind of situation is where hostile forks come from.

[BotBlake]

Hi there.

I genuinely don't understand what outcome people are expecting from this discussion.

If you're unhappy with one of Jellyfin's official clients but prefer a third-party client, that's actually one of the strengths of an open ecosystem. You're free to choose the client that works best for you.

I also don't understand the argument that the project should simply "increase the team size". Jellyfin is an open-source project. Anyone with the interest, skills, and willingness to contribute can already do so. People become contributors by contributing.

Speaking for the documentation team, because that's where I have the most visibility:
Contributions really are welcome. If you discuss a change with us beforehand, it's very unlikely to be rejected. The same principle applies elsewhere in the project. "Patches welcome" does not mean "every patch will be accepted". Contributions still need to meet quality standards and fit the direction of the project.

What I also don't understand is how this relates to the "unplanned feature" debate that @BenMcLean keeps bringing up.

If a maintainer says a feature is not planned for a particular client, then patches for that feature are usually not welcome either. That's not contributors being ignored; that's maintainers doing their job. Maintainers are responsible for shaping the project and deciding what belongs in it. Sometimes that means saying "yes", and sometimes it means saying "no".

Not every client needs every feature. Trying to add every requested feature everywhere is a good way to end up with bloated, difficult-to-maintain software.

You are treating "patches welcome" as if it's meant to shut down discussion. Often it's much simpler than that. Contributors are volunteers with limited free time. If someone points out a problem that nobody currently has the time or motivation to work on, "patches welcome" often literally means: "I agree this could be improved, but I'm not going to work on it myself. If you'd like to help, contributions are welcome".

That's not hostility. That's just the reality of volunteer-driven development.

Which brings me back to where I started: What exactly are you expecting us, or contributors like me specifically, to change as a result of this thread?

All I see is that this discussion has already consumed multiple hours of my time. Those are hours I could have spent working on community issues and improving the documentation instead.

I am not against criticism or feedback. Quite the opposite. But I would like feedback to be actionable and lead somewhere useful.

At this point, I am struggling to see what the expected result of this discussion is.

There is a difference between feedback and hostility. If the goal is to improve the project, constructive criticism goes a lot further than attacking the volunteers who are spending their free time working on it.

Kind regards

[thornbill]

This is not just about the "patches are welcome" argument

The title of this discussion:

The “patches welcome” defense is killing constructive criticism

---

You keep bringing up Wholphin as some prime example of community collaboration, but have you even bothered to look at its contributors? It's one person.

Did that person make any attempt to contribute to the official app? Absolutely not. They have only ever opened a single PR here and it was merged!

So your entire argument here is just completely baseless.

[mschumacher69]

That's the main issue here I guess, you guys seem to be stuck in a box and cannot think outside of it. You're too strict with semantics and with enforcing policies to the point that if a discussion is titled The "patches welcome" defense is killing constructive criticism, we cannot discuss anything else related to that, such as finding ways to speed up development or inviting more devs to join the team.

I never said Wholphin is a prime example of community collaboration, I'm just saying that even with one dev and a shorter development period, the guy managed to come up with a client that is far more superior than the official app.

Your argument is always that the Jellyfin Andorid TV app is primarily maintained by a single person, but Wholphin is a prime example that a decent app can be developed and maintained by a single person without sacrificing on features and speed of development. Now I understand that different people have different workloads and perhaps the Wholphin dev has more free time than the Jellyfin dev, but my point is that if your dev doesn't have much free time to speed up development, either become more lenient on your strict PR requirements or invite more devs to the team to speed up development.

You said that the Wholphin dev has never made any attempt to contribute to the official app and only ever opened a single PR that was merged. Do you ever think why? Don't you think it's because he knows that if he ever attempted to add any of those Wholphin features to the official app, all his PRs would have been rejected because they are either "unplanned" or "unwanted" or because he didn't fill the PR template correctly? So he decided it would be just faster and easier to develop his own app and create something amazing instead of having to deal with strict PR requirements and roadblocks everywhere and never get anything approved.

Again, my end goal here is not negative criticism, it is positive criticism to try to improve the app and speed up development because this is clearly possible with one dev as we can see from Wholphin. We all want this project to succeed, so there's no benefit from destructive criticism because it doesn't help with anything.

[BotBlake]

I don't have the motivation to write a proper response for this...

Our team does loads of stuff for you. Including maintaining the SDK that makes Wholophin possible in the first place.

Our team is busy. If you can't find the time to fill out a simple PR template then I personally can't imagine the change is even worth the review. Spending hours to review a PR only to realize it's garbage sucks, so we try to get as much info condensed into the PR description as possible.

Wholophin team is not accepting any more PRs then our team is, so whatever you are trying to say does not make any sense.

You changed the topic of this discussion multiple times by now trying to explain why the official client is not as good as Wholophin.

I'm glad you enjoy Wholophin.
Please leave it at that. Unless you have specific criticism that we can actually do something abou, we cannot help you here.

WHAT DO YOU WANT?