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

Add support for auto-scaling images #9

Closed
wants to merge 1 commit into from

Conversation

dhaavi
Copy link

@dhaavi dhaavi commented May 4, 2022

The idea behind this PR is that different implementations need different optimal image sizes and not all tray implementations are good at scaling the images. The new public variable allows for developers to quickly adapt the library to their target distro, or configure their preferred size via config without having to ship different binaries with different image sizes for different distros. It is not meant as a highly exposed feature, but as a small helpful addition for those who care to read the docs.

@dhaavi
Copy link
Author

dhaavi commented May 4, 2022

Ah, yes, I've meant the variable to be empty from the get go, but committed the testing value... As stated, the idea is that this is an advanced option - opt-in only.

@andydotxyz
Copy link
Member

Thanks for this, very sensible, the "opt in" approach works well.

However exposing a public global like this leads to unavoidable race conditions. Would it be OK to match the other setters (like SetIcon) so we can handle that complexity behind a method call if required?

@dhaavi
Copy link
Author

dhaavi commented May 5, 2022

Thinking about this more, I am unsure what the best approach is.

Ideally, icons would not be scaled on every call, but once by the user of the library.
So, maybe we could instead provide a utility function that just does the scaling and you would supply the result to SetIcon?

If scaling is only needed once, users could also just do systray.SetIcon(systray.ScaleIcon(iconData, 32)).

@andydotxyz
Copy link
Member

That is a good plan.
Then again, it is probably easier for the developer to scale in their code because they have the source data.
If we do it we have to decode, scale, encode then it can be set.
Perhaps it is a documentation issue?

@dhaavi
Copy link
Author

dhaavi commented May 9, 2022

Yes, I have now also done it in my code directly.
I agree the best way forward is to give an example how to easily scale the images.

You can find an example for scaling a PNG icon here: safing/portmaster-ui@5008392

@andydotxyz
Copy link
Member

As discussed this is probably a documentation issue, so I have opened #31 in place of this proposed change.

@andydotxyz andydotxyz closed this Nov 13, 2022
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.

2 participants