hydrogen-music / hydrogen

The advanced drum machine for Linux, macOS, and Windows
http://www.hydrogen-music.org
GNU General Public License v2.0
1.01k stars 172 forks source link

Commit a45e12d4b breaks the Open and Save file browsers for some languages #1908

Closed gitterdude closed 7 months ago

gitterdude commented 7 months ago

Hydrogen version * : git commit a45e12d4be750cedde559 Operating system + version : Ubuntu 22.04 with XFCE and Compiz Audio driver + version : ALSA/pipewire


Some of the files in share/hydrogen/data/i18n break the display of the file browser that opens when you try to open or save a file. The window opens but doesn't get repainted, so it displays a slightly distorted version of the Hydrogen main window

Setting the LANGUAGE variable to uk, nl, sv, es, ca, cs or gl (at least) before running Hydrogen creates the problem, as shown below.

HY

theGreatWhiteShark commented 7 months ago

Hey @gitterdude ,

Thanks for reporting!

Strange. I can't reproduce this behavior on my machine (Devuan ASCII + i3). I'll check using Ubuntu 22.04 live image.

gitterdude commented 7 months ago

I get the same error in Ubuntu 20.04 w XFCE/Compiz and openSUSE 15.5 and w pure XFCE (no Compiz), so it could be XFCE's file manager Thunar that throws a fit over some of the translation files.

If you can't reproduce it with the Ubuntu live image then try the Xubuntu one

theGreatWhiteShark commented 7 months ago

I get the same error in Ubuntu 20.04 w XFCE/Compiz and openSUSE 15.5 and w pure XFCE (no Compiz), so it could be XFCE's file manager Thunar that throws a fit over some of the translation files.

The dialog is entirely rendered using Qt. Thunar is not involved.

But it might be related to how XFCE handles internationalization. What are your language settings on OS level? Could you run locale in a terminal?

gitterdude commented 7 months ago

Like I said initially

