Allow ProfileTypeBuilder to pass its three filters list to both implementations of ProfileType #4325

obones · 2024-07-22T08:54:07Z

This pull request moves getSupportedChannelTypeUIDs and getSupportedItemTypesOfChannel to the ProfileType interface so that StateProfileType and TriggerProfileType can both take the three filter lists given by ProfileTypeBuilder

It fixes #4293 and will allow bundle writers to limit their transformation profiles to just the channels/items that are related to their profiles.

…o the ProfileType interface so that StateProfileType and TriggerProfileType can both take the three filter lists given by ProfileTypeBuilder Fixes openhab#4293 Signed-off-by: Olivier Sannier <[email protected]> (github: obones) Signed-off-by: OBones <[email protected]>

… all ProfileTypes Signed-off-by: OBones <[email protected]>

J-N-K

Pleas also check why there are test failures in ProfileTypeResourceTest.

...t.core/src/main/java/org/openhab/core/io/rest/core/internal/profile/ProfileTypeResource.java

...main/java/org/openhab/core/thing/internal/link/ItemChannelLinkConfigDescriptionProvider.java

bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/profiles/ProfileType.java

obones · 2024-07-25T07:35:26Z

I don't know if it's the same for everyone, but it takes a really long time to build the entire openhab-core project, and I can't even get it to work properly in its entirety with VSCode under Windows.
Hence the reason why I pushed commits to see what GitHub actions would say and try to figure out which sub project is complaining.
I'll make sure to squash those commits into a more readable one.

J-N-K · 2024-07-25T07:37:24Z

No need for squashing, I can do that during merge. In fact, if you squash and force-push usually the comment history is messed up, so better just push additional commits.

…ile type can tell onto which channel kind it applies, if any Signed-off-by: OBones <[email protected]>

Signed-off-by: OBones <[email protected]>

…ency Signed-off-by: OBones <[email protected]>

Signed-off-by: OBones <[email protected]>

obones · 2024-07-26T15:34:13Z

I have just pushed some changes that I believe are addressing your fine remarks.
Do not hesitate to tell me if I missed other things.

J-N-K

Thanks again for you work and sorry for being a PITA here.

J-N-K · 2024-07-26T16:32:43Z

