Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unused Serializable instances #10783

Closed

Conversation

Profpatsch
Copy link
Contributor

These two classes are not serialized anywhere (anymore).

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Due diligence

These two classes are not serialized anywhere (anymore).
@AudricV AudricV added template ignored The user didn't follow the template/instructions (or removed them) codequality Improvements to the codebase to improve the code quality labels Jan 23, 2024
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions

C Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

idea Catch issues before they fail your Quality Gate with our IDE extension SonarLint SonarLint

@snaik20
Copy link
Contributor

snaik20 commented Feb 3, 2024

Hi @Profpatsch,

Thank you for your contribution and the thought put into this pull request! I appreciate your effort in addressing the identified concern.

Considering the upcoming migration to Kotlin and Compose, it will likely address the concerns you've raised, making this change unnecessary in the long run.

To focus our collective efforts optimally, I recommend:
- Closing this pull request: Let's close this PR for now. Once we have a clearer picture of how the migration handles the relevant functionality, we can revisit this PR or, if needed, create a new one tailored to the new framework.
- Focusing on reported bugs: In the meantime, your expertise would be highly valuable in addressing existing bugs in the codebase. This would contribute significantly to improving the project's stability and user experience.

Additional feedback:
- Context and justification: While the proposed change seems well-intentioned, providing clear context and rationale would make the review process smoother. This could include clearly articulating the reasoning behind changes, especially those potentially impactful. Detail steps taken to validate their necessity (e.g., verifying serialization usage).
- Potential impact: It's always helpful to explicitly mention the potential impact of changes, especially those with broader implications. This allows reviewers to assess the trade-offs more effectively.

I hope this perspective helps! Feel free to discuss any questions or concerns you might have.

CC: @opusforlife2, @AudricV

@opusforlife2
Copy link
Collaborator

Hey snaik, good to see the reviews starting! Also, we get notifications, so no need to ping by username.


I'm no dev, but for tiny code quality improvements like this, is it worth thinking about them too much before merging? As long as it's clear that there will be no regression, I mean.

@snaik20
Copy link
Contributor

snaik20 commented Feb 3, 2024

Hey snaik, good to see the reviews starting! Also, we get notifications, so no need to ping by username.

SG. Will avoid tagging by username.

I'm no dev, but for tiny code quality improvements like this, is it worth thinking about them too much before merging? As long as it's clear that there will be no regression, I mean.

While the code change seems small, it's essential to carefully consider its potential impact before merging, even in the absence of obvious regressions. Here's why:

  • Wide Usage of PlayQueueItem: The PlayQueueItem class plays a significant role in the app's functionality. Any modification to it, however minor, has the potential to ripple through numerous parts of the code, potentially leading to unexpected issues.
  • Serialization Nuances: Changes to serialization can be particularly delicate. They can affect how data is stored and transmitted, potentially causing compatibility issues with existing data or integrations. Verifying the usages of the usages is crucial to ensure seamless functionality.

@Profpatsch
Copy link
Contributor Author

Ehm, this is a pretty easy simplification that does not touch serialization. It just removes unused implementations. It compiles, it’s good to go.

@Stypox
Copy link
Member

Stypox commented Mar 28, 2024

Unfortunately it's not true that if it compiles, then it works. This is what I get when I start the player. I agree with @snaik20 , it's better to avoid risking to introduce regressions if the improvements to the code are this small. But thank you anyway!

Serialization failed for:
java.io.NotSerializableException: org.schabi.newpipe.player.playqueue.PlayQueueItem
at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1240)

@Stypox Stypox closed this Mar 28, 2024
@Profpatsch
Copy link
Contributor Author

Is it seriously resolving instances at runtime? what a horrible design

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codequality Improvements to the codebase to improve the code quality template ignored The user didn't follow the template/instructions (or removed them)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants