refactor: replace speed button icon with text label

[nekoxuee]

Also moved some code into a function to make it easier to read. Initially I thought about rounding the number but seems like mpv already handles that.

[Samillion]

Awesome. Could you move it in modern-compact to match modern placement please?

modern-compact:

modern:

[nekoxuee]

Nice catch, I didn't notice it was placed there in that layout. Fixed now.

Also, do we prefer it to be "x1" or "1x"?

[Samillion]

Also, do we prefer it to be "x1" or "1x"?

I have no idea lol. What's the standard on this? I don't even know where to look because I never use speed control features anywhere.

[Samillion]

@Keith94 What do you think? x1 or 1x?

[Keith94]

I'm more familiar with seeing 1x. Maybe the x can be "floating" in the middle.

[nekoxuee]

That's the multiplication sing ×. This is how It would look:

edit: the only problem I could think by using that sign is with fonts rendering it differently

[Keith94]

@Xurdejl Good find. I like that one.

[Keith94]

edit: the only problem I could think by using that sign is with fonts rendering it differently

Checked it with my OpenSans Medium, it looks fine. Not sure about all other fonts though.

[Samillion]

That's the multiplication sing ×.

I like it, and matches other UI use for speed. Nice.

edit: the only problem I could think by using that sign is with fonts rendering it differently

Checked it with my OpenSans Medium, it looks fine. Not sure about all other fonts though.

It should display fine regardless of font. Either way, if we get reports about it, easy to replace. This one is a solid choice I think.

[Keith94]

What about changing tooltip to "Playback speed" or just "Speed" for simplicity.

[Samillion]

-return string.format("%g", mp.get_property_number("speed", 1)) "×"
+return string.format("%g", mp.get_property_number("speed", 1)) .. "×"

[Samillion]

What about changing tooltip to "Playback speed" or just "Speed" for simplicity.

We can do a locale review of all phrases, see what could improve in another issue/PR.

Then I'll use Google Translate to adjust the other languages.

[nekoxuee]

-return string.format("%g", mp.get_property_number("speed", 1)) "×"
+return string.format("%g", mp.get_property_number("speed", 1)) .. "×"

Let’s just pretend I didn’t fumble a 5 letter copy paste

[Samillion]

I think no one but me sees when I start a review or comment directly to code in a PR. I'm not actually sure.

I tested it before I posted the diff comment, I even started a review for the new font PR, it's still there.

Can you guys see it in #498 ?

Is it a repo setting that I'm not aware of?

[Samillion]

Either way, looks good to me. Want me to merge or do you want to adjust anything else?

[nekoxuee]

Can you guys see it in #498 ?

I can't

Is it a repo setting that I'm not aware of?

no idea since I don't use github that often

Either way, looks good to me. Want me to merge or do you want to adjust anything else?

it's ready

[Samillion]

Merging. Thank you very much for the effort.

[Samillion]

What about changing tooltip to "Playback speed" or just "Speed" for simplicity.

@Keith94 Should I open an issue to see if phrases need adjusting?

[Keith94]

What about changing tooltip to "Playback speed" or just "Speed" for simplicity.

@Keith94 Should I open an issue to see if phrases need adjusting?

Yeah sure, we can go through them.