Skip to content
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

Improve taskjob translation #613

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

eduardomozart
Copy link
Contributor

@eduardomozart eduardomozart commented Jan 27, 2025

This is a revision of PR#601. When translating on client-side, the translation may fail for some items (e.g. "updatetheitem" strings) because it creates a string on-the-fly (e.g. "Update the item Computer", "Update the item Network Device"). Also, when translating on client-side, it creates some artifacts when calling the $Class->getLink() call because the link are escaped and isn't rendered as HTML on client-side.
This also improves the translation of ==updatetheitem== because the Class type was still being saved translated on DB, so this PR removes saving the translated Class name on DB and uses the Class instantiation to retrieve asset name ("Computer", "Network Device" etc) on-the-fly from Class itself.
This PR also includes #609. There's other small fixes included into this PR, too, as using plural number based from current user session/language and not showing 'ok' twice as code/message status on ESX tasks.

Update (01/28/2024): Now this PR supports placeholders. Into some languages the number has a different positioning than English, but the current translation system doesn't allow moving numbers/placeholders. Now it's possible to translate those strings as expected into other languages without it looking akward, as the only way by now would be using "10 ==threads== 15 ==timeout==" placeholders which would give "10 Número de threads 15 segundos de timeout" into Portuguese (the order of the words into sentence is wrong).

Into the new format, that same string is saved as [[10]] threads [[15]] timeout into DB. The RegEx has been edited into convertComment() as it has been changed to accept [[itemtype]] or [[itemtype:item_id]]. If [[itemtype]] isn't a Class it's then considered as a placeholder, e.g. the string "[[10]] threads [[15]] timeout" may be translated as "Número de threads 10, 15 segundos de timeout" into Portuguese.

Checklist before requesting a review

Please delete options that are not relevant.

  • I have performed a self-review of my code.
  • I have added tests (when available) that prove my fix is effective or that my feature works.
  • This change requires a documentation update.

Description

Screenshots (if appropriate):

This is a revision of PR#601. When translating on client-side, the translation may fail for some items (e.g. "updatetheitem" strings) because it creates a string on-the-fly (e.g. "Update the item Computer", "Update the item Network Device"). Also, when translating on client-side, it creates some artifacts when calling the ``$Class->getLink()`` call because the link are escaped and isn't rendered as HTML on client-side.
@eduardomozart eduardomozart changed the title Improve translation Improve taskjob translation Jan 27, 2025
inc/taskjoblog.class.php Outdated Show resolved Hide resolved
@stonebuzz
Copy link
Collaborator

Thanks for your PR and for the hard work you've put into it. I've had a good look at the changes, and I have to admit that I'm a bit wary of incorporating such a major modification, especially in such a sensitive part of the code.

My main concern is the maintainability and stability of this critical section. With a large volume of modifications, it becomes more difficult to anticipate the impact and ensure that no side-effects emerge.

Honestly, I don't have the time/resources to test all the use cases impacted by these changes.

The risk of bugs/improvements is far too great to consider merging it for now.

I know you spend quite a bit of time on the plugin, and it's great to see that despite its complexity, you’re sticking with it. But honestly, this plugin is destined to disappear in favor of a completely rewritten version, and we have already identified these kinds of "gaps" or "oddities" that will need to be drastically improved.

@eduardomozart
Copy link
Contributor Author

eduardomozart commented Jan 29, 2025

I see, no problem. This commit only does the following to convertComment function:

  1. Replace all instances of "[[]]" to place holders (first foreach), which allows them to be gathered by gettext, e.g. "[[2]] devices found" becomes "%1$s devices found". Every instance of "[[]]" will receive a number (%1$s, %2$s and go on).
  2. Now it's static! It means we can translate them using gettext. It also means the user can move the numbering through the PO file if the user language requires it, e.g. "Dispositivos encontrados %1$s" in Portuguese).
  3. String is translated, let's restore the placeholder value with it's respective value (second foreach). Into this example, it would return "Dispositivos encontrados [[2]]".

The rest of the logic remains the same, only the second argument of [[]] becomes optional, so it supports [[ItemType::ItemID]] and [[<String>]] syntax. If it is [[ItemType::ItemID]], it will translate as "Network Device (Name with Hyperlink to Asset)" as it is today (no changes), but if it uses the [[<String>]], it will simply remove the brackets from , so "Dispositivos encontrados [[2]]" would become "Dispositivos encontrados 2" into the final translation.

If this change is too cubersome, please let me know that I remove those changes and keep the code as it is, only mantaining the added static string which hasn't been added to the translation before.

@stonebuzz
Copy link
Collaborator

At the risk of repeating myself, the likelihood of bugs or unforeseen improvements is too high to consider a merge at this stage.

That being said, this enhancement, while highly relevant in substance, is an improvement I would like to see integrated into the next version of the plugin.

In the meantime, feel free to keep this enhancement as a branch or patch in your repository.

For now, I prefer to avoid any major changes to this plugin.

@eduardomozart
Copy link
Contributor Author

eduardomozart commented Jan 29, 2025

No problem @stonebuzz. I don't think to create a private fork/branch for myself, I think that if my commits can't benefit the community, then it's better not to implement it.
I updated the commits and now this is a simplified version of the original function, only with additional translated strings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants