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

"no artefacts" option makes the extension crash #31

Closed
aunetx opened this issue Apr 20, 2021 · 19 comments · Fixed by #52
Closed

"no artefacts" option makes the extension crash #31

aunetx opened this issue Apr 20, 2021 · 19 comments · Fixed by #52
Assignees
Labels
bug Something isn't working

Comments

@aunetx
Copy link
Owner

aunetx commented Apr 20, 2021

See https://redd.it/muxj77 for more infos

@aunetx aunetx self-assigned this Apr 20, 2021
@aunetx aunetx added bug Something isn't working Gnome 40 labels Apr 20, 2021
@CorvetteCole
Copy link
Collaborator

CorvetteCole commented Apr 20, 2021

Oh man I literally had this exact discussion with the gnome devs at one point. Part of why I stopped working on blur-provider. Got very complicated and I didn't have time. I think I'd like to migrate the intent of blur-provider in to this one. Being able to arbitrarily provide blur to applications feels like a natural next step for this extension.

You are the project maintainer, so please let me know if that sounds good to you. If so I'll start working on it!

@aunetx
Copy link
Owner Author

aunetx commented Apr 20, 2021

Yeah, I understand the "got very complicated"... Even just porting to gnome 40 took me almost 2 days, and it's still not finished due to this bug...
But overall I quite like working with gnome-shell in the sense that I use the software I help to extend every day!

If you want to merge your extension into mine, that would be really a great addition! If you need help, don't hesitate to ask me of course :)
Would you like to be granted write permissions to this repository now or maybe later?

For my part, I think I will try (after fully porting to 40) to add some settings (for #30, #22 and #14 notably), and try to fix some compatibility issues (for #10 and #25, and for dash-to-dock, as it is still not stable for gnome 40)

So tell me if you need anything to start working on this!

@CorvetteCole
Copy link
Collaborator

great to hear! I'll make a separate issue so I can track the tasks I have to do. I never fully finished blur-provider so this will be a good opportunity for me to implement the last couple things I had to do (ability to manually select an application to blur, better Wayland support, better fix for blur jumping in front of actors).

Regarding write-access, I don't need it immediately. I'm happy to work on it in a fork and then submit a pull request for now. I think once I've got my initial implementation integrated in write-access might be nice to have but until then no worries.

I've got to take a fine look at the code in this extension so I can try to match the coding style. Anything special I need to know?

This should be cool!

@aunetx
Copy link
Owner Author

aunetx commented Apr 20, 2021

Thanks a lot for this, Blur my Shell should then be a lot more complete than now :)
I understand for the write access! And tell me if you need to contact me in a more direct way than github issues (eg discord, element, what you need), as you may encounter some hard time with my extension :(

Well, I guess the main thing is to create connections with the connections.js helper so they are removed when the actor is destroyed or the extension disabled, like this (once it is added to this):

this._connections.connect(actor_to_connect_to, 'signal', () => {
    // what you want
});

Also, if possible (to make logging easier), you can add the following method to your classes:

_log(str) {
   log(`[Blur my Shell] ${str}`)
}

And if possible, try to 'use strict'
If you need to add preferences, it's a little harder (especially after gtk4, as glade does not work anymore), but ask me if you need then!

(I'm going to sleep, it's already 1:38 past tomorrow for me :/ so sorry if I do not answer now)

@CorvetteCole
Copy link
Collaborator

No worries! My Discord handle is CorvetteCole#9802, but I am also on telegram at t.me/coles. I've actually already added a preference, it is not too bad a process!

I will keep those things in mind. You've got a pretty extendable set of standard functions which will make it nice and easy. I have always preferred stricter languages!

I'll be tracking my progress on the initial merge in #35. After I get most the functionality of blur-provider working in blur-my-shell I will probably request write access and create my own little branch for slowly improving it :)

Screenshot from 2021-04-20 21-17-26

@aunetx
Copy link
Owner Author

aunetx commented Apr 21, 2021

That's cool, I'm glad my code style is not too bad!
And congrats for the preferences, that was simpler than expected so well done :)

I will give you write access before Sunday if that's ok for you, as I will have less time after that (still in university) :)

@CorvetteCole
Copy link
Collaborator

regarding vfuncs, I saw this which might be a good reference for it. https://gitlab.gnome.org/GNOME/gnome-shell/blob/master/js/ui/panelMenu.js#L38. I am also in university so I understand lack of time no worries :)

@aunetx
Copy link
Owner Author

aunetx commented Apr 21, 2021