Setting the LANGUAGE variable to uk, nl, sv, es, ca, cs or gl (at least) before running Hydrogen creates the problem, (for openSUSE it's the LANG variable that controls it).

So LANGUAGE=it hydrogen or "fr" works but not "nl", "sv" or "ca".

Interestingly enough "es" , "cs" and "gl" work in openSUSE but not in Ubuntu 20.04 or 22.04. They all have the same 2.31 glibc version but they're obviously patched differently

And reverting the .ts files to a pre-a45e12d version and recompiling and replacing the newer ones with them makes all locales work again.

theGreatWhiteShark commented 7 months ago

Like I said initially

Ah, sorry. I mistook this for "setting the language in the Preferences Dialog". Most people opening issues here don't use CLI :)

So LANGUAGE=it hydrogen or "fr" works but not "nl", "sv" or "ca".

In order for the language you specified using the LANGUAGE variable to be picked up by Hydrogen properly, you had to either set one in the Preferences Dialog which is not available on you system or clear <preferredLanguage> in the $HOME/.hydrogen/hydrogen.conf file, right?

Interestingly enough "es" , "cs" and "gl" work in openSUSE but not in Ubuntu 20.04 or 22.04.

I just booted a live image of Ubuntu 22.04 and tested the 1.2.2 AppImage which includes the commit as well. (Thanks for dissecting the problem to the culprit commit by the way!) Translations are picked up properly and dialogs open nicely. I'll check a Xubuntu 22.04 image next.

And reverting the .ts files to a pre-a45e12d version and recompiling and replacing the newer ones with them makes all locales work again.

Yeah. It is only very recently that I integrated internationalization into our build pipeline in such a way translation files are update with each build. Previously, they were only updated in larger batches and there was almost no way to check something went wrong. But those translation which work - fr and it - were updated recently. So, it's very plausible that something broke on the commit you mentioned.

gitterdude commented 7 months ago

I didn't have <preferredLanguage> set so when I ran, say, LANGUAGE=fr hydrogen from the CLI I got all menus in French without having to set anything in the Preferences dialog. Setting the variable(s) is a fast way to test different languages of programs.

But do I get the same Open/Save issue if I don't specify LANGUAGE and instead set, say , "espanol" in Preferences and restart

--- added to original post ---

From what I understand the AppImage is a self contained system, so it shouldn't be using any external libs, right? Since we've seen that the issue is a bit different between openSUSE and Xubuntu it would suggest that there's something specific in the OS's libs that are triggering the bug. So you probably won't see it using the AppImage.

I'll see if I can test my compiled version in a live image.

gitterdude commented 7 months ago

Yeah, I get the same non-repainted file browser for the selected languages when I run my compiled version in the Xubuntu 22.04.3 live image.

One striking difference is that our different versions are using different file browsers. Your AppImage one looks like this

HyApp

and my looks like

HyComp

So that could very well be the reason your always works and mine doesn't.

Ubuntu 22.04 and openSUSE 15.5 use Qt 5.15 and your AppImage seems to be doing it as well, so there must be some other difference. I compared what our different libhydrogen-core-1.2.2.so were linked against and didn't find any smoking gun. Yours have libxslt and my has libX11-xcb but I don't think that should make a difference.

theGreatWhiteShark commented 7 months ago

and my looks like

What the? There are different themes to pick for the base Qt dialogs and the AppImage might just carry a subset of those available when you compile the code yourself. Also, we could - in theory - ask the host system to open the file in a web browser. But we don't do that. We use QFileDialog and I don't see how Hydrogen could open up an external file browser instead.

Or is it just a theme and most dialogs look the same? Which one did you set in the Preferences? And could you run a program like pstree to check if Hydrogen did invoke another program while showing the dialog?

What make me wounder most is that e.g. for the "Save As" dialog there is just the window title which gets translated. And this one does not even has changed in most .ts files in the commit you found using dissection. https://github.com/hydrogen-music/hydrogen/commit/a45e12d4be750cedde5598dca108c54b89f5d097#diff-62d3e27acb19b5b51a79ad71ea7fa35512651b96d646e7969bc951d66263dd42L1269

my has libX11-xcb

Hmm. This smells a lot like being related to opening dialogs.

From what I understand the AppImage is a self contained system, so it shouldn't be using any external libs, right?

Yes. It does not ask the linker of your OS to find shared library libXXX.so and, instead, uses one shipped within the AppImage. But that's it. All configuration files and resources, programs called, services etc. are used from your host system. It is just shipping a subset of its required shared libraries but is not sandboxed in any way.

So, whether the bug is caused by the shared lib from your package repos or by the overall configuration of the OS, the bug may not or may also occur in the AppImage.

gitterdude commented 7 months ago

Ah, you get the default Qt dialog and I the native one because "The implementation of FileDialog will be a platform file dialog if possible. If that isn't possible, then it will try to instantiate a QFileDialog."

I get the exact same dialog when I go to File->Open in any program, like Mousepad or Ristretto, so that's my platform one, which is GTK3 based.

Your AppImage doesn't have access to my file browser so it gets the Qt built-in one.

Whether that's related to the repainting bug is unclear but it could very well be that Qt's dialog treats umlauts and stuff different to what the GTK3 dialog does.

gitterdude commented 7 months ago

I found the problematic entry in the .ts files.

For some reason, probably due to buffer allocation, the Mute translation below the <source>M</source> entry needs to be shortened for the platform file dialog to open properly.

How much it has to be shortened appears to be language dependent but I can't figure out why just by looking at the .ts-files.

Another workaround is to close the Mixer and then either leaving it closed or reopen it again. That'll also make the file dialog work. The .ts entry is apparently for the Mixer so it seems that it doesn't get enough memory allocated on start-up but does on a subsequent close/reopen.

    <message>
        <source>M</source>
        <extracomment>Text displayed on the button for muting an instrument strip in the mixer. Its size is designed for a single character.</extracomment>
        <translation type="unfinished"></translation>
    </message>
    <message>
        <source>Mute</source>
        <extracomment>Text displayed on the button for muting the master strip. Its size is designed for a four characters.</extracomment>
        <translation type="unfinished">Silenciar</translation>  <========  THIS ONE
    </message>
theGreatWhiteShark commented 7 months ago

Ah, you get the default Qt dialog and I the native one because "The implementation of FileDialog will be a platform file dialog if possible. If that isn't possible, then it will try to instantiate a QFileDialog."

Haha. Seems I never used a system where this "was possible"

I found the problematic entry in the .ts files.

Nice catch! Thanks a lot for al your work!

I'll try to reproduce it again using all ingredients (building it natively on my system, transferring it to a XUbuntu live image, ensuring the mixer is shown).

Feels like we run into problems when the text displayed is wider than the button widget holding it. Translating "M" with "MUTEMUTEMUTE" will probably break it too.

Sorry for the late reply by the way. I had a cold.

gitterdude commented 7 months ago

Yeah, it is width dependent. Translating "Mute" to six lowercase "s" works but six uppercase "S" fails, since the capital letters take more horizontal space, for the specific font anyway, than the lower case ones.

No, problem; glad you're feeling better again.

theGreatWhiteShark commented 7 months ago

Ah. Okay. I'm starting to understand what is going on. The problem is not translated text itself. https://github.com/hydrogen-music/hydrogen/commit/a45e12d4be750cedde5598dca108c54b89f5d097 was the first new commit for the translation files after porting the button widget from a custom pixmap to Qt's native QPushButton class. Its parent commit and a lot more probably won't work either.

So, we have rendering issues whenever the internationalized text is overflowing the new version of the Button class. Nice. That feels like we are already half way there.

Translating "Mute" to six lowercase "s" works but six uppercase "S" fails, since the capital letters take more horizontal space, for the specific font anyway, than the lower case ones.

And that is just for the default font. We using a larger font + setting the font size to "Large", we should be even more likely to have such an overflow in some button.

theGreatWhiteShark commented 7 months ago

Hey @gitterdude ,

Could you check whether #1914 fixes the issue at your end?

gitterdude commented 7 months ago

Yes it works! I notice that for some translations the font size gets almost illegibly small, but that's just an incentive for the translators to come up with something using fewer letters.

theGreatWhiteShark commented 7 months ago

Yes it works!

Awesome!

I notice that for some translations the font size gets almost illegibly small, but that's just an incentive for the translators to come up with something using fewer letters.

Yes. This is the intend. I tried to ensure to show the entire string and it is up to the translator to put one of proper size