....core/src/main/java/org/openhab/core/io/rest/core/internal/link/ItemChannelLinkResource.java

            if (!(profileType.getSupportedItemTypes().isEmpty()
                    || profileType.getSupportedItemTypes().contains(itemType))
-                    || !(((TriggerProfileType) profileType).getSupportedChannelTypeUIDs().isEmpty()
-                            || ((TriggerProfileType) profileType).getSupportedChannelTypeUIDs()
-                                    .contains(channel.getChannelTypeUID()))) {
+                    || !(profileType.getSupportedChannelTypeUIDs().isEmpty()
+                            || profileType.getSupportedChannelTypeUIDs().contains(channel.getChannelTypeUID()))
+                    || !(profileType.getSupportedItemTypesOfChannel().isEmpty()
+                            || profileType.getSupportedItemTypesOfChannel().contains(itemType))) {


That looks really horrible. It would probably be better to

Collection<String> supportedItemTypes = profileType.getSupportedItemTypes(); ... if ((!supportedItemType.isEmpty() && !supportedItemTypes.contains(itemType)) || ...) {

Why do you check profileType.getSupportedItemTypesOfChannel? A profile can change the item-type:

The timestamp update profile works with all channel-types (e.g. a humidity channel with supported item-type Number:Dimensionless) but can only be linked to DateTime items. So a check would fail (in fact, currently the profile does not provide information about accepted channel types, so this will not cause an issue, but in principle it could).

A profile could map numeric values to strings, so the channel would support Number, but the profile String. In this case you could never link them.

I agree that it looks horrible, but it was jut to "fit in" with the already existing code.

As to why I do the check, well it's because I did a global search for the interface methods and made sure both methods are called in all the places where one was called following an instanceof call.

I agree, the style was not good before either and if you want to keep it: ok. But the getSupportedItemTypesOfChannel is excluding valid combinations.

I'm definitely in favor of changing the code style here, but as always when I'm submitting a pull request, I try to limit my changes to the code logic, adapting to the local style I encounter.
I'll try to come up with a more sensible way of writing this up.

J-N-K · 2024-07-26T16:34:33Z

...t.core/src/main/java/org/openhab/core/io/rest/core/internal/profile/ProfileTypeResource.java

-                return true;
-            }
+    private boolean profileTypeMatchesChannelType(ProfileType profileType, ChannelType channelType) {
+        @Nullable


You don't need @Nullable annotation for local variables.

My IDE (VSCode) disagrees and outputs a warning saying that the value cannot be null. That's what I've been doing in my plugin bundles, but I can remove it if you prefer.

IMO it's wrong. The default locations for @NonNullByDefault are PARAMETER, RETURN_TYPE, FIELD, TYPE_BOUND, TYPE_ARGUMENT, so it does not apply to local variables.

Fair enough, I have removed them

...main/java/org/openhab/core/thing/internal/link/ItemChannelLinkConfigDescriptionProvider.java

....core.thing/src/main/java/org/openhab/core/thing/internal/profiles/StateProfileTypeImpl.java

...ore.thing/src/main/java/org/openhab/core/thing/internal/profiles/TriggerProfileTypeImpl.java

bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/profiles/ProfileType.java

Signed-off-by: OBones <[email protected]>

…the two other locations where ProfileType are tested for applicability Signed-off-by: OBones <[email protected]>

obones · 2024-07-29T13:45:23Z

The more I look at how things are going, the more I feel it would be beneficial to have a ProfileTypeUtil abstract class that provides two methods receiving (among other things) a Channel for the first and a ChannelType for the second and those methods would return a boolean indicating if a given ProfileType applies to the channnel(type)
This would avoid very similar code to be placed in the three different places it is currently placed.
Does that make any sense to you?

obones · 2024-09-28T20:31:26Z

@J-N-K , sorry to ping you directly, but do you have an opinion on my last suggestion?

obones requested a review from a team as a code owner July 22, 2024 08:54

obones force-pushed the 4293-ProfileTypeBuilder_should_pass_all_lists branch from be1c40a to a3fea88 Compare July 22, 2024 09:25

obones mentioned this pull request Jul 22, 2024

ProfileTypeBuilder.withSupportedChannelTypeUIDs is ignored by StateProfileTypeImpl #4293

Open

Use getSupportedChannelTypeUIDs and getSupportedItemTypesOfChannel on…

347fbe7

… all ProfileTypes Signed-off-by: OBones <[email protected]>

J-N-K requested changes Jul 24, 2024

View reviewed changes

obones added 5 commits July 26, 2024 14:04

Introduce getSupportedChannelKind on ProfileType so that a given prof…

e092440

…ile type can tell onto which channel kind it applies, if any Signed-off-by: OBones <[email protected]>

Must check for the channel kind supported by the profile type

7d37835

Signed-off-by: OBones <[email protected]>

Must not exit too early, both lists must be tested

e3052cc

Signed-off-by: OBones <[email protected]>

Use code similar to the one in ProfileTypeResource for better consist…

116046b

…ency Signed-off-by: OBones <[email protected]>

Improve the javadoc for ProfileType

86e6852

Signed-off-by: OBones <[email protected]>

J-N-K requested changes Jul 26, 2024

View reviewed changes

obones added 2 commits July 29, 2024 15:22

Remove @nullable on local variables

ef03c14

Signed-off-by: OBones <[email protected]>

Rewrite the link() tests for better styling and make them similar to …

d0130fd

…the two other locations where ProfileType are tested for applicability Signed-off-by: OBones <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow ProfileTypeBuilder to pass its three filters list to both implementations of ProfileType #4325

Allow ProfileTypeBuilder to pass its three filters list to both implementations of ProfileType #4325

obones commented Jul 22, 2024

J-N-K left a comment

obones commented Jul 25, 2024

J-N-K commented Jul 25, 2024

obones commented Jul 26, 2024

J-N-K left a comment

J-N-K Jul 26, 2024

obones Jul 26, 2024

J-N-K Jul 27, 2024

obones Jul 29, 2024

J-N-K Jul 26, 2024

obones Jul 26, 2024

J-N-K Jul 27, 2024

obones Jul 29, 2024

obones commented Jul 29, 2024

obones commented Sep 28, 2024

Allow ProfileTypeBuilder to pass its three filters list to both implementations of ProfileType #4325

Are you sure you want to change the base?

Allow ProfileTypeBuilder to pass its three filters list to both implementations of ProfileType #4325

Conversation

obones commented Jul 22, 2024

J-N-K left a comment

Choose a reason for hiding this comment

obones commented Jul 25, 2024

J-N-K commented Jul 25, 2024

obones commented Jul 26, 2024

J-N-K 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

obones commented Jul 29, 2024

obones commented Sep 28, 2024