Profile API improvements #4516

sjsf · 2017-11-08T15:48:38Z

Having had some first feedback and some chats with @kaikreuzer, it obviously made sense to rework and simplify the profile API at some points. This is the current result.

Simplified the API for implementing profiles
(at the price of a little more complexity on CommunicationManager side)
Allow implementing profiles without exposing Items and ItemChannelLinks
to bindings
Restore backward-compatibility by making the default master profile
forward handleUpdate() calls to ThingHandlers again (see also CommunicationManager & DefaultMasterProfile : linking items through channels not supported by default #4464 (comment))
Exposing constants in SystemProfiles
Allow for easier evolvement of the API
Having the ProfileCallbacks requires less code within profiles and therefore
eliminates the need to export the default profiles for inheritance.
Renamed system profiles from master/slave to default/follow

Although this technically is APIBreaking (and therefore adding the corresponding label), there cannot yet exist a productive usage of this API, therefore I don't see any risks involved here.

Signed-off-by: Simon Kaufmann simon.kfm@googlemail.com

openhab-bot · 2017-11-08T16:16:24Z

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

https://community.openhab.org/t/enocean-bindings-opencean-or-aleoncean/12493/57

* Simplified the API for implementing profiles (at the price of a little more complexity on CommunicationManager side) * Allow implementing profiles without exposing Items and ItemChannelLinks to bindings * Maintain backward-compatibility by making the default master profile forward handleUpdate() calls to ThingHandlers again * Exposing constants in SystemProfiles * Allow for easier evolvement of the API * Having the ProfileCallbacks requires less code within profiles and therefore eliminates the need to export the default profiles for inheritance. Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

kungfoolfighting · 2017-11-08T22:08:50Z

...org/eclipse/smarthome/core/thing/internal/link/ItemChannelLinkConfigDescriptionProvider.java

    }

