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.

Sign up for GitHub

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

Jump to bottom

F41 timed transitions broken (again) due to alpha channel (again) #67

Closed
ferdnyc opened this issue Jan 6, 2025 · 5 comments · Fixed by #69 or #70 · May be fixed by #68
Closed

F41 timed transitions broken (again) due to alpha channel (again) #67

ferdnyc opened this issue Jan 6, 2025 · 5 comments · Fixed by #69 or #70 · May be fixed by #68

Comments

@ferdnyc
Copy link
Contributor

ferdnyc commented Jan 6, 2025

See #51 for a previous report of this issue during the F38 timeframe, and my Fedora Discussions thread for the journey I've been on to get here, with F41.

So it appears (from #51) that the issue with RGBA images not working, and needing to convert them to RGB (without alpha channel) is sorta-known, but in the past the fix has been to replace the images in the repo with corrected ones.

IMHO that's not a real solution, because people will keep forgetting that the images need to be RGB and that's totally understandable. So, given that it's a technical limitation of the GNOME timed-background implementation, my proposal would be to implement a technical solution, and just ensure that the images are alpha-channel-free by converting them during the build/packaging process. If the image is already in the right format, then the conversion won't take very long.

@hrismarin mentioned (@ the Fedora Discussions thread) using optipng to process the images, but its lack of control over the processing gives me pause. I'd personally prefer to use ImageMagick, with which it's possible to say "ONLY remove the alpha channel from this image, and do nothing else to it".

(Specifically, that would take a form something like:)

MAGICK=/usr/bin/magick

install:
	$(MKDIR) $(WP_DIR)/default
	$(INSTALL) $(WP_NAME).xml $(WP_DIR)/default/$(WP_NAME).xml
	for tod in 01-day 01-night; do \
	  $(MAGICK) $(WP_NAME)-$${tod}.png -alpha off \
	  $(WP_DIR)/default/$(WP_NAME)-$${tod}.png ; \
	done;
	
	# ...

...in place of the current Makefile install rule. But it does mean making ImageMagick a package dependency / BuildRequires.
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Jan 6, 2025

Oh, see also-also my report against gnome-desktop @ gitlab.gnome, which I guess I'll now close because, while ideally the code should be able to deal with RGBA images, ultimately if it can't and the fix is to convert the images to RGB, then that's a solution that's out of the hands of the GNOME team.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Jan 6, 2025

Another option, thinking about this a bit more, would be to install a CI workflow on the repo that checks the image formats, and fail any PRs that cause RGBA PNGs to be installed.

I'd be willing to work on a PR along those lines, if that solution would be agreeable.

@luyatshimbalanga
Copy link
Contributor

Hello @ferdnyc,
thank you for tackling an old issue affecting PNG background. Please do and provide a comparison for either method as we can make a decision. Should you use CI workflow, make sure it will be compatible to Gitlab as we may mirror this repository.

This was referenced Jan 9, 2025
Open
Merged
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Jan 9, 2025

@luyatshimbalanga Done and done.

(The Python script used in the CI workflow would require a bit of tweaking for GitLab, since their CI doesn't support the same annotation markup as GitHub. But I could make the output of the script configurable with just a few additional lines of code. And even without those changes, the exit status of the process is the main thing that determines success or failure, and that'll work the same whether it's being run on GitHub, GitLab, or any other CI system.)

@luyatshimbalanga
Copy link
Contributor

Thank you for the changes. Looking at the PR, the fixes are logical. With the introduction of ImageMagick as a build dependency on #68 , perhaps we should do for Fedora 42 instead while we can focus in this ticket on #69 and I will merge it first.

@ferdnyc ferdnyc mentioned this issue Jan 18, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@ferdnyc @luyatshimbalanga