[nikohomecontrol] Prepare for translation

[mherwege]

• Externalize strings to properties file to allow translation as a next step.
• Give listener thread a name.
• Fixed SAT warning

[lolodomo]

Please look at other bindings how they defined translations for thing type label/description or config parameter label/description. There is no need to add @text in you thing files. You have predefined properties automatically defined for that.
Take a look at the NTP binding as an example.

[lolodomo]

And please note that you did not use strictly the recommended thread name convention. The threads name should start by "OH-binding-" followied by the thing UID and optionally followed by an additional specific part in case you create several threads for the same thing.

[mherwege]

Please look at other bindings how they defined translations for thing type label/description or config parameter label/description. There is no need to add @text in you thing files. You have predefined properties automatically defined for that.
Take a look at the NTP binding as an example.

@lolodomo Thank you for looking into this. I am well aware I do not strictly need the @text in my thing files. But if I understood well from #10635, I would then need to replicate all strings anyway for them to be picked up by Crowdin. I consider that prone to error when one makes a change and forgets to keep them in sync. Therefore I opted for the @text construct.

[mherwege]

And please note that you did not use strictly the recommended thread name convention. The threads name should start by "OH-binding-" followied by the thing UID and optionally followed by an additional specific part in case you create several threads for the same thing.

@lolodomo, thank you for noticing, I didn’t look far enough in my research and will correct. Done

[lolodomo]

@lolodomo Thank you for looking into this. I am well aware I do not strictly need the @text in my thing files. But if I understood well from #10635, I would then need to replicate all strings anyway for them to be picked up by Crowdin. I consider that prone to error when one makes a change and forgets to keep them in sync. Therefore I opted for the @text construct.

@cweitkamp : can you confirm that is the new way to go for all bindings ?!

[lolodomo]

Review comments excluding all stuff with @text

I have now like a doubt that this binding implemented a proper bridge/thing handling.

[lolodomo]

https://www.openhab.org/docs/concepts/things.html#thing-status

The statuses UNINITIALIZED, INITIALIZING and REMOVING are set by the framework, whereas the statuses UNKNOWN, ONLINE and OFFLINE are assigned from a binding.

[cweitkamp]

@lolodomo Thank you for looking into this. I am well aware I do not strictly need the @text in my thing files. But if I understood well from #10635, I would then need to replicate all strings anyway for them to be picked up by Crowdin. I consider that prone to error when one makes a change and forgets to keep them in sync. Therefore I opted for the @text construct.

@cweitkamp : can you confirm that is the new way to go for all bindings ?!

According to our documentation this is a valid way to define translatable phrases. There are other bindings which are already designed in this way. There are no strict guidelines for the future way of localization and thus it is up to the developers to choose their favorite path. From my POV @mherwege arguments for his decision are strong and I personally agree on his opinion.

[lolodomo]

According to our documentation this is a valid way to define translatable phrases. There are other bindings which are already designed in this way. There are no strict guidelines for the future way of localization and thus it is up to the developers to choose their favorite path. From my POV @mherwege arguments for his decision are strong and I personally agree on his opinion.

I don't really like this choice being dictated by a tool (Crowdin) and the fact that we have 2 different ways of doing it, but if this is compliant with our coding rules, I will review (and approve) this part too. Thank you for the answer.

[mherwege]

@lolodomo Thank you for your review. I believe I have fixed all thing lifecycle issues. Can you review again?

[mherwege]

I believe the build failure is not related to the binding.

[lolodomo]

Please avoid redundant calls to updateStatus.
I see something wrong in your thing handlers initialize(): you are checking the bridge status at the very end, even in the separate thread you are scheduling. In fact, it should be the first thing you should check at the beginning of the method initialize.

[mherwege]

Sorry, I missed your review earlier. Thank you for the feedback.

Please avoid redundant calls to updateStatus.

I have removed these.

I see something wrong in your thing handlers initialize(): you are checking the bridge status at the very end, even in the separate thread you are scheduling. In fact, it should be the first thing you should check at the beginning of the method initialize.

I am a bit reluctant to touch this. It has been working well for many years for myself and other users. I made a few changes while doing the preparation to allow for translation, but this is becoming much bigger than anticipated. And that comes with added risk. I value your input, and it does make sense. But I don't want to risk destroying a working system.
If I understand well, initialize() in the thing handlers is called after initialize() for the bridge handler. The trigger is not for the bridge being online, just the initialize() method having finished. If I return from the thing handler initialize method at the start when the bridge is not online (and skip the rest of initialize), would initialize get called again when the bridge goes online? Or would I have to implement bridgeStatusChanged and put the initialization logic of the thing handler there?

[lolodomo]

I am a bit reluctant to touch this. It has been working well for many years for myself and other users. I made a few changes while doing the preparation to allow for translation, but this is becoming much bigger than anticipated. And that comes with added risk. I value your input, and it does make sense. But I don't want to risk destroying a working system.

I agree that it could be changed in a separate PR, especially if this is currently working well.
I will look again to understand if this is just a not ideal implementation or if there is a serious risk. But yes, my feeling when I commented was more the first option.

[lolodomo]

One last thing to be fixed

[mherwege]

@lolodomo Many thanks for you torough review effort. I believe I have covered all your remarks.

[lolodomo]

LGTM