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

Add effect to emit 'paint' signal #52

Merged
merged 5 commits into from
May 26, 2021
Merged

Add effect to emit 'paint' signal #52

merged 5 commits into from
May 26, 2021

Conversation

aunetx
Copy link
Owner

@aunetx aunetx commented Apr 25, 2021

I successfully added the effect to fix #31, and it works great!
I tested it for the panel, it works as great as the ancient 'paint' signal (so there are still some artefacts, due to windows shadows...).

We may need a way to instantly add this effect and connect it, keep track of it and maybe clean it afterwards? Just like we do for the connected signals, it would simplify the design a bit.

@CorvetteCole
Copy link
Collaborator

I think I understand what you mean, but it looks like you are already adding the connection to that modules connection object. What more did you have in mind?

@CorvetteCole CorvetteCole added the enhancement New feature or request label Apr 30, 2021
@aunetx
Copy link
Owner Author

aunetx commented May 3, 2021

What I was thinking about is simply that, when we remove the blur effect from the actor, with this method we will need to remove the EmitPaintSignal effect too (or it could be duplicated, or cause errors/overhead...)

So I was thinking about a method very similar to the one in connections.js, but maybe adapted to use this thing?

@CorvetteCole
Copy link
Collaborator

oh that's an interesting point, we don't want to needlessly emit extra signals. I'm not sure how we would implement this, but I think it is definitely a good idea. Maybe a PaintSignals.js class. Did you want to code that or should I put it on my to-do?

@aunetx
Copy link
Owner Author

aunetx commented May 4, 2021

Yeah maybe a dedicated class... Will you use this effect a lot for #39? If yes, this should definitively be added, but if not maybe just keeping track and removing the effects within the extension (for panel and overview) could be enough...

Anyway, if you want to work on this you can do it! And if not I will do this in some days/weeks anyway :)

@CorvetteCole
Copy link
Collaborator

CorvetteCole commented May 4, 2021

It is pretty essential for #39. Artifacts on the blur effect of windows are really common since they are often over other actor shadows and moved around a lot. So will definitely be used there! I'm not going to start working on the class yet since I am pretty busy at the moment, but I will let you know before I do if I get to it first so we don't duplicate efforts :)

I also have a few other ideas about unifying blur effects in general, perhaps we should have all of our blur effects listen for a signal that sigma or brightness has changed instead of having methods in each module to set sigma? This is further down the road of course. I think I remember seeing something like this in the dash module you wrote.

@aunetx
Copy link
Owner Author

aunetx commented May 16, 2021

I'll start to see what I can do about the paint signal :)

This is a good idea, it may be hard to keep track of everything else! Would you like to try to do this?

@aunetx aunetx requested a review from CorvetteCole May 16, 2021 15:26
@aunetx
Copy link
Owner Author

aunetx commented May 21, 2021

I think this is good enough to be merged! But I'll wait for your review first, don't want to merge it if the model is not adapted to #39 :)

By the way, if you have dash-to-dock, you can test it with this PR, I did not test it enough because I don't have it installed... But it should be ok though!

@aunetx
Copy link
Owner Author

aunetx commented May 21, 2021

I will try to entirely remove artefacts BTW, I think that I have a very hacky solution (being to track windows, and update the panel blur when they are moved... because their shadow is still creating artefacts, so I may be better with this).

@CorvetteCole
Copy link
Collaborator

sorry for such a late response. I started a new internship only a few days ago and have been very very busy with it and moving. I'm going to be spending my weekends working on this again after the next two where I will be away. sorry again

Copy link
Collaborator

@CorvetteCole CorvetteCole left a comment

Choose a reason for hiding this comment

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

I like these changes and think this is a pretty good way of handling things. A great workaround

@aunetx
Copy link
Owner Author

aunetx commented May 26, 2021

Cool thanks for the review, will merge this right now :)

And don't worry I really understand for the delay! I'm not very focused on the extension too, feel free not to answer for as long as you want :)

@aunetx aunetx merged commit 5a197c1 into master May 26, 2021
@aunetx aunetx deleted the emit-paint-signal branch June 6, 2021 11:13
weblate pushed a commit to weblate/blur-my-shell that referenced this pull request Nov 9, 2023
Adds support for better dynamic blur in gnome 40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"no artefacts" option makes the extension crash
2 participants