-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
WebUI: Optimize i18next stuff #20643
WebUI: Optimize i18next stuff #20643
Conversation
Reverted .ts files from before commit 6964132 because they contain the strings from public part before the drop of QBT_TR Then run tstool.py to merge new strings from the JSON files.
df84d9b
to
a34b740
Compare
Guys, I think you're creating a pain in your ass with these "legacy" contexts. IMO, they need to be adjusted to meet current needs. |
Sure, but we still want to allow for some kind of comment (optionally). It could be used for disambiguation. |
A look from a different angle... |
@glassez the way we currently use it, it is the simplest one and is like a glorified dictionary data structure. |
We already have a tool that does this (extracts the source strings from html/js and synchronizes them with .ts files). All you need is a JS script that would replace QBT_TR statements with translated strings loaded from .ts files. |
Maybe. But for efficiency reasons, when trying to figure out what needs translating in an HTML document you probably need some kind of mark/tag like we do with the I am neutral on this. I can go both ways. I'll wait for @Chocobo1's view too. |
I don't think it is that simple. With your way, it will require additional code to fetch the html & js files and replace the I would prefer ditching |
@@ -99,13 +104,13 @@ function submitLoginForm(event) { | |||
if ((xhr.status === 200) && (xhr.responseText === "Ok.")) | |||
location.replace(location); | |||
else | |||
errorMsgElement.textContent = i18next.t('Invalid Username or Password.'); | |||
errorMsgElement.textContent = i18next.t('Invalid Username or Password.', { context: 'HttpServer' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should ditch the context
when it is not necessary, like this one.
If necessary, it can just map it to some generic context such as WebUI
in .ts file via the python scripts.
I think I need to signify the importance of disambiguation.
As explained in my reply above, we still need the
I would really suggest we ditch the |
I somehow don't like this example. It think it is important that the html can still be rendered correctly (with the default language Eng) in case i18next failed to work (or accidentally broken). That means we can't put everything only into Assuming we can build our own new string extractor, perhaps we can do: <p customAtrr1="value1" customAtrr2="value2" data-i18n="customAtrr1;customAtrr2;textContent">text123</p> The Excuse me If I misunderstood something, the design is quite open and broad and also needs to consider the ability of the tools, it isn't easy and straightforward. |
Okay, forget it. |
👍 |
@Chocobo1 |
Your example looks good but has the disadvantage of requiring to write our string extractor. |
The webui code is quite unorganized and many of it is just free functions. It would be unclear to say what is the context of it. A simple method is use the source filename as the context. But it require a new string extractor and existing tools won't be enough. Also I could imagine it might cause string duplication, for example the same string appeared in different context (or different source file, assuming we choose it that way). Is it really necessary to differentiate them?
Perhaps it isn't too hard to write a string extractor for .html: similarly to the
It doesn't sound bad, but I can only make the decision after seeing the result. |
Such duplication in different contexts is a lesser evil than the requirement to manually take care of disambiguation when adding each new string, IMO. |
I have pushed a new commit implementing this. <p data-i18n>Some text_context</p> becomes invalid. It should be done like this: <p data-i18n="Some text_context"></p> Reason: After the 1st pass the _context is lost. So subsequent lang changes can't find the correct translation due to missing context info.
I did a quick test. It isn't supported. I don't know if there are config options for it in i18next-parser. But, IMO, it is best that JS and HTML are separate. |
IMO, if we currently don't have any duplicate string in the WebUI it is a good indication that disambiguating where needed will be enough and not inconvenient. |
It is unfortunate that the more workaround we put in the more workaround is required. The result not only stray from our original visions it also (might) become brittle for maintenance in the long term. Not sure about others, but I wouldn't suggest going this way. A few questions beforehand:
|
The way I see it, no matter what method we invent, it is impossible to use FYI, At the moment, I don't think I have much to offer. If you decide to go the custom extractor/i18n library route, then feel free to close my PRs. They were more for discussions with concrete examples.
I wouldn't want to ship v5.0 with only the public portion transition, especially if we are going to go towards another route. If the new route isn't ready, I suggest reverting the public portion to QBT_TR() again.
I suspect that implementing this in JS/node will be vastly more productive and hassle-free. Doesn't nodejs effectively provide an up-to-date way to parse HTML and JS?
I am bit skeptical about Qt .ts on the client side during runtime. For me XML is a pain to work with when compared to JSON. But I am not insisting on this. An alternative would be to have a "build step" that would produce the final webui and it would convert the Qt ts to json files. eg using a bundler like Vite. A bundler would have the additional benefit of tree-shaking the JS files (something like LTO for JS). |
As I mentioned earlier, I strongly object to 'client-side translation' being a required feature of v5.0. The only requirement from me is that we must return the possibility of server-side translation of 3rd-party WebUIs until 'client-side translation' is implemented. |
You mean changing to another language on the fly? Maybe it isn't a concern. The language setting resides in Options dialog and closing the dialog will reload the whole WebUI which will in turn re-run the i18n procedure. The reload is already there for a long time.
Thanks. It indeed looks useful.
OK. In any case if the 'client-side translation' isn't fully ready for v5.0, we'll revert to
All my google result points to 3rd party libraries.
Reason I proposed ditching JSON is that if we want to have proper support for
The 'build step' springs into my mind from time to time. Although it would bring many possibilities but I always called it off. I'm concerned that it might affect packaging and development. Wouldn't it become difficult to build from source? Wouldn't it become harder to debug? In the end I decided to keep it as is, it isn't worth the effort IMO. BTW, I pondered on the |
👍
I agree. I would try to avoid any additional/intermediate steps if it is possible to do it in an acceptable way. |
OK, got it. |
OK. Please consider investigating if we could make it work with i18next in an acceptable way (to avoid reinventing the wheel and tool).
I hadn't investigated. You're correct. But still, IMO, nodejs is preferable to implement these scripts. The ecosystem seems to me more active than python (at least in the web dev front). Plus, it is far more easier to document and install depedencies and run scripts(a simple
Sure. It's probably my personal distaste to the format.
I am going offtopic. This is just for FYI and not a dependency for i18n. Vite has two modes. The
Probably the reason why their namespaces (equivalent for .ts context) seem to require explicit action. I am afraid I don't have any concrete proposal for the i18n stuff. I'll wait for your proposal. |
I had a rethink about this. The public part of webui should/must have client-side translation is because I don't want the user preference of locale to be leaked publicly. Everyone able to access your webui will know what language you are speaking/prefer. The private part of webui doesn't have this problem. Reason I rethink is that even if we have a perfect custom string extractor and i18n translation library, One will still have to migrate all |
Can't this problem be solved on top of the server-side translation? I believe it can. And if we are not going to switch entire WebUI to client side translation, I would strictly insist that we do exactly that. |
So it's not about necessarily using client-side translation, but about using client-side locale (or locale requested by client-side). IMO, it's generally a good idea to do this for the entire WebUI. |
I have some ideas but I'm not sure they are good. I'll look into it. |
IIRC doing some quick Internet searches, from the server side it could look like this:
|
My reply loosely addresses your comment:
In case you have missed it, I have done some work in #20610. Each commit in that PR documents the specific regex used. The regex'es can be reused to implement some other scheme if needed. |
@Chocobo1 |
I've done some client-side experiments but the result isn't good, I have more ideas but will still need time. Also stumbled upon this while researching: https://developer.mozilla.org/en-US/docs/Web/HTTP/Content_negotiation#the_accept-language_header |
This PR is stale because it has been 60 days with no activity. This PR will be automatically closed within 7 days if there is no further activity. |
This PR was closed because it has been stalled for some time with no activity. |
In the HTML files the i18next works this way:
This
becomes
This
becomes
This
becomes
Python tooling
translations_qt2i18next.py
It is meant to be run when strings are migrated from QBT_TR to i18next and AFTER the i18next-parser has created them in their respective JSON files. The
translations_qt2i18next.py
script will load the translated strings from the[locale].ts
into the[locale].json
.Aka it will migrate already existing translations to lessen the burden of the translators.
split_json.py
It is meant to be run after translations are pulled from Transifex. It will split the strings from the .ts files into their respective .json files
tstool.py
Old behavior: It scanned all the .js/.html/.css files for QBT_TR and updated the .ts files
New behavior: In addition to the old behavior it also merges the keys/context from
public/lang/en.json
andprivate/lang/en.json
into the .ts files. It accounts for the case where new strings are introduced via the i18next format.Context vs disambiguation
According to Transifex's page on Qt TS format, Transifex will present the context value as a comment to translators. So I opted to not remap the .ts context/name fields to comment fields.
The QBT_TR method required a context present. It is unclear to me what will happen if in the i18next way a context isn't given. Aka it is unclear to me what the interaction between the python tooling will be with a missing context.