Cool thanks, that's a good way to understand indeed :) but I wonder which actor do I need to extend, as Shell.BlurEffect is the buggy one, but I used the other actor's paint signal to update the blur effect...
And I still don't really understand if it is possible for me to extend for example Main.paneland change its paint vfunction, so I can update my blur effect from there (which is an ugly hack, but works pretty well)

@CorvetteCole
Copy link
Collaborator

Right, I'm not sure either. vfuncs are going to need extensive testing. I'm not sure which one to extend right now, I may do some testing and get back to you on that. I guess we can emit a custom signal on every repaint of a target actor using a custom extended actor paint vfunc?

@aunetx
Copy link
Owner Author

aunetx commented Apr 21, 2021

Yeah, that would definitively be the best solution...
I would love to have this bug fixed in gnome-shell, as it would simplify a lot this extension (and probably the additions you're going to add with #39)

@CorvetteCole
Copy link
Collaborator

CorvetteCole commented Apr 21, 2021

Right, that was a big problem facing me in the past. The solution will be needed in both the applications and shell parts of this extension. If we do this right it should be usable by both parts as well. I think creating a generic actor class that extends the actor and provides a paint vfunc that emits our signal would be the way to go. The question is then are we going to repaint ALL of our blur effects on that signal? I think we should probably figure out a way to connect the class containing a blur effect to the signal emitted by its "parent" actor so we can repaint on demand only the things that need to be repainted. This might mean extending those classes to have a vfunc we can connect to that aforementioned signal, not sure yet.

This probably won't be as important for the shell, but from a performance perspective a user could have dozens of blurred applications open at once so I'm trying to consider that

@aunetx
Copy link
Owner Author

aunetx commented Apr 21, 2021

With #38 fixed, the artefacts should be gone from top panel (at the expense of dynamic blur) :) but it is primordial for us to fix this issue anyway

Anyway, no I don't think we will need to update all our blur on paint signal! The important thing is to update the blur which is attached to the actor from which the paint signal is fired
For example, with blur_1 attached to actor_1 and blur_2 attached to actor_2, we will need to do:

actor_1.connect('paint', () => { blur_1.queue_repaint() };
actor_2.connect('paint', () => { blur_2.queue_repaint() };

So, whenever actor_1 is repainted (and the artefacts might appear), blur_1 is repainted; but not blur_2
In general, this methods works well because actors are repainted roughly every time the blur should be (and maybe more, so maybe some overhead is introduced, but not too much as it is not at all every frame)

In conclusion, our real need it to actually re-implement the paint signal (exactly like how it was before gnome 40); and find a way to add it to existing actors (like Main.panel for example, which would looove to have dynamic blur again)

@aunetx
Copy link
Owner Author

aunetx commented Apr 21, 2021

By the way, I pushed to extensions.gnome.org (once again, sorry :p) to let users have static blur (as it immensely improves the user experience)

@CorvetteCole
Copy link
Collaborator

That is a very concise explanation of what we need to do. I'll take a look at ways we could possibly do that. I wonder if we could essentially cast it to a different object with the overridden function within, but I don't know much JS so I will have to do a little research. Side effect of dynamic blur being means that dynamic backgrounds won't have the right blur applied since it looks like you are only getting the background once (I suspect you'll see a bug report regarding this soon).

@aunetx
Copy link
Owner Author

aunetx commented Apr 23, 2021

Yeah, static blur means a less responsive experience, but it the user do not have any extension that dramatically changes the way the panel is used, then nearly nothing changes :)

To be honest I really don't know much js either, I am basically typing things and seeing what works... So I guess with a lot of testing, we might come with a working solution :)

@aunetx
Copy link
Owner Author

aunetx commented Apr 24, 2021

I think I have something (thanks to /u/Scheengans on reddit):

const MyEffect = GObject.registerClass({ GTypeName: 'MyEffect' },
    class MyEffect extends Clutter.Effect {
        vfunc_paint(paintFlags) {
            log('repaint!');
            super.vfunc_paint(paintFlags);
        } 
});

// And then later:
our_actor.add_effect(new MyEffect());

I will investigate this tonight, and will push to extensions.gnome.org before (as I still not pushed the dash-to-panel fix)

@CorvetteCole
Copy link
Collaborator

That is a very very interesting few lines of code... Although that seems to just override the paint function of the effect? Not sure let me know how that goes and I'll test as well when I have time

@aunetx
Copy link
Owner Author

aunetx commented Apr 25, 2021

I implemented it in #52, works very well :)

The paint function of the effect is actually called each time the actor is painted, so we can do what we want there (as the effect does not actually do anything in itself, so overriding its paint function is possible)

@CorvetteCole
Copy link
Collaborator

Wow! Great news. This feels a bit like the magic bullet for our blur implementation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants