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

Thing upgrade gives different result than recreating thing #3569

Open
jlaur opened this issue Apr 22, 2023 · 19 comments
Open

Thing upgrade gives different result than recreating thing #3569

jlaur opened this issue Apr 22, 2023 · 19 comments
Labels
bug An unexpected problem or unintended behavior of the Core

Comments

@jlaur
Copy link
Contributor

jlaur commented Apr 22, 2023

This is a follow-up to openhab/openhab-docs#2040 (comment).

I have now tested the different update scenarios I could think of:

Channel id Channel type id Channel label Channel type label Channel label localized Channel type label localized Update instruction label After upgrade After recreate
test1 test1 Type 1 Type 1 Type 1
test2 test2 Channel 2 Type 2 Type 2 Channel 2
test3 test3 Channel 3 Type 3 Kanal 3 Type 3 Kanal 3
test4 test4 Channel 4 Type 4 Kanal 4 Art 4 Art 4 Kanal 4
test5 test5 Channel 5 Type 5 Kanal 5 Art 5 Update 5 Update 5 Kanal 5
test6 test6 Type 6 Update 6 Update 6 Type 6
test7 test7 Type 7 Art 7 Update 7 Update 7 Art 7

Retested after #3576 (build 3455), no changes.

Expected Behavior

Thing upgrade should give same result as recreating thing. At least I understand this as one of the motivations for #3330.

I would expect channel label to be considered and take precedence over channel type label.

I would additionally expect I18N localized strings to take precedence over English strings.

If it would work this way, update instruction labels would not be needed as they would be redundant.

All of this should apply to descriptions as well.

Current Behavior

  • Channel item labels are not taken into consideration.
  • Update instruction labels are taken into consideration. This can set a label that can not be set any other way (e.g. by creating a new thing) - why is this useful?
  • Update instruction labels are not localizable.

Currently it's impossible to get it right with I18N. Update instruction label can be copied from channel label, which makes it almost correct, but then restricted to English.

Steps to Reproduce

I added this to a binding for testing:

instructions.xml

		<instruction-set targetVersion="2">
			<add-channel id="test1" groupIds="main">
				<type>danfossairunit:test1</type>
			</add-channel>
			<add-channel id="test2" groupIds="main">
				<type>danfossairunit:test2</type>
			</add-channel>
			<add-channel id="test3" groupIds="main">
				<type>danfossairunit:test3</type>
			</add-channel>
			<add-channel id="test4" groupIds="main">
				<type>danfossairunit:test4</type>
			</add-channel>
			<add-channel id="test5" groupIds="main">
				<type>danfossairunit:test5</type>
				<label>Update 5</label>
			</add-channel>
			<add-channel id="test6" groupIds="main">
				<type>danfossairunit:test6</type>
				<label>Update 6</label>
			</add-channel>
			<add-channel id="test7" groupIds="main">
				<type>danfossairunit:test7</type>
				<label>Update 7</label>
			</add-channel>
		</instruction-set>

thing-types.xml

			<channel id="test1" typeId="test1"/>
			<channel id="test2" typeId="test2">
				<label>Channel 2</label>
			</channel>
			<channel id="test3" typeId="test3">
				<label>Channel 3</label>
			</channel>
			<channel id="test4" typeId="test4">
				<label>Channel 4</label>
			</channel>
			<channel id="test5" typeId="test5">
				<label>Channel 5</label>
			</channel>
			<channel id="test6" typeId="test6"/>
			<channel id="test7" typeId="test7"/>

	<channel-type id="test1">
		<item-type>String</item-type>
		<label>Type 1</label>
	</channel-type>
	<channel-type id="test2">
		<item-type>String</item-type>
		<label>Type 2</label>
	</channel-type>
	<channel-type id="test3">
		<item-type>String</item-type>
		<label>Type 3</label>
	</channel-type>
	<channel-type id="test4">
		<item-type>String</item-type>
		<label>Type 4</label>
	</channel-type>
	<channel-type id="test5">
		<item-type>String</item-type>
		<label>Type 5</label>
	</channel-type>
	<channel-type id="test6">
		<item-type>String</item-type>
		<label>Type 6</label>
	</channel-type>
	<channel-type id="test7">
		<item-type>String</item-type>
		<label>Type 7</label>
	</channel-type>

da.properties

channel-group-type.danfossairunit.main.channel.test3.label = Kanal 3
channel-group-type.danfossairunit.main.channel.test4.label = Kanal 4
channel-group-type.danfossairunit.main.channel.test5.label = Kanal 5

channel-type.danfossairunit.test4.label = Art 4
channel-type.danfossairunit.test5.label = Art 5
channel-type.danfossairunit.test7.label = Art 7
@jlaur jlaur added the bug An unexpected problem or unintended behavior of the Core label Apr 22, 2023
@J-N-K
Copy link
Member

J-N-K commented Apr 23, 2023

Most of that looks fine. The "channel label" is not considered during upgrade, that is why the label can be set in the update instruction, and it seems that works fine.

The only issue I see is that the English version is used instead of the localized version in test 5 when a label is provided. Is that also the case if you use Channel 5 in the update instruction?

@jlaur
Copy link
Contributor Author

jlaur commented Apr 23, 2023

