-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[weatherunderground] Consider TimeZoneProvider to get the timezone #7878
Conversation
Also fix few warnings about null check Signed-off-by: Laurent Garnier <[email protected]>
Travis tests were successfulHey @lolodomo, |
@@ -82,7 +86,7 @@ public boolean supportsThingType(ThingTypeUID thingTypeUID) { | |||
ThingTypeUID thingTypeUID = thing.getThingTypeUID(); | |||
|
|||
if (thingTypeUID.equals(THING_TYPE_WEATHER)) { | |||
return new WeatherUndergroundHandler(thing, localeProvider, unitProvider); | |||
return new WeatherUndergroundHandler(thing, localeProvider, unitProvider, timeZoneProvider.getTimeZone()); |
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.
Shouldn't the timeZoneProvider be passed in its entirety to allow users to change the timezone without having to recreate the 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.
At the first reading, I did not understand your point. But now I realize I am not sure how it behaves when a core system setting is updated.
The timezone is an openHAB system configuration setting that the user can update, using Paper UI for example. When this setting is updated, the bundle (which one exactly ?) is updated. As we have an OSGi reference to this service in the binding thing handler factory component, what will happen ? Will the handler factory component be restarted with the updated reference ? If true, all thing handlers will be created again and my solution is ok. Passing the TimeZoneProvider object to the thing handler will make no change.
In case the setting change will have no impact on the binding handler factory component (no deactivate/activate), your remark will be valid. But in this case, I don't understand how OSGi components are informed of changes in referenced components.
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.
This is not clear for me in fact.
I should test a change of timezone and see the impact on the thing handler factory and thing handlers.
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.
Ok, tested now. In fact, changing the timezone setting has no direct impact on the thing handler factory or thing handler. Nothing is restarted. So my solution is not good.
If I pass the TimeZoneProvider as parameter to the thing handler, it looks like it is correctly updated in the thing 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.
Fixed
Signed-off-by: Laurent Garnier <[email protected]>
Travis tests were successfulHey @lolodomo, |
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.
LGTM
…penhab#7878) * [weatherunderground] Consider TimeZoneProvider to get the timezone Also fix few warnings about null check * TimeZoneProvider passed as parameter to the thing handler Signed-off-by: Laurent Garnier <[email protected]>
…penhab#7878) * [weatherunderground] Consider TimeZoneProvider to get the timezone Also fix few warnings about null check * TimeZoneProvider passed as parameter to the thing handler Signed-off-by: Laurent Garnier <[email protected]> Signed-off-by: CSchlipp <[email protected]>
…penhab#7878) * [weatherunderground] Consider TimeZoneProvider to get the timezone Also fix few warnings about null check * TimeZoneProvider passed as parameter to the thing handler Signed-off-by: Laurent Garnier <[email protected]>
…penhab#7878) * [weatherunderground] Consider TimeZoneProvider to get the timezone Also fix few warnings about null check * TimeZoneProvider passed as parameter to the thing handler Signed-off-by: Laurent Garnier <[email protected]>
…penhab#7878) * [weatherunderground] Consider TimeZoneProvider to get the timezone Also fix few warnings about null check * TimeZoneProvider passed as parameter to the thing handler Signed-off-by: Laurent Garnier <[email protected]>
…penhab#7878) * [weatherunderground] Consider TimeZoneProvider to get the timezone Also fix few warnings about null check * TimeZoneProvider passed as parameter to the thing handler Signed-off-by: Laurent Garnier <[email protected]>
…penhab#7878) * [weatherunderground] Consider TimeZoneProvider to get the timezone Also fix few warnings about null check * TimeZoneProvider passed as parameter to the thing handler Signed-off-by: Laurent Garnier <[email protected]> Signed-off-by: Daan Meijer <[email protected]>
…penhab#7878) * [weatherunderground] Consider TimeZoneProvider to get the timezone Also fix few warnings about null check * TimeZoneProvider passed as parameter to the thing handler Signed-off-by: Laurent Garnier <[email protected]>
Also fix few warnings about null check
Signed-off-by: Laurent Garnier [email protected]