home-assistant / core

:house_with_garden: Open source home automation that puts local control and privacy first.
https://www.home-assistant.io
Apache License 2.0
73.3k stars 30.61k forks source link

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

Open NoRi2909 opened 3 weeks ago

NoRi2909 commented 3 weeks ago

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 commented 3 weeks ago

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

NoRi2909 commented 3 weeks ago

@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 commented 3 weeks ago

Can you adding a value to both?

NoRi2909 commented 3 weeks ago

@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 commented 3 weeks ago

@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 commented 2 weeks ago

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 commented 2 weeks ago

@joostlek Well, creating those sentences with is above xor 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 commented 2 weeks ago

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 commented 2 weeks ago

@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 commented 2 weeks ago

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 commented 2 weeks ago

@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 commented 2 weeks ago

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

NoRi2909 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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

NoRi2909 commented 2 weeks ago

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

https://github.com/home-assistant/core/blob/c4ff3f731b1f9f703ea9f83141eb4700516f80a4/homeassistant/components/cover/strings.json#L18

https://github.com/home-assistant/core/blob/c4ff3f731b1f9f703ea9f83141eb4700516f80a4/homeassistant/components/cover/strings.json#L19

can be removed.

I filed a frontend bug for that ICU message so someone can take a look and fix that one: https://github.com/home-assistant/frontend/issues/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 commented 5 days ago

@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: https://github.com/home-assistant/frontend/issues/22566