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

Skip empty attributes to translate. #38

Merged
merged 2 commits into from
May 2, 2021

Conversation

jun66j5
Copy link
Member

@jun66j5 jun66j5 commented Feb 13, 2021

I encountered that Genshi generates placeholder with *.po header for empty placeholder like this:

<input type="text" placeholder="" />

to:

<input type="text" placeholder="Project-Id-Version: ..." />

Another example of empty alt attribute in https://trac.edgewall.org/ticket/10108#comment:2.

I think we should skip empty attributes to translate.

@FelixSchwarz
Copy link
Member

I think we should skip empty attributes to translate.

Sounds sensible to me.

@FelixSchwarz
Copy link
Member

I briefly looked at the code (though I don't pretend to fully understand the translation mechanics) and it looks to me as if your patch just prevents the translated value to be inserted in the final HTML, right? So the empty string will be extracted but the translation won't be inserted?

My idea was that empty strings might be skipped during extraction (not sure if that is feasible in Genshi currently).

  • Did I understand the patch correctly?
  • If so would you mind explaining your approach and why we should do it that way?

@jun66j5
Copy link
Member Author

jun66j5 commented Feb 16, 2021

I briefly looked at the code (though I don't pretend to fully understand the translation mechanics) and it looks to me as if your patch just prevents the translated value to be inserted in the final HTML, right?

Yes.

So the empty string will be extracted but the translation won't be inserted?

Yes.
In Genshi 0.7.5, the empty string is not extracted however it is translated.
After the patch, the empty string is not extracted nor translated.

My idea was that empty strings might be skipped during extraction (not sure if that is feasible in Genshi currently).

I temporarily added the following test, but it passes without my patch:

+    def test_extract_included_empty_attribute_text(self):
+        tmpl = MarkupTemplate(u"""<html>
+          <span title="">...</span>
+        </html>""")
+        translator = Translator()
+        messages = list(translator.extract(tmpl.stream))
+        self.assertEqual([], messages)
+

Investigating the source, Genshi (0.7.5) skips empty attributes at https://github.com/edgewall/genshi/blob/0.7.5/genshi/filters/i18n.py#L918-L921.

Also, I noticed spaces in string are stripped during the extraction. I think we should strip spaces in string during the translation, too.

@jun66j5
Copy link
Member Author

jun66j5 commented Feb 16, 2021

Root cause is that getext() function translates empty string to *.po header information. This behavior is by gettext's design. See https://stackoverflow.com/a/7659746.

For example, alt attribute of img element is often leave with empty string. Genshi with i18n feature translates such an empty attribute to with *.po header information. The behavior confuses a user and it assume the user doesn't want alt attribute with *.po header information (at least to me).

Workaround is preventing the translation by using expression in attribute, e.g. alt="${''}".

@@ -711,8 +711,9 @@ def __call__(self, stream, ctxt=None, translate_text=True,
for name, value in attrs:
newval = value
if isinstance(value, six.string_types):
if translate_attrs and name in include_attrs:
newval = gettext(value)
text = value.strip()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me awhile to understand why value.strip() is needed (because the value is stripped by Genshi during extraction) and why and text is needed (because gettext doesn't support being passed empty strings), so thank you very much to everyone who supplied explanations in the comments.

@hodgestar
Copy link
Contributor

@jun66j5 Thank you for the patch!

@hodgestar hodgestar merged commit ac3a59e into edgewall:master May 2, 2021
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.

3 participants