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 logging related to shader loading #1394

Open
wants to merge 2 commits into
base: next
Choose a base branch
from

Conversation

l4desu-mizu
Copy link

@l4desu-mizu l4desu-mizu commented Dec 9, 2024

Changelog

  • Category: Bug fixes
  • Description:
    • Fixed check for error when loading shaders via fg-window-rules (inverted condition)
    • I think the opened shader files are not properly closed when successfully read, so I added a fclose for the happy path

* the logs were printing errors when loading shaders with rules,
  signalling there was an issue when loading a shader, but the shaders
  would load fine just the error message would print on success instead
  of error
@l4desu-mizu l4desu-mizu marked this pull request as ready for review December 9, 2024 21:49
@l4desu-mizu l4desu-mizu changed the title Minor fixes fix logging related to shader loading Dec 10, 2024
@Swivelgames
Copy link

Swivelgames commented Dec 19, 2024

load_shader_source uses inverted boolean logic, so it returns true in the case of errors.

This change would break the logging related to shader loading, unless load_shader_source was updated to always return true, and return false in the case of errors.

I'm not recommending that, of course, but I am curious: Is there are particular issue that you're running into that warranted this PR? Or was this found while perusing the codebase?

@l4desu-mizu
Copy link
Author

l4desu-mizu commented Dec 19, 2024

load_shader_source uses inverted boolean logic, so it returns true in the case of errors.

So this would mean, that the logmessage should display if load_shader_source is true, which is the change here. In theory it would be nice to improve the logging here on what shader failed to load, but as I'm just dipping my toes I'm cautions in suggesting larger changes. (e.g. I found some missing nullpointer checks in the dbus api which causes picom to segfault. I've suggestions for that but waiting on feedback on this for now)

I was checking how the shaders are integrated in picom and how to configure them.
As I understand it, it would only change the behaviour of logging that a shader could not be loaded due to an error, when loading multiple shaders instead of stating that there was an error with the shader even if there is none.

Putting it in other words:
I was (initially) toying with the shaders and was irritated when I saw an error but the shader was properly displaying. Though before fixing my shader I was not seeing this error message. I was checking on the shader integration in general and found that the boolean check for loading multiple shaders is invers to the one when loading only the window_shader_fg shader.
Thus I assumed this was an oversight. I might be wrong, but for me that seems plausible.

I don't need this change, I just thought it might be nice to fix something (even minor) when I see it. That is, if my understanding is correct.

@l4desu-mizu
Copy link
Author

l4desu-mizu commented Dec 19, 2024

For comparison
picom v12.5 image

With this change:
image

The shaders configured in the picom config are used both times - I've terminated picom for the screenshots though.
For these functional shaders in the first screenshot picom v12.5 (upstream) states for each, that they are erroneous. In the updated version these messages are not printed - as they are technically false.

@Swivelgames
Copy link

Ah, no, you're very correct! My apologies, I tripped over myself. Carry on! 😅

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