-    public void removeProfileAdvisor(ProfileAdvisor profileAdvisor) {
-        profileAdvisors.remove(profileAdvisor);
+    public void removeProfileAdvisor(ProfileTypeRegistry profileTypeRegistry) {


This function and the addProfileAdvisor function should probably be renamed, right?

kungfoolfighting · 2017-11-09T01:04:26Z

...thome.core.thing/src/main/java/org/eclipse/smarthome/core/thing/profiles/SystemProfiles.java

+    public static final TriggerProfileType RAWBUTTON_TOGGLE_SWITCH_TYPE = new TriggerProfileType() {
+        @Override
+        public ProfileTypeUID getUID() {
+            return SLAVE;


maggu2810 · 2017-11-09T10:42:40Z

...org/eclipse/smarthome/core/thing/internal/link/ItemChannelLinkConfigDescriptionProvider.java

+    }
+
+    private boolean isSupportedChannelType(ProfileType profileType, Channel channel) {
+        return profileType instanceof StateProfileType


profileType instanceof TriggerProfileType?

No I think that StateProfileType is what is meant here, but I don't quite understand why a StateProfileType would always support all ChannelTypes. Could there not be profiles that only support a specific channel type?

maggu2810 · 2017-11-09T10:46:48Z

...marthome.core.thing/src/main/java/org/eclipse/smarthome/core/thing/profiles/ProfileType.java

+    /**
+     * Constant for mathing *ANY* item type.
+     */
+    public static final Collection<String> ANY_ITEM_TYPE = new ArrayList<>(0);


Just a minor notice:
We can declare variables in Java interfaces. By default, these are public, final and static.
So, no need to add the default modifiers.

maggu2810 · 2017-11-09T10:47:23Z

...thome.core.thing/src/main/java/org/eclipse/smarthome/core/thing/profiles/SystemProfiles.java

+ * @author Simon Kaufmann - initial contribution and API.
+ *
+ */
+public interface SystemProfiles {


Same comments about default modifiers as above.

maggu2810 · 2017-11-09T10:47:49Z

...e.core.thing/src/main/java/org/eclipse/smarthome/core/thing/profiles/TriggerProfileType.java

+    /**
+     * Constant for mathing *ANY* channel type.
+     */
+    public static final Collection<ChannelTypeUID> ANY_CHANNEL_TYPE = new ArrayList<>(0);


see above (default modifiers)

kungfoolfighting · 2017-11-09T12:43:41Z

...org/eclipse/smarthome/core/thing/internal/link/ItemChannelLinkConfigDescriptionProvider.java

+
+    private boolean isSupportedChannelType(ProfileType profileType, Channel channel) {
+        return profileType instanceof StateProfileType
+                || ((TriggerProfileType) profileType)


While it is currently the case that usually profiles fall within the StateProfileType or the TriggerProfileType group, it would make sense to check if the profile type is an instance of TriggerProfileType here anyways in order to make sure we dont get a problem here later, in case other types are added.

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

kungfoolfighting · 2017-11-09T12:50:27Z

I might have done something wrong, but a working item definition that used the rawbutton channel and a toggle profile stopped working after I merged this patch.
I get a log entry that says It was not able to find a profile factory for this profile type. This message is from line 211 in CommunicationManager.

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

sjsf · 2017-11-09T12:51:32Z

Thanks, I will have a look.

sjsf · 2017-11-09T12:55:12Z

What does your item definition look like?
And what exactly does the warning in your log say?

kungfoolfighting · 2017-11-09T12:55:24Z

...org/eclipse/smarthome/core/thing/internal/link/ItemChannelLinkConfigDescriptionProvider.java

@@ -107,7 +107,7 @@ private boolean isSupportedItemType(ProfileType profileType, Item item) {
    }

    private boolean isSupportedChannelType(ProfileType profileType, Channel channel) {
-        return profileType instanceof StateProfileType
+        return profileType instanceof TriggerProfileType


This must be wrong now. It would now always return true if profileType is of type TriggerProfileType. That would mean that the rest of the condition is superfluous.

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

...from master/slave to default/follow Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

philomatic · 2017-11-09T13:40:24Z

Thanks for being compliant with me! 😄

If I get this right there is no need for another ProfileCallback implementation right now (Profiles alone "should" be enough for information flow)?
It's irritating in the first moment, but reduces complexity for new profiles and code duplication a lot!

LGTM!

philomatic · 2017-11-09T13:50:59Z

How do you want to deal with

@Deprecated
void handleUpdate(ChannelUID channelUID, State newState);

in ThingManager?

I think we should either stay backward-compatible by forwarding handleUpdate() calls (and therefore cancel deprecation?) or drop support for handleUpdate() completely before 1.0.
Just mentioning this so we won't have to deal with it twice.

Best regards

kungfoolfighting · 2017-11-09T13:51:23Z

@SJKA I don't have the code at work atm. But it looks something like this:
Switch ToggleSwitch {channel="bindingname:thingID:triggerChannelName" [profile="system:RawButtonToggleSwitchProfile"] }
Thinking about it, maybe I forgot to rename the profile from RawButtonToggleProfile to RawButtonToggleSwitchProfile. It was very late yesterday. So I will check tonight and keep you posted.

sjsf · 2017-11-09T13:58:00Z

maybe I forgot to rename the profile

That's why I've been asking... It should read rawbutton-toggle-switch or system:rawbutton-toggle-switch now.

kungfoolfighting · 2017-11-09T14:06:13Z

Right My mistake. So It if the ProfileTypeUID didnt change, then it is still the correct string, being "system:rawbutton-toggle". It did work yesterday before the merge and I was happily toggling my lights. I just made a mistake right now. So I had a working item, and now it no longer works.
I will post the exact logger message tonight. but it was something like
"no ProfileFactory found which supports 'itemname_system_rawbutton-toggle_channelname'" I am not exactly certain anymore, but it concatenated parts with underscore and the system_rawbutton_toggle was part of it.
Maybe that helps a bit. But don't spend too much time investigating on this flimsy information before I have verified it.

The exact same message was also printed for my custom enocean profile that I wrote, linked to my custom trigger channel. But that might have potentially other reasons that are unrelated, since this profile didn't work before the merge.

sjsf · 2017-11-09T15:08:37Z

No, my initial post was wrong (and I corrected it immediately, but that doesn't send out notification mails). The profile ID indeed has changed and got a -switch appended. Sure, just try it out once you find the time and let me know...

kaikreuzer · 2017-11-10T07:58:15Z

How do you want to deal with @Deprecated void handleUpdate?

-> to be discussed in #4525.

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

kungfoolfighting · 2017-11-10T09:43:16Z

After I appended the -switch to the profile name, the toggle profile worked again.
I am still not able to make my own profile work.
Do I need to define some XML or something?
What sort of information do I need to provide so you can tell me what I am doing wrong?
None of the break points in any ofthe functions in the profile factory are ever hit.
I have basically just taken the toggle profile as a template and done everything the same way.
When looking into the code I simply cannot find how the registries are being filled with the factories though. It is all obfuscated through the osgi services.

kaikreuzer

Looks like a huge simplification for profile implementors (as can be seen at the RawButtonToggleSwitchProfile).

Some small remarks on the code below.

kaikreuzer · 2017-11-10T08:25:58Z

...in/java/org/eclipse/smarthome/core/thing/internal/profiles/RawButtonToggleSwitchProfile.java

+
+    @Override
+    public void onUpdate(State state) {
+        this.previousState = state;


you can omit "this" here (as you do in onTrigger as well)

kaikreuzer · 2017-11-10T08:26:54Z

...ng/src/main/java/org/eclipse/smarthome/core/thing/internal/profiles/SystemFollowProfile.java

+import org.slf4j.LoggerFactory;
+
+/**
+ * This is the default implementation for a slave profile.


slave -> follow

kaikreuzer · 2017-11-10T08:27:09Z

...ng/src/main/java/org/eclipse/smarthome/core/thing/internal/profiles/SystemFollowProfile.java

+ * In contrast to the {@link SystemDefaultProfile} it does not forward any commands to the ThingHandler. Instead, it
+ * turn {@link State} updates into {@link Command}s (if possible) and then forwards those to the {@link ThingHandler}.
+ * <p>
+ * This allows devices to be operated as "slaves" of another one directly, without the need to write any rules.


slaves -> followers

kaikreuzer · 2017-11-10T08:29:21Z

...g/src/main/java/org/eclipse/smarthome/core/thing/internal/profiles/SystemProfileFactory.java

+    }
+
+    @Override
+    public Collection<@NonNull ProfileType> getAll() {


Instead of using @NonNull here, shouldn't you use @NonNullByDefault on the class?

kaikreuzer · 2017-11-10T09:19:42Z

...home.core.thing/src/main/java/org/eclipse/smarthome/core/thing/profiles/ProfileCallback.java

+     *
+     * @param command
+     */
+    void sendCommandEvent(Command command);


I would remove the word "Event" here - after all, you are sending a command, not an event.

kaikreuzer · 2017-11-10T09:44:05Z

...thome.core.thing/src/main/java/org/eclipse/smarthome/core/thing/profiles/TriggerProfile.java

+     *
+     * @param state the new {@link State}
+     */
+    void onUpdate(State state);


Maybe better onStateUpdate?

kaikreuzer · 2017-11-10T09:53:43Z

....core.thing/src/main/java/org/eclipse/smarthome/core/thing/profiles/ProfileTypeProvider.java

+ *
+ */
+@NonNullByDefault
+public interface ProfileTypeProvider extends Provider<ProfileType> {


Shouldn't this provider rather be similar to ThingTypeProvider etc., i.e. specifically able to provide a profile type for a given locale (such that its label is localisable)?

kaikreuzer · 2017-11-10T09:55:33Z

...se.smarthome.core.thing/src/main/java/org/eclipse/smarthome/core/thing/profiles/Profile.java

@@ -19,6 +21,7 @@
 * @author Simon Kaufmann - initial contribution and API.
 *
 */
+@NonNullByDefault
 public interface Profile {



I would expect to have a ProfileTypeUID getProfileTypeUID(); method here.

kaikreuzer · 2017-11-10T09:58:34Z

...g/src/main/java/org/eclipse/smarthome/core/thing/internal/profiles/SystemProfileFactory.java

+
+    @Override
+    public void addProviderChangeListener(ProviderChangeListener<@NonNull ProfileType> listener) {
+        // TODO Auto-generated method stub


replace or delete comment

kaikreuzer · 2017-11-10T09:58:37Z

...g/src/main/java/org/eclipse/smarthome/core/thing/internal/profiles/SystemProfileFactory.java

+
+    @Override
+    public void removeProviderChangeListener(ProviderChangeListener<@NonNull ProfileType> listener) {
+        // TODO Auto-generated method stub


replace or delete comment

* renamed profile methods * made provider/registry obey obey localization by following the example of ThingTypeProvider * fixed a bug in the provider registration * fixed several typos Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

kaikreuzer · 2017-11-10T12:26:06Z

...se.smarthome.core.thing/src/main/java/org/eclipse/smarthome/core/thing/profiles/Profile.java

@@ -24,4 +25,13 @@
 @NonNullByDefault
 public interface Profile {

+    ProfileTypeUID getProfileTypeUID();


Would you also spend it a single line of JavaDoc? :-)

maggu2810 · 2017-11-13T09:58:11Z

I see several non-null violations

Identifiable<T> is using @NonNullByDefault
ProfileType extends Identifiable<ProfileTypeUID> and is using @NonNullByDefault
StateProfileType extends ProfileType and is using @NonNullByDefault
SystemProfiles contains a new annonymous StateProfileType implementations and does not use @NonNullByDefault

Related to: #4516 (comment) Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>

sjsf added the APIBreaking label Nov 8, 2017

sjsf force-pushed the profileAPISimplification branch from e860e1e to 9675aa4 Compare November 8, 2017 16:07

sjsf force-pushed the profileAPISimplification branch from 9675aa4 to 5dbb680 Compare November 8, 2017 17:07

sjsf force-pushed the profileAPISimplification branch from 5dbb680 to c25e449 Compare November 8, 2017 17:13

kungfoolfighting reviewed Nov 8, 2017

View reviewed changes

kungfoolfighting reviewed Nov 9, 2017

View reviewed changes

maggu2810 reviewed Nov 9, 2017

View reviewed changes

kungfoolfighting reviewed Nov 9, 2017

View reviewed changes

Simon Kaufmann added 2 commits November 9, 2017 13:46

fixed method names and constant

c6829ea

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

fixed accidental profile re-use across links

52aee65

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

removed superfluous modifiers

0582930

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

kungfoolfighting reviewed Nov 9, 2017

View reviewed changes

Simon Kaufmann added 3 commits November 9, 2017 13:59

fixed boolean expression in config description provider

7444fde

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

renamed system profiles

05b59af

...from master/slave to default/follow Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

using constant for item type

d57e48e

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

sjsf mentioned this pull request Nov 9, 2017

CommunicationManager & DefaultMasterProfile : linking items through channels not supported by default #4464

Closed

sjsf added the Core label Nov 9, 2017

passing config to profile factories

866305c

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

kaikreuzer suggested changes Nov 10, 2017

View reviewed changes

incorporated review feedback

f7d1a0d

* renamed profile methods * made provider/registry obey obey localization by following the example of ThingTypeProvider * fixed a bug in the provider registration * fixed several typos Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

kaikreuzer reviewed Nov 10, 2017

View reviewed changes

kaikreuzer approved these changes Nov 10, 2017

View reviewed changes

kaikreuzer merged commit 9cddf01 into eclipse-archived:master Nov 10, 2017

sjsf deleted the profileAPISimplification branch November 10, 2017 15:05

maggu2810 mentioned this pull request Nov 13, 2017

add missing non-null (default) annotation for system profiles #4538

Merged

sjsf mentioned this pull request Nov 13, 2017

System trigger profiles #4542

Closed

sjsf pushed a commit that referenced this pull request Nov 13, 2017

add missing non-null (default) annotation for system profiles (#4538)

4a27988

Related to: #4516 (comment) Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>

sjsf mentioned this pull request Nov 14, 2017

re-rename demo.items #4545

Merged

kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017

kgoderis mentioned this pull request Dec 3, 2017

Default System Profile behaviour #4669

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Profile API improvements #4516

Profile API improvements #4516

sjsf commented Nov 8, 2017 •

edited

Loading

openhab-bot commented Nov 8, 2017

kungfoolfighting Nov 8, 2017 •

edited

Loading

kungfoolfighting Nov 9, 2017

maggu2810 Nov 9, 2017

kungfoolfighting Nov 9, 2017

maggu2810 Nov 9, 2017

maggu2810 Nov 9, 2017

maggu2810 Nov 9, 2017

kungfoolfighting Nov 9, 2017

kungfoolfighting commented Nov 9, 2017

sjsf commented Nov 9, 2017

sjsf commented Nov 9, 2017 •

edited

Loading

kungfoolfighting Nov 9, 2017

philomatic commented Nov 9, 2017

philomatic commented Nov 9, 2017 •

edited

Loading

kungfoolfighting commented Nov 9, 2017 •

edited

Loading

sjsf commented Nov 9, 2017 •

edited

Loading

kungfoolfighting commented Nov 9, 2017 •

edited

Loading

sjsf commented Nov 9, 2017

kaikreuzer commented Nov 10, 2017

kungfoolfighting commented Nov 10, 2017

kaikreuzer left a comment

kaikreuzer Nov 10, 2017

kaikreuzer Nov 10, 2017

kaikreuzer Nov 10, 2017

kaikreuzer Nov 10, 2017

kaikreuzer Nov 10, 2017

kaikreuzer Nov 10, 2017

kaikreuzer Nov 10, 2017

kaikreuzer Nov 10, 2017

kaikreuzer Nov 10, 2017

kaikreuzer Nov 10, 2017

kaikreuzer Nov 10, 2017

maggu2810 commented Nov 13, 2017

Profile API improvements #4516

Profile API improvements #4516

Conversation

sjsf commented Nov 8, 2017 • edited Loading

openhab-bot commented Nov 8, 2017

kungfoolfighting Nov 8, 2017 • edited Loading

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

kungfoolfighting commented Nov 9, 2017

sjsf commented Nov 9, 2017

sjsf commented Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

philomatic commented Nov 9, 2017

philomatic commented Nov 9, 2017 • edited Loading

kungfoolfighting commented Nov 9, 2017 • edited Loading

sjsf commented Nov 9, 2017 • edited Loading

kungfoolfighting commented Nov 9, 2017 • edited Loading

sjsf commented Nov 9, 2017

kaikreuzer commented Nov 10, 2017

kungfoolfighting commented Nov 10, 2017

kaikreuzer 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

maggu2810 commented Nov 13, 2017

sjsf commented Nov 8, 2017 •

edited

Loading

kungfoolfighting Nov 8, 2017 •

edited

Loading

sjsf commented Nov 9, 2017 •

edited

Loading

philomatic commented Nov 9, 2017 •

edited

Loading

kungfoolfighting commented Nov 9, 2017 •

edited

Loading

sjsf commented Nov 9, 2017 •

edited

Loading

kungfoolfighting commented Nov 9, 2017 •

edited

Loading