Most of that looks fine. The "channel label" is not considered during upgrade, that is why the label can be set in the update instruction, and it seems that works fine.

Would it be possible to consider the channel label during upgrade? I think that would be the only way to fully ensure consistency (with textual configuration and thing recreation).

The only issue I see is that the English version is used instead of the localized version in test 5 when a label is provided. Is that also the case if you use Channel 5 in the update instruction?

I'll try to rerun that test to verify this.

@J-N-K
Copy link
Member

J-N-K commented Apr 23, 2023

I didn't find a way to do that when I originally added the code, that's why I added the label/description instructions. I still think that the non-localized versions are much better than requiring to re-create the whole thing.

Another thing you could try: @text/channel-group-type.danfossairunit.main.channel.test5.label.

@jlaur
Copy link
Contributor Author

jlaur commented Apr 23, 2023

@J-N-K - update on the two suggested variations of test case 5:

Channel id Channel type id Channel label Channel type label Channel channel localized Channel type label localized Update instruction label After upgrade After recreate
test5 test5 Channel 5 Type 5 Kanal 5 Art 5 Channel 5 Channel 5 Kanal 5
test5 test5 Channel 5 Type 5 Kanal 5 Art 5 @text/channel-group-type.danfossairunit.main.channel.test5.label @text/channel-group-type.danfossairunit.main.channel.test5.label Kanal 5

I still think that the non-localized versions are much better than requiring to re-create the whole thing.

I fully agree on that.

@J-N-K
Copy link
Member

J-N-K commented Apr 23, 2023

It would be interesting to know what happens if you create a new thing and then change the language. Do the labels/description change in that case?

@lolodomo
Copy link
Contributor

@J-N-K : does your PR #3576 changes something to this analysis and will require to update it when the PR is merged ?

@J-N-K
Copy link
Member

J-N-K commented Apr 30, 2023

It solves the missing localization of the channel-type labels/descriptions but not for individual channel labels/descriptions.

@lolodomo
Copy link
Contributor

lolodomo commented May 5, 2023

@jlaur : would it possible to have your table updated considering PR #3576 ?

@lolodomo
Copy link
Contributor

lolodomo commented May 5, 2023

  • Case 2 is in my opinion a bug, the channel label should always have higher priority than the channel type label.
  • Cases 3 and 4 are about localization and @J-N-K said he fixed something in PR Improve thing updates #3576. So to be tested again.
  • Cases 5, 6 and 7 look ok to me, the label in the instruction file has higher priority. In my opinion, it should just never be used ... except if the old channel had no label.

@jlaur
Copy link
Contributor Author

jlaur commented May 6, 2023

It would be interesting to know what happens if you create a new thing and then change the language. Do the labels/description change in that case?

It seems not. I'm using file-based configuration, so haven't experienced this problem, but when testing a managed thing in OH 4, I didn't see the language change reflected in the UI.

@jlaur
Copy link
Contributor Author

jlaur commented May 6, 2023

would it possible to have your table updated considering PR #3576 ?

Sure, when that PR is merged I can try to re-run my manual tests.

@lolodomo
Copy link
Contributor

lolodomo commented May 9, 2023

@jlaur : please let us know the new status now that the fix is merged.

@jlaur
Copy link
Contributor Author

jlaur commented May 9, 2023

@lolodomo - done, status is that nothing changed for these specific tests.

@J-N-K
Copy link
Member

J-N-K commented May 9, 2023

As I already said (and you confirmed): If the channel is created and localization changes, the channel label/description stays in the old language. It is quite difficult to get the correct label, but I'll have another look.

@jlaur
Copy link
Contributor Author

jlaur commented May 9, 2023

@J-N-K - I just ran the tests again on @lolodomo's request without any expectations, but changing localization is not part of running those tests. It was set to Danish from the beginning to see if any localized texts would be picked up, for example at least the channel type label.

@lolodomo
Copy link
Contributor

lolodomo commented May 9, 2023

Case 1 is fine.
Cases 2 and 3 are very annoying, it looks like the channel label is ignored and only the channel type label is considered. I guess it is because it is lost after channel removal.
Case 4 shows that localization is considered but only for channel type.
Cases 5, 6 and 7 are fine. If you set a label in the instructions, it is used.

@jlaur
Copy link
Contributor Author

jlaur commented May 9, 2023

Cases 2 and 3 are very annoying, it looks like the channel label is ignored and only the channel type label is considered. I guess it is because it is lost after channel removal.
Cases 5, 6 and 7 are fine. If you set a label in the instructions, it is used.

The only comment I have here is that if 2 and 3 would work, there would be no need for cases 5, 6 and 7 since they are a partial work-around for 2 and 3 not working (partial because localization is not supported). Otherwise I believe we all agree, but @J-N-K had a hard time trying to accomplish 2 and 3. It's still better than before, but so far nothing beats file-based things. 😉

@lolodomo
Copy link
Contributor

Case 2 is when a label is defined on the channel itself (to override the channel type label). @J-N-K : is it a data not available when adding/updating the channel ?

@openhab-bot
Copy link
Collaborator

This issue has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/come-back-and-learn-how-to-use-file-based-configuration/147067/15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of the Core
Projects
None yet
Development

No branches or pull requests

4 participants