Skip to content

Conversation

@ogios
Copy link

@ogios ogios commented Dec 13, 2024

fix #809

this make sure that all the icon's height are the same.
but some might considered "ok" size before may seem a bit off than usual:

BEFORE:
image

AFTER:
image

But we can change the icon size ourself (used to be 16, now 24):
image

So this is just one way of fixing the problem
i can close this if it's not a proper solution.

@ogios
Copy link
Author

ogios commented Dec 13, 2024

and also, should we find a proper size in pixmap function instead of just use the first one?

.and_then(|pixmap| pixmap.first())

or since we have re-scale as post-process, we can just choose the largest one.

@JakeStanger
Copy link
Owner

Thanks for this, haven't given a full review yet but looks good.

I think it would make sense to pick the next biggest size and scale that down. That'd be cheaper than always scaling the largest.

@ogios
Copy link
Author

ogios commented Dec 15, 2024

I think it would make sense to pick the next biggest size and scale that down.

i've added the functionality for this if i understand correctly.

One thing bothering me is that, whether the Vec<IconPixmap> is sorted with height, if so then we can use a better way to speed up the search.

@JakeStanger
Copy link
Owner

One thing bothering me is that, whether the Vec is sorted with height, if so then we can use a better way to speed up the search.

The spec is so incredibly vague and has so many incorrect implementations that I'd keep away from trying to do anything like that. The good news is in reality there will only be a few entries on rare occasions so the search will be fast enough always.

@JakeStanger JakeStanger changed the title Force scaling icon Tray - Force scaling icon Apr 10, 2025
@JakeStanger JakeStanger added M:Tray Related to the Tray module A:UX/UI Relating to user experience or interface. labels Apr 10, 2025
Copy link
Owner

@JakeStanger JakeStanger left a comment

Choose a reason for hiding this comment

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

I'm really sorry, going through old PRs and I didn't realise I left this review in draft. Let me know if you're still up for picking up the changes, if not I'll get it finished up and merged

.as_ref()
.and_then(|pixmap| pixmap.first())
// The vec is sorted(ASC) with size(width==height) most of the time,
// but we can not be sure that it'll always sorted by `height`
Copy link
Owner

Choose a reason for hiding this comment

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

There's a slight catch here that ideally needs to be incorporated. I'm hoping it shouldn't be too hard as it's just a switch on the value to check, but for vertical bars the bar "height" property actually refers to its width. We'd need to perform all operations here off the width instead.

Copy link
Author

Choose a reason for hiding this comment

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

changed to use width in find_approx_size 455545e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A:UX/UI Relating to user experience or interface. M:Tray Related to the Tray module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

vlc tray icon not respecting icon_size

2 participants