The problem is here is not AI, the problem here is pushing code that didn’t even compile straight to main. It’s a huge collaborative project, running it as a one man show just doesn’t work anymore.
Had he made a PR/Draft, none of this would have happened. Hell, even his commit message says “needs to be reviewed before being deployed”
Which by that point they’d already fixed. They mentioned they also accidentally pushed to the wrong branch.
I dunno, this just looks like someone accidentally made a bad commit and instead of being mature about it and letting them fix it we get statements like “all bridges have been burned” and a split in the community. That’s a bit overblown, and if at the first sign of disagreement the decision is to blow up a project you’re making it very hard to work with you.
Well, in that case, fair enough.
However, given that a lot of contributors allude to “putting up” with him for years in the comments of the GitHub issue, it seems it wasn’t just a bad commit that broke the camel’s back
Well, the detection is broken for KDE and backwards in the XDG implementation (which is also only used as a fallback when the three DE-specific implementations fail, even though all of them actually support XDG so having separate implementations is pointless).
Also with the way it’s implemented, it will have unexpected results for users who have both KDE and Gnome installed (or at least have leftover configuration files) - if you for example used KDE in the past with a theme considered to be “dark” by this and now use Gnome and have it set to light mode, you will get dark mode GZdoom with no obvious reason why.
Oh and the XDG implementation is also very fragile and will not work on everyone’s system because it depends on a specific terminal utility being installed. The proper way would be to use a DBus library and get the settings through that.
And when somebody comes to fix it, they will have to figure out a) what’s so special about the DE-specific implementations that XDG wasn’t enough (they might just assume that XDG isn’t supported widely enough), b) learn how to detect dark theme properly on the DE they’re fixing, c) rework the code so that there is a difference between “this DE wants light mode” and “couldn’t figure out of this DE is in light or dark mode” - both of these are now represented by the “false” return value.
I don’t think a well written and functioning code made with AI assistance would get a response this strong, but the problem here is that the code is objectively bad and its (co-)author kept doubling down about something they probably barely even checked.
I mostly agree with you, but this is not quite true:
XDG implementation (which is also only used as a fallback when the three DE-specific implementations fail, even though all of them actually support XDG so having separate implementations is pointless)
Yes, the DE-specific implementations is pointless (as far as I know, I use a WM), but the XDG implementation is actually used first, and the function returns true if any impl returns true, like xdg() || gnome() || gnome_old() || kde().
rework the code so that there is a difference between “this DE wants light mode” and “couldn’t figure out of this DE is in light or dark mode” - both of these are now represented by the “false” return value.
This isn’t that bad? Yes, having an enum with three variants would be better and more readable, but the code just defaults to light mode if nothing wants dark mode, and prefers dark mode even if separate impls want both light and dark mode.
With multiple impls, you have to resolve conflicts somehow. You could, for example, match on current DE/WM name, only using the current DE’s impl, defaulting to XDG, avoiding the problem entirely or just use first impl that doesn’t return “default” or “error”.
I don’t like AI generated code, having reviewed some disgusting slop before. But it’s better to criticize the code’s actual faults, like the incorrect impls (which you listed) or failing the Linux CI.
Yes, the DE-specific implementations is pointless (as far as I know, I use a WM), but the XDG implementation is actually used first, and the function returns true if any impl returns true, like xdg() || gnome() || gnome_old() || kde().
True, I must’ve read the code wrong when making the comment.
This isn’t that bad?
Yes, which is why I take issue with a PR (or rather what should have been a PR) that introduces crap code with clearly visible low effort improvements - the submitter should’ve already done that so the project doesn’t unnecessarily gain technical debt by accepting the change.
With multiple impls, you have to resolve conflicts somehow.
Yep, that’s why I think it’s important for the implementations to actually differentiate between light and fail state - that’s the smallest change and allows you to keep the whole detection logic in the individual implementations. Combine that with XDG being the default/first one and you get something reasonable (in a world where the separate implementations are necessary). You do mention this, but I feel like the whole two paragraphs are just expanding on this idea.
But it’s better to criticize the code’s actual faults (…)
I made a mistake with the order in which the implementations are called, but I consider the rest of the comment to still stand and the criticisms to be valid.
Holy shit. It’s the end of the world. AI helped code a few lines to detect dark mode on a Linux system!
The problem is here is not AI, the problem here is pushing code that didn’t even compile straight to main. It’s a huge collaborative project, running it as a one man show just doesn’t work anymore.
Had he made a PR/Draft, none of this would have happened. Hell, even his commit message says “needs to be reviewed before being deployed”
Which by that point they’d already fixed. They mentioned they also accidentally pushed to the wrong branch.
I dunno, this just looks like someone accidentally made a bad commit and instead of being mature about it and letting them fix it we get statements like “all bridges have been burned” and a split in the community. That’s a bit overblown, and if at the first sign of disagreement the decision is to blow up a project you’re making it very hard to work with you.
Well, in that case, fair enough.
However, given that a lot of contributors allude to “putting up” with him for years in the comments of the GitHub issue, it seems it wasn’t just a bad commit that broke the camel’s back
That’s fair, I don’t know what other things have come before. It just seems really odd that there was such a severe reaction to what happened here.
Gotcha
Well, the detection is broken for KDE and backwards in the XDG implementation (which is also only used as a fallback when the three DE-specific implementations fail, even though all of them actually support XDG so having separate implementations is pointless).
Also with the way it’s implemented, it will have unexpected results for users who have both KDE and Gnome installed (or at least have leftover configuration files) - if you for example used KDE in the past with a theme considered to be “dark” by this and now use Gnome and have it set to light mode, you will get dark mode GZdoom with no obvious reason why.
Oh and the XDG implementation is also very fragile and will not work on everyone’s system because it depends on a specific terminal utility being installed. The proper way would be to use a DBus library and get the settings through that.
And when somebody comes to fix it, they will have to figure out a) what’s so special about the DE-specific implementations that XDG wasn’t enough (they might just assume that XDG isn’t supported widely enough), b) learn how to detect dark theme properly on the DE they’re fixing, c) rework the code so that there is a difference between “this DE wants light mode” and “couldn’t figure out of this DE is in light or dark mode” - both of these are now represented by the “false” return value.
I don’t think a well written and functioning code made with AI assistance would get a response this strong, but the problem here is that the code is objectively bad and its (co-)author kept doubling down about something they probably barely even checked.
I mostly agree with you, but this is not quite true:
Yes, the DE-specific implementations is pointless (as far as I know, I use a WM), but the XDG implementation is actually used first, and the function returns true if any impl returns true, like
xdg() || gnome() || gnome_old() || kde().This isn’t that bad? Yes, having an enum with three variants would be better and more readable, but the code just defaults to light mode if nothing wants dark mode, and prefers dark mode even if separate impls want both light and dark mode.
With multiple impls, you have to resolve conflicts somehow. You could, for example, match on current DE/WM name, only using the current DE’s impl, defaulting to XDG, avoiding the problem entirely or just use first impl that doesn’t return “default” or “error”.
I don’t like AI generated code, having reviewed some disgusting slop before. But it’s better to criticize the code’s actual faults, like the incorrect impls (which you listed) or failing the Linux CI.
True, I must’ve read the code wrong when making the comment.
Yes, which is why I take issue with a PR (or rather what should have been a PR) that introduces crap code with clearly visible low effort improvements - the submitter should’ve already done that so the project doesn’t unnecessarily gain technical debt by accepting the change.
Yep, that’s why I think it’s important for the implementations to actually differentiate between light and fail state - that’s the smallest change and allows you to keep the whole detection logic in the individual implementations. Combine that with XDG being the default/first one and you get something reasonable (in a world where the separate implementations are necessary). You do mention this, but I feel like the whole two paragraphs are just expanding on this idea.
I made a mistake with the order in which the implementations are called, but I consider the rest of the comment to still stand and the criticisms to be valid.