Skip to content
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

[fix] register settings callback for weather manager is called before it is ready #13326

Merged
merged 1 commit into from Jan 7, 2018

Conversation

@xhaggi
Copy link
Member

commented Jan 7, 2018

As @FernetMenta pointed out, the weather manager is not ready when we want to register the settings callbacks. Now the registration/deregistration is moved to our service manager.

@MartijnKaijser ping you too.

@xhaggi xhaggi requested a review from FernetMenta Jan 7, 2018

@xhaggi xhaggi force-pushed the xhaggi:fix-register-settings branch 2 times, most recently from b63f693 to 9a3c15c Jan 7, 2018

@@ -151,6 +152,9 @@ bool CServiceManager::InitStageTwo(const CAppParamParser &params)

m_weatherManager.reset(new CWeatherManager());

// register settings callbacks
GetSettings().GetSettingsManager()->RegisterCallback(m_weatherManager.get(), { CSettings::SETTING_WEATHER_ADDON, CSettings::SETTING_WEATHER_ADDONSETTINGS });

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Jan 7, 2018

Member

This should be done inside weatherManager (maybe ctor/dtor). ServiceManager should not bother with internals.

This comment has been minimized.

Copy link
@xhaggi

xhaggi Jan 7, 2018

Author Member

done

@xhaggi xhaggi force-pushed the xhaggi:fix-register-settings branch from 9a3c15c to a19bc8e Jan 7, 2018

@xhaggi xhaggi force-pushed the xhaggi:fix-register-settings branch from a19bc8e to 48f90cc Jan 7, 2018

@FernetMenta
Copy link
Member

left a comment

Great. Thanks

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Jan 7, 2018

had to kill all 5 triggers for this single PR.
Jenkins build this please

@xhaggi

This comment has been minimized.

Copy link
Member Author

commented Jan 7, 2018

Thanks

@xhaggi xhaggi merged commit a059b8c into xbmc:master Jan 7, 2018

1 check passed

default You're awesome. Have a cookie
Details

@Rechi Rechi added this to the L 18.0-alpha1 milestone Jan 7, 2018

@ksooo

This comment has been minimized.

Copy link
Member

commented Jan 7, 2018

@xhaggi: There is another reference to the weather manager left in Settings.cpp : 48f90cc#diff-8d943449895d0ff6b503e49c8ecf6868R832

Is this intended? If not, the include for the weather manager header could be removed from Settings.cpp

@xhaggi xhaggi deleted the xhaggi:fix-register-settings branch Feb 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.