Separate ChannelGroupType infrastructure from ChannelType #6072

htreu · 2018-08-17T14:05:16Z

This splits a ChannelTypeGroupProvider, ChannelTypeGroupRegistry and i18n classes from the ChannelType infrastructure.

Fixes cyclic dependency reported in #6054.

Signed-off-by: Henning Treu <henning.treu@telekom.de>

kaikreuzer · 2018-08-17T16:41:53Z

....thing/src/main/java/org/eclipse/smarthome/core/thing/internal/i18n/ChannelI18nUtilImpl.java

+ */
+@NonNullByDefault
+@Component(immediate = true, service = ChannelI18nUtil.class)
+public class ChannelI18nUtilImpl implements ChannelI18nUtil {


This one strucks me weird. We have so far XXXI18nUtil classes, which are no DS components and are simply instantiated by passing a TranslationProvider.

Doing an interface here with an impl class, which is then registered as a component seems to fall out of this pattern.
Is this rather supposed to be a ChannelI18nLocalizationService (or even a ChannelDefinitionI18nLocalizationService) instead?

Yes, the first idea was to just expose the implementation class as a service. I had an offline review and this was one of the comments. will change back.

I'm afraid, you didn't read all of my comment:

We have so far XXXI18nUtil classes, which are no DS components and are simply instantiated by passing a TranslationProvider.

You have now refactored the class by removing the interface, but it still is a DS component.

kaikreuzer · 2018-08-17T17:11:48Z

@cweitkamp You are very much invited to review this PR as well as you have implemented major parts of the translation infrastructure!

Signed-off-by: Henning Treu <henning.treu@telekom.de>

cweitkamp

I like this change. It is not only a solution for #6054, it is an improvement for the framework. Thanks for your work.

While we are introducing dedicated util classes for localization of the different types I am wondering why we do not add a util class for the ChannelGroupType in the same way. I have left a comment inside the code too. After that we can consider to move all methods from ThingTypeI18nUtil into the related util classes. Wdyt?

The rest looks pretty good. Only some minor remarks.

cweitkamp · 2018-08-17T17:42:01Z

...ipse.smarthome.core.thing/src/main/java/org/eclipse/smarthome/core/thing/type/ThingType.java

@@ -274,16 +274,16 @@ public String getBindingId() {
 @SuppressWarnings("rawtypes")
 ServiceReference ref = bundleContext.getServiceReference(ChannelTypeRegistry.class.getName());


We have to get a ServiceReference for the ChannelGroupTypeRegistry here.

actually the whole method is unused. Removed.

cweitkamp · 2018-08-17T17:54:18Z

...ng/src/main/java/org/eclipse/smarthome/core/thing/i18n/ThingTypeI18nLocalizationService.java

- }
- return builder.build();
+ protected void unsetChannelI18nUtil(ChannelI18nUtil channelI18nUtil) {
+ this.channelI18nUtil = null;
 }

 private List<ChannelGroupDefinition> createLocalizedChannelGroupDefinitions(final Bundle bundle,


We are introducing translation services / util classes for every single entity now thus this method could be factored out to its own service / util class too.

I only did the ChannelI18nUtil because the createLocalizedChannelDefinitions method is used in both ThingTypeI18nLoc... and ChannelGroupTypeI18nLoc....
I would like to keep further refactorings out of this PR and consolidate, move, clean up in a separate PR.

cweitkamp · 2018-08-17T18:01:42Z

....xml/src/main/java/org/eclipse/smarthome/core/thing/xml/internal/XmlChannelTypeProvider.java

@@ -64,21 +52,21 @@ public ChannelType getChannelType(ChannelTypeUID channelTypeUID, Locale locale)

 @Reference
 public void setThingTypeI18nLocalizationService(
- final ThingTypeI18nLocalizationService thingTypeI18nLocalizationService) {
- this.thingTypeI18nLocalizationService = thingTypeI18nLocalizationService;
+ final ChannelTypeI18nLocalizationService thingTypeI18nLocalizationService) {


Please rename the method argument too.

cweitkamp · 2018-08-17T18:01:53Z

....xml/src/main/java/org/eclipse/smarthome/core/thing/xml/internal/XmlChannelTypeProvider.java

 }

 public void unsetThingTypeI18nLocalizationService(
- final ThingTypeI18nLocalizationService thingTypeI18nLocalizationService) {
- this.thingTypeI18nLocalizationService = null;
+ final ChannelTypeI18nLocalizationService thingTypeI18nLocalizationService) {


Please rename the method argument too.

cweitkamp · 2018-08-17T19:02:17Z

...src/main/java/org/eclipse/smarthome/core/thing/xml/internal/XmlChannelGroupTypeProvider.java

 import java.util.Locale;

 import org.eclipse.smarthome.config.xml.AbstractXmlBasedProvider;
 import org.eclipse.smarthome.core.thing.UID;
+import org.eclipse.smarthome.core.thing.i18n.ChannelGroupTypeI18nLocalizationService;
 import org.eclipse.smarthome.core.thing.i18n.ThingTypeI18nLocalizationService;


This import is not needed anymore.

cweitkamp · 2018-08-17T19:02:52Z

....xml/src/main/java/org/eclipse/smarthome/core/thing/xml/internal/XmlChannelTypeProvider.java

 import java.util.Locale;

 import org.eclipse.smarthome.config.xml.AbstractXmlBasedProvider;
 import org.eclipse.smarthome.core.thing.UID;
+import org.eclipse.smarthome.core.thing.i18n.ChannelTypeI18nLocalizationService;
 import org.eclipse.smarthome.core.thing.i18n.ThingTypeI18nLocalizationService;


This import is not needed anymore.

cweitkamp · 2018-08-17T19:03:37Z

...e.thing/src/main/java/org/eclipse/smarthome/core/thing/DefaultSystemChannelTypeProvider.java

@@ -25,9 +25,8 @@
 import java.util.stream.Stream;

 import org.eclipse.jdt.annotation.Nullable;
+import org.eclipse.smarthome.core.thing.i18n.ChannelTypeI18nLocalizationService;
 import org.eclipse.smarthome.core.thing.i18n.ThingTypeI18nLocalizationService;


This import is not needed anymore.

Did you see this comment?

thanks, was overlooked first.

cweitkamp · 2018-08-17T19:15:28Z

...marthome.core.thing/src/main/java/org/eclipse/smarthome/core/thing/i18n/ChannelI18nUtil.java

+ */
+@NonNullByDefault
+@Component(immediate = true, service = ChannelI18nUtil.class)
+public class ChannelI18nUtil {


Maybe we should rename this class to ChannelTypeI18nUtil?

Its currently localising a list of ChannelDefinitions which is not an ideal method signature imho. As said in the other comment I would like to (see a) clean up in another PR.

cweitkamp · 2018-08-17T19:30:04Z

...core.thing/src/main/java/org/eclipse/smarthome/core/thing/type/ChannelGroupTypeRegistry.java

+ }
+
+ @Reference(cardinality = ReferenceCardinality.MULTIPLE, policy = ReferencePolicy.DYNAMIC)
+ protected void addChannelTypeProvider(ChannelGroupTypeProvider channelTypeProviders) {


Please rename the argument to channelTypeProviders.

cweitkamp · 2018-08-17T19:30:20Z

...core.thing/src/main/java/org/eclipse/smarthome/core/thing/type/ChannelGroupTypeRegistry.java

+ this.channelGroupTypeProviders.add(channelTypeProviders);
+ }
+
+ protected void removeChannelTypeProvider(ChannelGroupTypeProvider channelTypeProviders) {


…eUID Signed-off-by: Henning Treu <henning.treu@telekom.de>

htreu · 2018-08-18T13:33:15Z

I did the cosmetic changes, thanks @cweitkamp for the review. As said in the comments - in case you are okay with it - I would like the clean up in a separate PR.

maggu2810 · 2018-08-19T09:52:40Z

Can't we remove the immediate activation?

cweitkamp · 2018-08-19T15:21:16Z

@htreu Yes of course, I am okay with it. It was just a thought coming to my mind while reading through the code and I did not want to loose it.

Signed-off-by: Henning Treu <henning.treu@telekom.de>

htreu · 2018-08-20T05:34:30Z

@maggu2810 I did switch all ChannelType related services to delayed activation. The default setup runs on my machine, but to be honest I can not overlook the exact impact of this change.
@ALL: Please test & review.

…ete import Signed-off-by: Henning Treu <henning.treu@telekom.de>

maggu2810 · 2018-08-20T07:10:20Z

Didn't there exist similar cicular references as before?

XmlChannelGroupTypeProvider 1..1 ChannelGroupTypeI18nLocalizationService
ChannelGroupTypeI18nLocalizationService 1..1 ChannelI18nUtil
ChannelI18nUtil 1..1 ChannelTypeRegistry
ChannelTypeRegistry 0..n ChannelTypeProvider

kaikreuzer · 2018-08-20T07:18:04Z

I am still very much against a DS service ChannelI18nUtil - see #6072 (comment). This does not seem to be right.

kaikreuzer · 2018-08-20T07:19:41Z

There exists a typo: Channell

@maggu2810 Where do you see "Channell"?

maggu2810 · 2018-08-20T07:24:54Z

@maggu2810 Where do you see "Channell"?

It need to take two more coffees 😉 (the lower "L" and the upper "i" confused me).

kaikreuzer · 2018-08-20T07:26:43Z

Ok, better have some more cooofffffeee 🤣

Signed-off-by: Henning Treu <henning.treu@telekom.de>

htreu · 2018-08-20T10:52:41Z

@maggu2810 the cycle is broken apart because XmlChannelGroupTypeProvider is not a ChannelTypeProvider anymore. Its now a ChannelGroupTypeProvider.

cweitkamp

All my comments have been addressed. Thanks.

kaikreuzer · 2018-08-20T13:18:01Z

...marthome.core.thing/src/main/java/org/eclipse/smarthome/core/thing/i18n/ChannelI18nUtil.java

+ *
+ */
+@NonNullByDefault
+class ChannelI18nUtil {


Instead of defining it package private, wouldn't the proper way be to move it to an internal package?

kaikreuzer · 2018-08-20T13:19:04Z

...ng/src/main/java/org/eclipse/smarthome/core/thing/i18n/ThingTypeI18nLocalizationService.java

- return builder.build();
+ @Activate
+ protected void activate() {
+ channelI18nUtil = new ChannelI18nUtil(channelTypeI18nLocalizationService, channelTypeRegistry);


You should also set it to null in a deactivate method.

kaikreuzer · 2018-08-20T13:19:13Z

...main/java/org/eclipse/smarthome/core/thing/i18n/ChannelGroupTypeI18nLocalizationService.java

+
+ @Activate
+ protected void activate() {
+ channelI18nUtil = new ChannelI18nUtil(channelTypeI18nLocalizationService, channelTypeRegistry);


You should also set it to null in a deactivate method.

kaikreuzer · 2018-08-20T13:26:15Z

...core.thing/src/main/java/org/eclipse/smarthome/core/thing/type/ChannelGroupTypeRegistry.java

+ }
+
+ @Reference(cardinality = ReferenceCardinality.MULTIPLE, policy = ReferencePolicy.DYNAMIC)
+ protected void addChannelTypeProvider(ChannelGroupTypeProvider channelGroupTypeProvider) {


method should be called addChannelGroupTypeProvider

kaikreuzer · 2018-08-20T13:26:34Z

...core.thing/src/main/java/org/eclipse/smarthome/core/thing/type/ChannelGroupTypeRegistry.java

+ this.channelGroupTypeProviders.add(channelGroupTypeProvider);
+ }
+
+ protected void removeChannelTypeProvider(ChannelGroupTypeProvider channelGroupTypeProvider) {


method should be called removeChannelGroupTypeProvider

kaikreuzer · 2018-08-20T13:27:56Z

...core.thing/src/main/java/org/eclipse/smarthome/core/thing/type/ChannelGroupTypeRegistry.java

+ * The {@link ChannelGroupTypeRegistry} tracks all {@link ChannelGroupType}s provided by
+ * registered {@link ChannelGroupTypeProvider}s.
+ *
+ * @author Dennis Nobel - Initial contribution


As this is a completely new class, better put your own name here.

kaikreuzer · 2018-08-20T13:28:08Z

...core.thing/src/main/java/org/eclipse/smarthome/core/thing/type/ChannelGroupTypeProvider.java

+ *
+ * @see ChannelGroupTypeRegistry
+ *
+ * @author Dennis Nobel - Initial contribution


As this is a completely new class, better put your own name here.

Signed-off-by: Henning Treu <henning.treu@telekom.de>

kaikreuzer

Thanks for the updates, lgtm! Let's wait for Travis.

kaikreuzer · 2018-08-21T06:18:05Z

Marked as "API Breaking" as I just found this in the openHAB build:

[ERROR] /scratch/jenkins/workspace/sandbox-openhab2-release/openhab2-addons/addons/binding/org.openhab.binding.nest.test/src/test/java/org/openhab/binding/nest/handler/NestThingHandlerOSGiTest.java:[195] 
[ERROR] 	.getChannelGroupType(channelGroupDefinition.getTypeUID());
[ERROR] 	 ^^^^^^^^^^^^^^^^^^^
[ERROR] The method getChannelGroupType(ChannelGroupTypeUID) is undefined for the type ChannelTypeRegistry
[ERROR] 1 problem (1 error)

Should not be too severe as hopefully not many people use the ChannelTypeRegistry directly.

maggu2810 · 2018-08-21T06:45:20Z

If it is already API breaking, why not removing the deprecated default methods from the ChannelTypeProvider?

    @Deprecated
    @Nullable
    default ChannelGroupType getChannelGroupType(ChannelGroupTypeUID channelGroupTypeUID, @Nullable Locale locale) {
        return null;
    }

    /**
     * @deprecated The {@link ChannelGroupTypeProvider} is now to be implemented/used instead.
     */
    @Deprecated
    @Nullable
    default Collection<ChannelGroupType> getChannelGroupTypes(@Nullable Locale locale) {
r

kaikreuzer · 2018-08-21T06:47:48Z

Because ChannelTypeProvider is something that many bindings implement (and they are allowed to) - it is fair to give them a grace period to adapt to the new interface.

The registry is instead normally considered an internal service and not to be used by extensions (my finding above was merely a test that made use of it).

…6109) With #6072 the ChannelGroupType infrastructure was separated from ChannelType. This contribution applies corresponding adaptions to the homematic binding. Signed-off-by: Michael Reitler <michael.reitler@telekom.de>

Henning Treu added 2 commits August 17, 2018 13:59

Separate infrastructure for ChannelGroupType

9606462

Signed-off-by: Henning Treu <henning.treu@telekom.de>

Additional fixes & test clean up

15c202e

Signed-off-by: Henning Treu <henning.treu@telekom.de>

kaikreuzer suggested changes Aug 17, 2018

View reviewed changes

Refactor ChannelI18nUtil to expose service directly

5453cec

Signed-off-by: Henning Treu <henning.treu@telekom.de>

cweitkamp suggested changes Aug 17, 2018

View reviewed changes

Fix cosmetic review comments, remove obsolete ThingType#getChannelTyp…

b9c086c

…eUID Signed-off-by: Henning Treu <henning.treu@telekom.de>

Switch to delayed activation for all Channel*Type* services

632ae29

Signed-off-by: Henning Treu <henning.treu@telekom.de>

delayed activation for DefaultSystemChannelTypeProvider, remove obsol…

60181df

…ete import Signed-off-by: Henning Treu <henning.treu@telekom.de>

Refactor ChannelI18nUtil to simple utility class

5ce32f3

Signed-off-by: Henning Treu <henning.treu@telekom.de>

cweitkamp approved these changes Aug 20, 2018

View reviewed changes

kaikreuzer reviewed Aug 20, 2018

View reviewed changes

kaikreuzer suggested changes Aug 20, 2018

View reviewed changes

Henning Treu added 3 commits August 20, 2018 15:30

Move utility to internal package, change author

5bc1105

Signed-off-by: Henning Treu <henning.treu@telekom.de>

clean up references on deactivation

7dea8d1

Signed-off-by: Henning Treu <henning.treu@telekom.de>

rename method to match parameter name, change author

25fd2bf

Signed-off-by: Henning Treu <henning.treu@telekom.de>

kaikreuzer approved these changes Aug 20, 2018

View reviewed changes

kaikreuzer merged commit 71f772f into eclipse-archived:master Aug 20, 2018

kaikreuzer added the APIBreaking label Aug 21, 2018

kaikreuzer mentioned this pull request Aug 21, 2018

adapted tests to changed ESH API openhab/openhab-addons#3853

Merged

This was referenced Aug 24, 2018

[homematic] Split ChannelTypeProvider and ChannelGroupTypeProvider #6109

Merged

[homematic] Fix issue #6053: HomematicIP-Devices offline with 2.4.0 Snapshot 1330 #6101

Merged

htreu added this to the 0.10.0 milestone Oct 30, 2018

cweitkamp mentioned this pull request May 6, 2019

Separate i18n infrastructure for 'ChannelGroup' openhab/openhab-core#804

Merged

		@@ -274,16 +274,16 @@ public String getBindingId() {
		@SuppressWarnings("rawtypes")
		ServiceReference ref = bundleContext.getServiceReference(ChannelTypeRegistry.class.getName());

Separate ChannelGroupType infrastructure from ChannelType #6072

Separate ChannelGroupType infrastructure from ChannelType #6072

Conversation

htreu commented Aug 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaikreuzer commented Aug 17, 2018

cweitkamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htreu commented Aug 18, 2018

maggu2810 commented Aug 19, 2018

cweitkamp commented Aug 19, 2018

htreu commented Aug 20, 2018 • edited Loading

maggu2810 commented Aug 20, 2018 • edited Loading

kaikreuzer commented Aug 20, 2018

kaikreuzer commented Aug 20, 2018

maggu2810 commented Aug 20, 2018

kaikreuzer commented Aug 20, 2018

htreu commented Aug 20, 2018

cweitkamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaikreuzer left a comment

Choose a reason for hiding this comment

kaikreuzer commented Aug 21, 2018

maggu2810 commented Aug 21, 2018

kaikreuzer commented Aug 21, 2018

htreu commented Aug 20, 2018 •

edited

Loading

maggu2810 commented Aug 20, 2018 •

edited

Loading