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

Cleanup DelegateActor #2134

Closed
wants to merge 15 commits into from
Closed

Cleanup DelegateActor #2134

wants to merge 15 commits into from

Conversation

lenemter
Copy link
Member

@lenemter lenemter commented Dec 3, 2024

@lenemter lenemter requested a review from a team December 3, 2024 17:06
@leolost2605
Copy link
Member

I'll continue to argue against committing to this approach because

  1. Make Picture-in-Picture "mask" the window instead of drawing a region #658 is a new feature which we can justify to bring to wayland only
  2. I'm really, really looking forward to reverting this one day and currently it's just an easy git revert for a single commit
  3. Judging from your commit history this might introduce a crash somewhere, we just haven't found it yet
  4. For that reason if you really want it I'd argue for accepting some code duplication and do it in the pip plugin only so that if it brings a regression it's limited to pip and doesn't affect panels

Once again this is my opinion and i stand by it but if enough people think otherwise there is not much I can do 🤷

@lenemter
Copy link
Member Author

lenemter commented Dec 3, 2024

  1. I don't like justifying bringing a feature to Wayland only, we literally have code that works on both X11 and Wayland.
  2. When we will be dropping X11 (I really hope this will happen very soon) the removal of this class will be very trivial and every method from this class we use can be replaced in 1-3 lines of code.
  3. Solid point. I'm not good at that. I already got a number of crashes when developing this branch, I'll add more null checks just to be sure.
  4. TBH I wanted to bring PiP plugin in-tree for a long time.

@lenemter lenemter deleted the lenemter/cleanup-delegate-actor branch December 6, 2024 14:55
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.

2 participants