Conversation
|
I have merged and reordered some patches to reduce the amount of patches |
|
Hey @josselinonduty, as always thanks for the work you put in. While I might not be able to test it out, I like reviewing code. 🙂 In that regard, is there a reason on why we reverted the behavior of the kernel version so that now is shown by default as opt-out? Other than the small privacy benefit, I think it'd be also great on Deezer's side to not start getting all these kernel version numbers. They don't support Linux, so it'd be extra noise in my opinion. I'm still for this option to be opt-in or, even better, to not be user configurable at all as I don't really see a reason for it. What do you all think? |
|
Thanks for the review @randshell 🙌 I reverted to this version because I don't think we should tamper with the core behaviour of the app, because of legal reasons and also because it is not our purpose. I think our role is more to add optional stuff (i.e. opt-in features or switches) and fix issues that only occur on linux.
just to let users choose individually. I think we shouldn't change the default values without adding an easy way to change them back. Anyway, that's only my opinion. I really don't mind making it opt-out by default if ppl think it is better |
|
I still have to finish the review, probably during this weekend, but I wanted to talk about this versioning topic.
I agree, and I'd consider this to be an edge case. 😄
It could be any value, really. I'd personally avoid 1.0.0 since it could be too generic, like potentially used in development. The way I see it is that there's 3 possible options:
I'm open to discuss this. If we need more time to decide, could you submit a separate PR for this change, please? |
|
I've finished reading the last changes and LGTM. The comment about forking the third-party dependencies would be out of scope for this now, perhaps I could open an issue for tracking it. As for the hiding of the kernel version being opt-out, see my previous comment. |
I'd argue this is the worst option
Fair enough, I'll revert to fixed version 6.1.0 which indeed should be fine for quite a while.
I get your point. They could just track the exact amount of linux users with 6.1.0, they don't need to know if it is kernel 6.1 or 6.2 or sth.
I'm reverting this patch. We could discuss that on another thread, but I'm okay for 6.1 👍 |
|
What about this @randshell ? Also fixed broken links |
aunetx
left a comment
There was a problem hiding this comment.
Reviewing this again, looks good to me!
Solves #109