Skip to content

Enhancement: update deps#110

Merged
randshell merged 11 commits intomasterfrom
josselinonduty/issue109
May 20, 2025
Merged

Enhancement: update deps#110
randshell merged 11 commits intomasterfrom
josselinonduty/issue109

Conversation

@josselinonduty
Copy link
Collaborator

@josselinonduty josselinonduty commented Apr 20, 2025

Solves #109

@josselinonduty josselinonduty marked this pull request as ready for review May 14, 2025 08:38
@josselinonduty
Copy link
Collaborator Author

I have merged and reordered some patches to reduce the amount of patches

@randshell
Copy link
Collaborator

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?

@josselinonduty
Copy link
Collaborator Author

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.
I totally agree with privacy;
However, I kind of disagree with the noise part. Sending 6.1.0 is already noise and it is meaningless on top of that since the "standard" kernel value changes regularly making it completely unusable. In that case, it would make more sense to put a really dummy value like 1.0.0 or 1.33.7 idk, but why 6.1.0?

to not be user configurable at all as I don't really see a reason for it

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

@randshell
Copy link
Collaborator

I still have to finish the review, probably during this weekend, but I wanted to talk about this versioning topic.

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.

I agree, and I'd consider this to be an edge case. 😄

Sending 6.1.0 is already noise and it is meaningless on top of that since the "standard" kernel value changes regularly making it completely unusable. In that case, it would make more sense to put a really dummy value like 1.0.0 or 1.33.7 idk, but why 6.1.0?

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:

  1. Don't modify the original behavior and use a fixed Windows version. - I don't like it since it falsely boosts the number of Windows users.
  2. Slightly change the original behavior by using a fixed version representing the Linux users. - As suggested in comment, I've picked 6.1.0, but it really doesn't matter which value we pick as long as it doesn't conflict with MacOS / Windows. So even if it's outdated, it was a way, in my opinion, to represent the Linux users when they check the statistics based on User Agent.
  3. Don't change the original behavior but fix it so that the exact kernel versions are picked up instead. - I'd be against this since they'd start getting versions that to them would be random and meaningless, since they're not officially tracking the installations on Linux distros. 6.1.0 is a Linux kernel version so it made sense to me to suggest this. Of course, 6.1.0 could be meaningless to them too, but at least it's one value rather than many of them of the various kernels.

I'm open to discuss this. If we need more time to decide, could you submit a separate PR for this change, please?

@randshell
Copy link
Collaborator

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.

@josselinonduty
Copy link
Collaborator Author

@randshell

The way I see it is that there's 3 possible options:

  1. Don't modify the original behavior and use a fixed Windows version. - I don't like it since it falsely boosts the number of Windows users.

I'd argue this is the worst option

  1. Slightly change the original behavior by using a fixed version representing the Linux users. - As suggested in comment, I've picked 6.1.0, but it really doesn't matter which value we pick as long as it doesn't conflict with MacOS / Windows. So even if it's outdated, it was a way, in my opinion, to represent the Linux users when they check the statistics based on User Agent.

Fair enough, I'll revert to fixed version 6.1.0 which indeed should be fine for quite a while.

  1. Don't change the original behavior but fix it so that the exact kernel versions are picked up instead. - I'd be against this since they'd start getting versions that to them would be random and meaningless, since they're not officially tracking the installations on Linux distros. 6.1.0 is a Linux kernel version so it made sense to me to suggest this. Of course, 6.1.0 could be meaningless to them too, but at least it's one value rather than many of them of the various kernels.

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 open to discuss this. If we need more time to decide, could you submit a separate PR for this change, please?

I'm reverting this patch. We could discuss that on another thread, but I'm okay for 6.1 👍

@josselinonduty
Copy link
Collaborator Author

What about this @randshell ?
I wanted to avoid regenerating patches. I added a small notice.

Also fixed broken links

Copy link
Owner

@aunetx aunetx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing this again, looks good to me!

@randshell randshell merged commit f8c1544 into master May 20, 2025
2 checks passed
@josselinonduty josselinonduty deleted the josselinonduty/issue109 branch May 21, 2025 06:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants