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

[sitemap] Remove sendFrequency parameter for Slider/Colorpicker widgets #4347

Merged
merged 1 commit into from
Sep 7, 2024

Conversation

lolodomo
Copy link
Contributor

Related to #4338

Signed-off-by: Laurent Garnier lg.hc@free.fr

Related to openhab#4338

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo lolodomo requested a review from a team as a code owner August 14, 2024 09:21
@lolodomo lolodomo changed the title [sitemap] Remove snedFrequency parameter for Slider/Colorpicker widgets [sitemap] Remove sendFrequency parameter for Slider/Colorpicker widgets Aug 14, 2024
Copy link
Contributor

@mherwege mherwege left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kaikreuzer
Copy link
Member

If people have this parameter in their sitemap, this is a backward-compatibility breaking change.
I wonder if we should maybe continue to allow it and rather log a warning, if people still use it. At a later stage, we could then remove it from the syntax. Wdyt?

@lolodomo
Copy link
Contributor Author

It has been already removed from the Main UI sitemap editor.
It is very improbable that this parameter is still used by anyone or maybe only by those that have very old sitemaps dating of the age of Classic UI.
Replacing by a log is possible but will require additional work & test, I am not sure it's worth the effort.

@holgerfriedrich
Copy link
Member

holgerfriedrich commented Sep 6, 2024

@lolodomo what will happen to users of the text based configuration?
We still list sendFrequency as an allowed option in the documentation of 4.2.x.
Or do I get something wrong?

@lolodomo
Copy link
Contributor Author

lolodomo commented Sep 7, 2024

@lolodomo what will happen to users of the text based configuration?

In case someone has this parameter in a sitemap, the loading of the sitemap will then fail and an error will be logged.

As previously explained, I expect it should impact only a very minority of users, maybe few old OH users that created sitemaps many years ago and preferred Classic UI over Basic UI when Classic UI was still existing. This parameter was only for Classic UI.

By the way, we should add a breaking change notice in 4.3 release notes.

We still list sendFrequency as an allowed option in the documentation of 4.2.x. Or do I get something wrong?

We are now working on version 4.3 and this was already removed from documentation for this next version: openhab/openhab-docs#2346

@lolodomo
Copy link
Contributor Author

lolodomo commented Sep 7, 2024

@kaikreuzer : I would be in favour to not loose too much time with a 2 steps solution as this parameter is probably still used by nobody. A breaking change in the release notes seems to be sufficient IMHO.
I will not spend time on developing and testing a solution that will keep a backward compatibility with an additional log message. But I am also ok if someone wants to do it.

@kaikreuzer
Copy link
Member

Ok, then please create a PR for the breaking change in the distro repo - thanks!

@kaikreuzer kaikreuzer merged commit edf5b62 into openhab:main Sep 7, 2024
5 checks passed
@kaikreuzer kaikreuzer added this to the 4.3 milestone Sep 7, 2024
@kaikreuzer kaikreuzer added the enhancement An enhancement or new feature of the Core label Sep 7, 2024
@lolodomo lolodomo deleted the sitemap_remove_sendFrequency branch September 7, 2024 16:50
@lolodomo
Copy link
Contributor Author

lolodomo commented Sep 7, 2024

Ok, then please create a PR for the breaking change in the distro repo

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API breaking enhancement An enhancement or new feature of the Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants