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

Default to using the Awesome theme font in spotify-widget #365

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

Conversation

chaorace
Copy link
Contributor

This change switches to using the theme font (beautiful.font) instead of 'Play 9'. This change brings the widget more in line with the other awesome widgets, particularly since Play is not used as the default in any other widget.

chaorace added 2 commits July 13, 2022 15:10
Switch to the configured beautiful default font instead of arbitrarily
choosing Play 9
@chaorace chaorace changed the title Spotify widget use theme font Default to using the Awesome theme font in spotify-widget Jul 13, 2022
@streetturtle
Copy link
Owner

Hi! Thanks for the PR!
There was some discussion here and here about not making beautiful a dependency, as it might be initialised after the widget.
In order to override the default font, one can set the font parameter to be beautiful.font, what do you think?

@chaorace
Copy link
Contributor Author

@streetturtle It seems as though beautiful is already a dependency for several other widgets in the package, though? Why are some allowed to use it and not others? Shouldn't a consistent package-wide default be agreed upon before deciding to not use the theme font?

@Aire-One
Copy link
Contributor

Aire-One commented Jul 21, 2022

AFAIR, the "this library shouldn't use beautiful" comes from #274.

The main idea is that some well-known libraries like https://github.com/lcpz/lain and https://github.com/lcpz/awesome-copycats enforces a pattern where the user defines the screen decoration (aka the desktop bar and widgets) in the theme.lua file. At this point, beautiful can't be initialized, so it makes all widget with values based on it unusable.

The fs-widget was the first (and only ?) to get the "new pattern" to not depends on beautiful. I'm not very active on this repository, so I don't know how it went after that.

The idea of the "new pattern" I have proposed with #277 is that the themable-values are taken with the priority waterfall config[prop] = args[prop] or beautiful[prop] or value. It is read as :

  1. if the user passes themable-value by function parameters (for example: local w = some_widget({ theme = { bg = "#F00" } })), the widget will use this parameter value,
  2. if the parameter is missing (for example because the user did local w = some_widget()), the widget will use beautiful.bg,
  3. then if beautiful isn't available (because the theme doesn't define the property, or because beautiful isn't initialized), it'll take the default value as defined by the widget internals.

So beautiful should still be a default for the user in a normal usage (aka not the awesome-copicats pattern).

@@ -33,7 +34,7 @@ local function worker(user_args)

local play_icon = args.play_icon or '/usr/share/icons/Arc/actions/24/player_play.png'
local pause_icon = args.pause_icon or '/usr/share/icons/Arc/actions/24/player_pause.png'
local font = args.font or 'Play 9'
local font = args.font or beautiful.font
Copy link
Contributor

Choose a reason for hiding this comment

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

Following my comment, here is what I think should be the reasonable way to do :

Suggested change
local font = args.font or beautiful.font
local font = args.font or beautiful.font or "Play 9"

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