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

Fix applications service error #314

Merged
merged 1 commit into from
Aug 22, 2022
Merged

Fix applications service error #314

merged 1 commit into from
Aug 22, 2022

Conversation

jrahmatzadeh
Copy link
Contributor

The service will be undefined when user don't use application blur.

Since ?. is supported in 40 and higher, we can use that to avoid calling undefined properties.

Related to #312

The service will be undefined when user don't use application blur.
Since `?.` is supported in 40 and higher, we can use that to avoid calling
undefined properties
@TanawatJukmongkol
Copy link

Yup. Looks like that's the fix. I think in the long run, using monads to check for undefined or null, instead of if statements could be beneficial (that's just my thoughts on this).

@TanawatJukmongkol
Copy link

TanawatJukmongkol commented Aug 22, 2022

Now that I think about it... If this.service is undefined, then this could mean there's some undefined behavior else where (which, if that's the case would make this only a work around). And not calling the service.unexport might lead to memory leak (because this.DBusImpl will not be unexported)?

@jrahmatzadeh
Copy link
Contributor Author

@TanawatJukmongkol
No, that's happening because the enable method hasn't been called before (as I explained in the commit).
And ?. is just a quick fix because I didn't want to change the architecture.
Basically, when it's not enabled, the first line of disable function should return.

This error is not easy to find on review time, that's why I didn't catch it on ego review.

@TanawatJukmongkol
Copy link

TanawatJukmongkol commented Aug 22, 2022

@justperfection Ahh, I see. Thank you for the clarification. 😊

I have to confess. I don't know much about gnome extension development, let alone the inner working of this extension, so I had to make lots of assumptions based on the source code, since there's no documentation. The code is super clean, and easy to understand though.

@aunetx
Copy link
Owner

aunetx commented Aug 22, 2022

Thank you very much, I'm kinda dumb and forgot that I choose to disable every component, even when they haven't been enabled before... That's stupid and I will change this behaviour, but at least this will fix the crash very well :)

@aunetx aunetx merged commit 3d3e026 into aunetx:master Aug 22, 2022
@jrahmatzadeh jrahmatzadeh deleted the service-error branch August 22, 2022 07:46
pull bot pushed a commit to TheRakeshPurohit/blur-my-shell that referenced this pull request Oct 24, 2023
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