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

Inconsistent "is" in component::cover::device_automation::condition_type:: strings #128233

Open
NoRi2909 opened this issue Oct 12, 2024 · 17 comments · May be fixed by #131161
Open

Inconsistent "is" in component::cover::device_automation::condition_type:: strings #128233

NoRi2909 opened this issue Oct 12, 2024 · 17 comments · May be fixed by #131161

Comments

@NoRi2909
Copy link
Contributor

The problem

The two strings

 component::cover::device_automation::condition_type::is_position
 component::cover::device_automation::condition_type::is_tilt_position

are the only ones of all "Current {entity_name} …" strings that end with "is".

Should be fixed as it triggers inconsistent / misleading translations.
Especially with very long entity names where in German e.g. the "ist" (for "is") has to be added at end of the line:

 Aktuelle Kippstellung von {entity_name} ist

Use just:

 Current {entity_name} position
 Current {entity_name} tilt position

instead, like all other strings for Automation conditions.

What version of Home Assistant Core has the issue?

core-2024.10.2

What was the last working version of Home Assistant Core?

n/a

What type of installation are you running?

Home Assistant OS

Integration causing the issue

No response

Link to integration documentation on our website

No response

Diagnostics information

No response

Example YAML snippet

No response

Anything in the logs that might be useful for us?

No response

Additional information

No response

@joostlek
Copy link
Member

Don't they append the conditional value set at the end in the UI?

@NoRi2909
Copy link
Contributor Author

@joostlek Those show up in the Conditions section of automations like this:

Screenshot 2024-10-12 11 53 13

Comparing to other entities that don't have that "is" at the end:

Screenshot 2024-10-12 11 56 59

So there is no problem in removing those as the value is not appended directly to the string.
All the other comparision conditions work well without the verb at the end.

@joostlek
Copy link
Member

Can you adding a value to both?

@NoRi2909
Copy link
Contributor Author

@joostlek What to you mean with

Can you adding a value to both?

I can set the above and below values in either context.
Using the percentage sliders for the covers or by entering wattages for the power consumtion.

@NoRi2909
Copy link
Contributor Author

NoRi2909 commented Oct 12, 2024

@joostlek Now I think I understand what you where after:

When I define a trigger this creates a sentence that is shown in the header of that section.
But for conditions there is no sentence created:

Screenshot 2024-10-12 12 56 12

See both in comparison in the above screenshot. And it remains like that when I collapse the lower part.

If a sentenced was created for conditions then we would need those verbs in all of them.
Thus the two reported can be fixed as suggested.

@joostlek
Copy link
Member

If a sentenced was created for conditions then we would need those verbs in all of them.

I think this is worth a frontend bug, since I think we then would have is above x or is below x

Thus the two reported can be fixed as suggested.

You mean this issue?

@NoRi2909
Copy link
Contributor Author

@joostlek Well, creating those sentences with is above x or is below x would be a nice ER, indeed. :-)
Currently I don't see that with any Conditions I set in an Automation. So that's a much bigger task.

But as the first step we need to address this bug here that these two strings I reported are inconsistent and have that "is" at the end that would also collide with the ER you suggested above:

 "Current {entity_name} position is"
 "Current {entity_name} tilt position is"

All other

 "Current {entity_name} …"

strings don't end with "is". That's all that needs to be fixed here.

So the two strings to fix are in: homeassistant/components/cover/strings.json

Note: I have just learned that those paths I see in Lokalise and used in my bug reports like

component::cover::device_automation::condition_type::is_position
component::cover::device_automation::condition_type::is_tilt_position

don't seem to help in finding the strings quickly in Github. You need the full text for searching.

@joostlek
Copy link
Member

Hmm, I see what you mean, I will have a check with a frontender to see if this is indeed a bug and that the frontend must change, or that the backend must change

@NoRi2909
Copy link
Contributor Author

NoRi2909 commented Oct 13, 2024

@joostlek I have to correct myself regarding those summaries for Conditions:

I found that for entity based Conditions that sentence is already build in the header:

image

Therefore my bug report is still valid, the static "is" in the two reported strings has to be removed,
because the text is added dynamically by the following, containing "is" or "are" for multiple entities:

Confirm{hasAttribute, select, 
 true { {attribute} of}
 other {}
} {numberOfEntities, plural,
 zero {an entity is}
 one {{entities} is}
 other {{entities} are}
} {numberOfStates, plural,
 zero {a state}
 other {{states}}
}{hasDuration, select, 
 true { for {duration}} 
 other {}
 }

Now as you can see in the screen shot above that German is terrible because the translation is not correct at the moment. I have changed that to:

Bestätigt ist, dass{hasDuration, select, 
 true { seit {duration}} 
 other {}
}{hasAttribute, select, 
 true { {attribute} von}
 other {}
} {numberOfEntities, plural,
 zero {eine Entität }
 one {{entities} }
 other {{entities} }
} {numberOfStates, plural,
 zero {ein Zustand}
 other {{states}}
} {numberOfEntities, plural,
 zero {ist}
 one {ist}
 other {sind}
}

So for the example in the screenshot I would get:

 "Bestätigt ist, dass seit 1:00 Position von Badfenster … 50 ist"

But Lokalise only saves this with a warning because I had to branch twice for numberOfEntities

Here a screenshot of English and German side-by-side so the colored syntax is easier to check:

image

If that works I can fix several more. Already managed a few of the simpler ones that weren't correct, yet.

@joostlek
Copy link
Member

I am not sure if Lokalise provides that when we download the translations. I am not sure how we can allow mutiple branches and keep it working

@NoRi2909
Copy link
Contributor Author

@joostlek Sorry, I think I misused the word "branch" here …

I meant that within that code I had to use numberOfEntities twice as a condition:

{numberOfEntities, plural,
 zero {eine Entität }
 one {{entities} }
 other {{entities} }
} 

…

{numberOfEntities, plural,
 zero {ist}
 one {ist}
 other {sind}
}

Because in German the state comes between the noun and the verb.

There is just single translation from me, no two code branches. ;-)

I manged all other of those conditional strings in the meantime, so that one is the only one where Lokalise throws a warning. Perhaps I find a way to work around the issue by rewording some way. But it is very difficult because that sentence can really have a ton of variation.

@joostlek
Copy link
Member

Yea I understood correctly but couldn't find the right word. but the rest of my comment was still valid :)

@NoRi2909
Copy link
Contributor Author

NoRi2909 commented Oct 13, 2024

I ran my code through the parser at

https://format-message.github.io/icu-message-format-for-translators/editor.html

It does not complain about the format of my code, so it's probably just Lokalise that believes I accidentially duplicated that condition. I could make the whole thing even simpler, but then Lokalize complains even more.

But I cannot trigger the choices for zero, both for numberOfEntities and numberOfStates, even with the English original code, when testing on the above page.

@NoRi2909
Copy link
Contributor Author

Found the bug, it's in the English original, at least when parsing it with
https://format-message.github.io/icu-message-format-for-translators/editor.html

If you replace the two occurences of zero with =0 it does work as expected:

Confirm{hasAttribute, select, 
 true { {attribute} of}
 other {}
} {numberOfEntities, plural,
 =0 {an entity is}
 one {{entities} is}
 other {{entities} are}
} {numberOfStates, plural,
 =0 {a state}
 other {{states}}
}{hasDuration, select, 
 true { for {duration}} 
 other {}
 }

With all variables empty and just specifying 0 for numberOfEntities and numberOfStates it does return the expected:

"Confirm an entity is a state" :-)

So in case you also use https://github.com/format-message/format-message this needs a fix.

@joostlek
Copy link
Member

I have no clue what we use, but what i do know is that this is frontend I believe

@NoRi2909
Copy link
Contributor Author

NoRi2909 commented Oct 13, 2024

Yes, that's frontend now. Sorry for taking that long detour about that.
But we have now confirmed that the "is" in the two strings at

"is_position": "Current {entity_name} position is",

"is_tilt_position": "Current {entity_name} tilt position is"

can be removed.

I filed a frontend bug for that ICU message so someone can take a look and fix that one:
home-assistant/frontend#22356
It's really just that single one that contains zero, no other are affected. No wonder I kept stumbling on that one …

Thank you very much for all your help!

@NoRi2909
Copy link
Contributor Author

@joostlek Just FYI, after some discussion on Lokalise about the error it flagged for the ICU string we discussed above not being OK:

  1. It does actually work just fine in the current release - I tested every possible combination.
  2. That plural "are" is not even correct in English.

I filed a new bug on that so we could settle the problem in German: home-assistant/frontend#22566

@NoRi2909 NoRi2909 linked a pull request Nov 21, 2024 that will close this issue
19 tasks
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 a pull request may close this issue.

2 participants