-
Notifications
You must be signed in to change notification settings - Fork 22
Fix handler callbacks WebIDL, update text and examples. #103
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
Conversation
Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>
Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>
Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>
Do not understand what
I think we cannot put dispatcher code there. According do my understanding the snippet above should be simply
|
Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>
@danielpeintner @knimura @mkovatsc could any of you review please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the changes are fine.
The two comments are just to clarify what happens if no read or write handler is provided. The current text is not 100% clear to me.
Apart from that the PR is fine!
</p> | ||
<p> | ||
There SHOULD be at most one handler for any given <a>Property</a>. If no handler is initialized for any given <a>Property</a>, implementations SHOULD implement default property read without calling this hook. | ||
There SHOULD be at most one handler for any given <a>Property</a> and newly added handlers replace the old handlers. If no handler is initialized for any given <a>Property</a>, implementations SHOULD implement a default property read handler. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comment: in node-wot we deal with any property without a readHandler by simply returning the value as is (last set value). Hence I am a bit unsure about the 2nd sentence. Does it make sense to simplify the 2nd sentence above to something like this?
"If no handler is provided for a given Property, implementations are required to report the property value as is (e.g., set by writeProperty() or ThingProperty.value)"
</p> | ||
<p> | ||
There SHOULD be at most one write handler for any given <a>Property</a>. If no write handler is initialized for any given <a>Property</a>, implementations SHOULD implement default property update and notifying observers. | ||
There SHOULD be at most one write handler for any given <a>Property</a> and newly added handlers replace the old handlers. If no write handler is initialized for any given <a>Property</a>, implementations SHOULD implement default property update and notifying observers on change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the previous comment the 2nd sentence might look like
"If no write handler is initialized for a given Property, implementations SHOULD set the property value with the provided new data and notify observers."
Maybe "default property update" means the same to you. IF so fine also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking was that the handler actually implements writing the property in a way known only to it (e.g. using local call such as GPIO or network etc). Or it may only want to implement some side-effects (hooks) to setting the property value. Of course the WoT runtime can always update the "proxy" property value it maintains (i.e. in your case you only have the "proxy" property value, basically a memory location that is not bound to HW).
That is why I used the term "default property update", to leave open what that actually means (until we define the algorithms), since it may be more than just updating a local memory location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. Makes sense. Let's leave it as is for now.
Fixes #102. |
Fixes #102.