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

Provide on/off icons for soundvolume_mute #2585

Merged
merged 2 commits into from
May 25, 2024

Conversation

jlaur
Copy link
Contributor

@jlaur jlaur commented May 25, 2024

@jlaur jlaur requested review from ghys and kaikreuzer as code owners May 25, 2024 10:44
@jlaur
Copy link
Contributor Author

jlaur commented May 25, 2024

I found this script also which I'm not sure about - is it still in use?

@lolodomo
Copy link
Contributor

I found this script also which I'm not sure about - is it still in use?

It is better to maintain it I believe.

@jlaur
Copy link
Contributor Author

jlaur commented May 25, 2024

I found this script also which I'm not sure about - is it still in use?

It is better to maintain it I believe.

I'm not sure how, can you guide me a bit? The script doesn't copy soundvolume_mute.svg currently, and it copies only soundvolume-100.svg into soundvolume.svg. What is the logic behind this?

@lolodomo
Copy link
Contributor

lolodomo commented May 25, 2024

I would add these lines:

cp -f soundvolume-100.svg   soundvolume_mute-off.svg
cp -f soundvolume_mute-on.svg   soundvolume_mute.svg

Only soundvolume_mute-on.svg is really needed, the other can be copied from other.

Signed-off-by: Jacob Laursen <[email protected]>
@jlaur
Copy link
Contributor Author

jlaur commented May 25, 2024

cp -f soundvolume-100.svg soundvolume_mute-off.svg
cp -f soundvolume_mute-on.svg soundvolume_mute.svg

Done. Do you know what the script is used for? Since we have all icons (even duplicates) in the icons folder, the script could copy 1:1?

cp -f soundvolume_mute.svg      soundvolume_mute.svg
cp -f soundvolume_mute-off.svg  soundvolume_mute-off.svg
cp -f soundvolume_mute-on.svg   soundvolume_mute-on.svg

And also:

it copies only soundvolume-100.svg into soundvolume.svg. What is the logic behind this?

It doesn't copy soundvolume-33.svg, soundvolume-66.svg, etc.?

Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@lolodomo
Copy link
Contributor

The logic of this script was to create all the expected icons from a minimal list of not duplicated icons and to be sure that icons that should be identical are really the same.
By running it you are sure for example that soundvolume.svg is not different than soundvolume-100.svg.
If we don't use it, we have to be sure for example in your case that you really copy/paste the existing files and you did not create a new icon.

I have just checked the appearance of the new icons, I assumed that you just copied the existing icons. I am sure at 99.999% this is what you did ;)

@jlaur
Copy link
Contributor Author

jlaur commented May 25, 2024

I have just checked the appearance of the new icons, I assumed that you just copied the existing icons. I am sure at 99.999% this is what you did ;)

I copied them according to your suggestion in the core PR. 🙂

@lolodomo lolodomo merged commit fabafbf into openhab:main May 25, 2024
5 checks passed
@lolodomo lolodomo added this to the 4.2 milestone May 25, 2024
@lolodomo lolodomo added the enhancement New feature or request label May 25, 2024
@jlaur jlaur deleted the soundvolume_mute-icons branch May 25, 2024 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants