wwmm / easyeffects

Limiter, compressor, convolver, equalizer and auto volume and many other plugins for PipeWire applications
GNU General Public License v3.0
6.45k stars 269 forks source link

Reimplementing locale format #1436

Closed Digitalone1 closed 2 years ago

Digitalone1 commented 2 years ago

EasyEffects uses always C locale by default, even if the system has set a different one.

Indeed spinbuttons which used to format values according to system locale in gtkmm 4.0, now are always at C locale. Maybe it's a difference made by plain gtk 4.0 or libadwaita?

As a result, even setting the L flag in ftm::format() (e.g {0:L}) results in unmodified strings because it seems that the whole appication is set to C locale and does not follow the locale one.

wwmm commented 2 years ago

Maybe it's a difference made by plain gtk 4.0 or libadwaita?

If I remember well gtkmm4 handles locale differently from gtkmm3. So it is possible there is something more we have to do to make locale work. I will see if they work in gtk-demo. If yes we can look at what they are doing there.

wwmm commented 2 years ago

They seem to work both on gtk and libadwaita demo. At least on spinbuttons that do not have custom parser. I will see if it works on ours when we do not set a custom formatting.

wwmm commented 2 years ago

I will see if it works on ours when we do not set a custom formatting.

No. I doesn't work for us in any case. There is definitely something that the demos are doing and we aren't.

wwmm commented 2 years ago

I tried to do some tests with LANG=pt_BR.UTF-8 ./easyeffects and it did not even change the labels to Portuguese. I am almost this worked before. I wonder if something has changed in the last gtk4 update.

wwmm commented 2 years ago

Never mind. It works with LANG=pt_BR.UTF-8 easyeffects. The problem was that my local build had the locale path set to somewhere else. Back to the number locazation problem.

wwmm commented 2 years ago

I still do not understand why LC_NUMERIC is being honored by adwaita-1-demo but not by us. The animation tag of the demo has a spinbutton where we can test the locale. I do not see anything special in the demo that could explain what is happening. So maybe we are doing something we should not be doing.

wwmm commented 2 years ago

Based on this locale should be working automatically https://docs.gtk.org/gtk4/func.disable_setlocale.html. It really feels that something I did is preventing it from happening. I wonder what.

wwmm commented 2 years ago

I have been doing some tests with util::warning(setlocale(LC_ALL, nullptr)). Before gtk is initialized its output is C. If we put this warning inside one of the widgets create functions. Like the one in application_ui.cpp the output is

LC_CTYPE=en_US.UTF-8;LC_NUMERIC=C;LC_TIME=pt_BR.UTF-8;LC_COLLATE=en_US.UTF-8;LC_MONETARY=pt_BR.UTF-8;LC_MESSAGES=en_US.UTF-8;LC_PAPER=pt_BR.UTF-8;LC_NAME=en_US.UTF-8;LC_ADDRESS=en_US.UTF-8;LC_TELEPHONE=en_US.UTF-8;LC_MEASUREMENT=pt_BR.UTF-8;LC_IDENTIFICATION=en_US.UTF-8

It is almost right. For some reason gtk is setting LC_NUMERIC=C. I still do not know why. I hope it isn't because I am creating widgets inside C++ namespaces...

Digitalone1 commented 2 years ago

I don't have any clue. Maybe setting the locale as we did with gtkmm could solve the issue.

https://github.com/Digitalone1/easyeffects/blob/pulseaudio-legacy/src/util.cpp#L48-L60

wwmm commented 2 years ago

Maybe setting the locale as we did with gtkmm could solve the issue.

I will see what happens once I have time. But considering I see the locale working both on the gtk and on the adwaita demos something In EasyEffects code is breaking gtk. The question is what. The only thing I could think of were the namespaces because they do not exist in C. But on a second thought it does not make sense. GTKMM also uses them. And util::warning(setlocale(LC_ALL, nullptr)) shows gtk is setting the locale and it is visible inside each plugin namespace.

wwmm commented 2 years ago

@Digitalone1 I have found the source of the problem but not a solution yet. GTK is actually setting the locale correctly. In my case that would be setting LC_NUMERIC=pt_BR.UTF-8. But the command that initializes PipeWire pw_init(nullptr, nullptr) is resetting it to C. Don't ask me why...

I think that when we used gtkmm totally by chance it initialized the locale after the call to pw_init. Now things are initialized in an order that is a little different.

wwmm commented 2 years ago

I looked at PipeWire's code and they reset the locale to C indeed https://gitlab.freedesktop.org/pipewire/pipewire/-/blob/master/src/pipewire/pipewire.c#L418. Why the hell they have to do this...

wwmm commented 2 years ago

I have opened an issue there https://gitlab.freedesktop.org/pipewire/pipewire/-/issues/2223. Even If changing the initialization order fixes the problem whatever PipeWire wants to do with the locale would be broken once we make GTK happy. As things are right now they conflict with each other.

wwmm commented 2 years ago

The next PipeWire release won't reset LC_NUMERIC anymore. I think now we wait for it to get to the Arch repositories. If we are lucky things will "just work".

Digitalone1 commented 2 years ago

Except for fmt strings that will need L flag for locale formatting.

Digitalone1 commented 2 years ago

Installed the new pipewire version, but I still don't see localized string in GTK widgets. I was expecting them to not be formatted by fmt, but what's wrong with the GTK widgets which are supposed to automatically follow the system locale?

Edit: closed by mistake, sorry.

There's something I don't get here.

wwmm commented 2 years ago

I will take a look at this now. At least with the new PipeWire I am seeing LC_NUMERIC=pt_BR.UTF-8; instead of C even after pw_init is called. So something else must be going on.

wwmm commented 2 years ago

I did a quick test where prepare_scales and prepare_spinbuttons were not called in the autogain window code. Without them the locale is properly set in the spinbutton. So as far as I can see the problem is just with the fmt usage.

wwmm commented 2 years ago

@Digitalone1 I was able to make it work but the fix is... Lets say... A little ugly

auto text = fmt::format(std::locale(setlocale(LC_ALL, nullptr)), "{0:.{1}Lf}{2}", value, precision,
                                    ((unit != nullptr) ? " "s + unit : ""));

It is already in the master branch so you can test. For some reason all fmt examples that use the L also pass the desired locale as argument. I tried without it and the L alone is not enough =/

wwmm commented 2 years ago

Example Screenshot from 2022-03-31 11-47-47

Digitalone1 commented 2 years ago

I don't remember what we used before, but isn't there a way to make the same thing with standard C++ or another format library which does not force us to call std:locale and setlocale?

wwmm commented 2 years ago

I don't remember what we used before

I think that a GTKMM function that under the hoods may be doing something similar to the ugly code above.

but isn't there a way to make the same thing with standard C++

Maybe. But one point in favor of fmt is that it is going to be standard c++20. It is in the c++20 specifications. We are only using the library right now because the compilers did not add the native support yet. But it is going to happen.

I like the flexibility in fmt syntax. But that way to handle locale is weird. Maybe because locale handling is a mess in general... What I am doing in the plugins window is saving the user locale in a variable. But I am not sure if we even have to be concerned about performance. As the locale is a global variable anyway it is possible this call is cheap.

Digitalone1 commented 2 years ago

Okay.

Are you sure all text has been formatted? I can't test now, but in the screenshot I don't see locale format in the footer bar.

wwmm commented 2 years ago

I don't see locale format in the footer bar.

That is right. When I printed the image above I had changed the fmt call only in the functions that prepare spinbutton and scales. Now I am changing the others.

wwmm commented 2 years ago

Fortunately we do not have to change all the fmt calls. Most of them force the precision to zero.

wwmm commented 2 years ago

I think I have changed all the calls that matter. When you have time try to see how it is in your computer.

Digitalone1 commented 2 years ago

Thousands separators on Frequencies in App Info UI/Pipewire Tab and Quantum in Pipewire Tab are the only remaining.

wwmm commented 2 years ago

Thousands separators on Frequencies in App Info UI/Pipewire Tab and Quantum in Pipewire Tab are the only remaining.

Ok. I will try fix this today.

wwmm commented 2 years ago

I have updated the master branch with these changes.

Digitalone1 commented 2 years ago

Okay. You did not add the separator for Quantum, but it's not a big issue. It's present for frequencies which is more meaningful.

If there's nothing else, you can close.

wwmm commented 2 years ago

I totally forgot about the quantum... But thinking about it now it does not seem really needed in its case. I think we can close this issue.