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

output/background: fix config ignoring fallback color #8558

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sahinf
Copy link
Contributor

@sahinf sahinf commented Feb 3, 2025

currently, the output background command handler prematurely returns with an error if the background file cannot be accessed. It should only error if user did not provide fallback color.

closes #8556

@emersion
Copy link
Member

emersion commented Feb 3, 2025

cc @mstoeckl

Copy link
Contributor

@mstoeckl mstoeckl left a comment

Choose a reason for hiding this comment

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

I've tested this a bit; while it works when the file does not exist, I find that it fails to parse the color argument when the file is accessible; swaymsg output '*' background ~/test.png center '#ff00ff' gives an "Error: invalid output subcommand: #ff00ff".

Also, there's a preexisting issue in sway/commands/output/background.c that, while I don't think you need to fix it entirely, should not be exacerbated: when updating variables like output->background_option, the previous value should be free()d to avoid leaking it, and if strdup() fails the function should return with an error message (like return cmd_results_new(CMD_FAILURE, "Unable to allocate resource");.)

sway/commands/output/background.c Outdated Show resolved Hide resolved
sway/commands/output/background.c Outdated Show resolved Hide resolved
@@ -117,23 +117,13 @@ struct cmd_results *output_cmd_background(int argc, char **argv) {
}

bool can_access = access(src, F_OK) != -1;
if (!can_access) {
sway_log_errno(SWAY_ERROR, "Unable to access background file '%s'",
Copy link
Contributor

@mstoeckl mstoeckl Feb 3, 2025

Choose a reason for hiding this comment

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

This error message, and maybe also the swaynag warning, would be useful to keep in the case where the file cannot be accessed and there is a fallback color.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree it would be useful to log this to SWAY_DEBUG or SWAY_INFO, but it might be obstructive to swaynag it. Like if I provided a fallback color and then it ended up using that fallback color because it couldn't access the file, then I would not want to be bothered about it because it's working as I instructed it to.

@sahinf
Copy link
Contributor Author

sahinf commented Feb 3, 2025

To outline what I was going for, I present 4 cases:

GOOD file BAD file
GOOD fallback same as before, use file log that sway is using fallback, use fallback
BAD fallback swaynag about malformed fallback, use file swaynag about malformed fallback, return with error
output * bg "$XDG_DATA_HOME/wallpapers/landscape-mountain-walk.jpg" fill #FF69B4
output * bg "$XDG_DATA_HOME/wallpapers/landscape-mountain-walk.jpg" fill #FF69B
output * bg "$XDG_DATA_HOME/wallpapers/landscape-mountain-walk.jp" fill #FF69B4
output * bg "$XDG_DATA_HOME/wallpapers/landscape-mountain-walk.jp" fill #FF69B

@mstoeckl
Copy link
Contributor

mstoeckl commented Feb 5, 2025

I've tested this, and think the logic is good now. Re code style, I don't have time today to look things over in detail. See CONTRIBUTING.md -- typical issues are indentation and where exactly * goes. Fixup commits can be squashed.

currently, the output background command handler prematurely
returns with an error if the background file cannot be accessed.
It should only error if user did not provide fallback color.

closes swaywm#8556

Changes

- Introduce variables to avoid uneccessary writing on output members
- Log a debug message when fallback is being used over inaccessible
   file
- Always parse the background color and swaynag warn if it is incorrect

- when updating output member variables, free previous values
- add cleanup label and goto it if `strdup` fails
- Move output->member initializations to before parsing fallback, Also
free and init output->background as well
@sahinf sahinf force-pushed the fix-fallback-color-ignored branch from bbd43a5 to d75dfba Compare February 5, 2025 03:23
@sahinf
Copy link
Contributor Author

sahinf commented Feb 5, 2025

I've tested this, and think the logic is good now. Re code style, I don't have time today to look things over in detail. See CONTRIBUTING.md -- typical issues are indentation and where exactly * goes. Fixup commits can be squashed.

Thank you for reviewing so quickly! I fixed the *. It now adheres to Macros, Error Codes, Construction/Destruction Functions, Names, Line Length, Indentation.

It does not adhere to Brackets with if (cond) goto cleanup. I wanted to follow sway/config.c because it looks so clean. Should I still break them up?

Copy link
Contributor

@mstoeckl mstoeckl left a comment

Choose a reason for hiding this comment

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

At this point, the changes look good to me.

Should I still break them up?

I think it's probably fine but am not certain; the project maintainers may prefer either way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Fallback color does not fill the screen background when the background file does not exist.
3 participants