blue-build / modules

BlueBuild standard modules used for building your Atomic Images
Apache License 2.0
22 stars 26 forks source link

feat: Introduce `wallpapers` module #135

Open fiftydinar opened 4 months ago

fiftydinar commented 4 months ago

This module will automatically copy wallpapers into system directory for all image variants.

If image is Gnome-based, it can additionally support advanced features:

Gnome additional support, which is not planned or not yet implemented:

Other DEs support:

I tested this thoroughly & it works great!

I made this module to be as easy to maintain as possible in the code. F.e, if some scaling option gets depreciated, it is very easy to remove it in the code, inside scaling_options variable. I also categorized the code & explained as much as possible.

Only thing that can be possibly adjusted in this PR is documentation & maybe some other minor things.

Image format support & file-size is mentioned as something that should be checked before applying. Image format support is dynamically changing, so I'm not sure is it worth to implement this. File-size check can be easily done, to prevent image bloat.

External wallpaper source is complex as mentioned, so we would probably not support that. Plus we don't know which source we would use. This module itself with current functionality is the most complex bash module made for BlueBuild as of now, so we have to think about implementing additional functionality carefully.

Related issue: https://github.com/ublue-os/bling/issues/130

xynydev commented 4 months ago

Thanks! Reviewing this is not a priority right now. I skimmed the source code just now, no glaring issues.
A couple notes though:

The config format feels a bit weird overall. Here's just a sketch for something different:

type: wallpapers
default-scaling: wallpaper
wallpapers:
   - file: filename-tile.jpg
   - dark: filename-center-dark.jpg
     light: filename-center-light.jpg
     scaling: centered
     default: true

or if the defaults shouldn't work like that:

type: wallpapers
default-scaling: wallpaper
wallpapers:
   - file: filename-tile.jpg
default-wallpaper:
     dark: filename-center-dark.jpg
     light: filename-center-light.jpg
     scaling: centered
fiftydinar commented 4 months ago

Why is the default wallpaper configured as an array?

I can fix that, I set them as array just because of convenience (already available sanitize_file_names variable & such).

What does zoom: all mean in practice when all specified wallpapers use a different scaling option (except the default)?

Per-wallpaper scaling is always overwriting global option. I can change the name of this option similar to your example & note that in recipe too (currently noted in README only).

Why use the light + dark syntax which might encounter unforeseen parsing errors

I use this format because current light+dark wallpaper is easier to manage & to notice compared to separating them. F.e., when having good amount of those wallpapers, you would not need to jump on finding & removing combination 1 by 1 (light by dark), but you would just remove a pair. I think that newlines are not a problem, except trailing newlines, which are removed (upstreamed in bluebuild-cli export too inside get_yamlarray function). Another factor is the convenience of the user, where he can include wallpaper files with whitespaces, without need to rename them. I automatically convert those whitespaces to character.

1st recipe looks kinda cluttered to me, with default wallpaper, light+dark wallpaper, wallpaper & scaling together.

2nd recipe looks better, but still not my taste, as I prefer light+dark wallpaper as a current format. I can maybe merge scaling into those, but I will have to think on how that will look.

tulilirockz commented 4 months ago

Guys maybe it would be interesting to have a "wallpaper-packs" option or something like that, because it would be awesome to add stuff like https://github.com/tulilirockz/ublue-os-wallpapers as easily as adding a single string like - wallpaper-packs: - $URL

xynydev commented 4 months ago

@tulilirockz Could be cool. We've discussed adding different ways to fetch wallpapers. I think git clone is a more promising way than custom rpm repos, though.

In the meantime, I saw that I actually kept the ublue wallpaper bling installer, so you can go ahead and add back a script there to get the wallpapers from your rpm if you wish: https://github.com/blue-build/modules/blob/main/modules/bling/installers/ublue-os-wallpapers.sh

tulilirockz commented 4 months ago

Sweet! Gonna make a PR adding them back up on the script :>

fiftydinar commented 4 months ago

I will mark this PR as draft, until it's investigated on how to support other DEs better. And until module code guidelines are formed.