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.

Sign up for GitHub

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

Jump to bottom

[zoneminder] Fix calls to set state options #11463

Merged
merged 2 commits into from
Oct 31, 2021
Merged

[zoneminder] Fix calls to set state options #11463

merged 2 commits into from
Oct 31, 2021

Conversation

mhilbush
Copy link
Contributor

There are situations when the binding updates the state options even if the options haven't changed. This results in an unnecessary reload of the page in Basic UI. With this change, the state options are updated only if they change.

Signed-off-by: Mark Hilbush [email protected]

@mhilbush mhilbush added the bug An unexpected problem or unintended behavior of an add-on label Oct 27, 2021
@openhab-bot
Copy link
Collaborator

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

https://community.openhab.org/t/new-zoneminder-binding-for-zoneminder-versions-1-34-0/95112/362

@kaikreuzer kaikreuzer added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Oct 28, 2021
@lolodomo
Copy link
Contributor

My immediate thinking is that'll it should rather be done / checked by the core framework.
@cweitkamp : WDYT?

@cweitkamp
Copy link
Contributor

Definitely yes. And IIRC it is already implemented in BaseDynamicStateDescriptionProvider::setStateOptions(...).

https://github.com/openhab/openhab-core/blob/40e23e23ca58fdb39be3b656430e8dcf8b18a344/bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/binding/BaseDynamicStateDescriptionProvider.java#L79

@cweitkamp
Copy link
Contributor

Do you think the ordering of the list elements might be the culprit?

@mhilbush
Copy link
Contributor Author

Do you think the ordering of the list elements might be the culprit?

No. 😉

Look at my original code. I had the call to setStateOptions in the wrong part of the code. It was inside the for loop. 🙄

@mhilbush
Copy link
Contributor Author

I'll update the PR to move those calls to after the for loop. That should resolve the issue. Agree?

@cweitkamp
Copy link
Contributor

Agree?

I do.

@mhilbush
Fix update of state options
6fa1456 
Signed-off-by: Mark Hilbush <[email protected]>
@mhilbush
Copy link
Contributor Author

Ok, I believe this is the correct fix. Thanks for helping me find my error.

cweitkamp
cweitkamp approved these changes Oct 31, 2021
View reviewed changes
Copy link
Contributor

@cweitkamp cweitkamp left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@lolodomo Fine for you?

@cweitkamp cweitkamp added this to the 3.2 milestone Oct 31, 2021
lolodomo
lolodomo approved these changes Oct 31, 2021
View reviewed changes
Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

LGTM

@lolodomo lolodomo merged commit edd3e01 into openhab:main Oct 31, 2021
@lolodomo
Copy link
Contributor

I realized the PR title is wrong. Can you fix it?

@mhilbush mhilbush changed the title [zoneminder] Only update state options if they've changed [zoneminder] Fix calls to set state options Oct 31, 2021
@mhilbush
Copy link
Contributor Author

Good catch. fixed.

@mhilbush mhilbush deleted the zm-state-options branch October 31, 2021 12:52
dschoepel pushed a commit to dschoepel/openhab-addons that referenced this pull request Nov 9, 2021
@mhilbush @dschoepel
[zoneminder] Only update state options if they've changed (openhab#11463
4021987 
)

* Only update state options if they've changed

Signed-off-by: Mark Hilbush <[email protected]>

* Fix update of state options

Signed-off-by: Mark Hilbush <[email protected]>
Signed-off-by: Dave J Schoepel <[email protected]>
jpg0 pushed a commit to jpg0/openhab-addons that referenced this pull request Nov 10, 2021
@mhilbush @jpg0
[zoneminder] Only update state options if they've changed (openhab#11463
59d13c7 
)

* Only update state options if they've changed

Signed-off-by: Mark Hilbush <[email protected]>

* Fix update of state options

Signed-off-by: Mark Hilbush <[email protected]>
NickWaterton pushed a commit to NickWaterton/openhab-addons that referenced this pull request Dec 30, 2021
@mhilbush @NickWaterton
[zoneminder] Only update state options if they've changed (openhab#11463
eafbb9a 
)

* Only update state options if they've changed

Signed-off-by: Mark Hilbush <[email protected]>

* Fix update of state options

Signed-off-by: Mark Hilbush <[email protected]>
Signed-off-by: Nick Waterton <[email protected]>
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Jan 28, 2022
@mhilbush
[zoneminder] Only update state options if they've changed (openhab#11463
d6e139b 
)

* Only update state options if they've changed

Signed-off-by: Mark Hilbush <[email protected]>

* Fix update of state options

Signed-off-by: Mark Hilbush <[email protected]>
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
@mhilbush
[zoneminder] Only update state options if they've changed (openhab#11463
ecab60e 
)

* Only update state options if they've changed

Signed-off-by: Mark Hilbush <[email protected]>

* Fix update of state options

Signed-off-by: Mark Hilbush <[email protected]>
andan67 pushed a commit to andan67/openhab-addons that referenced this pull request Nov 6, 2022
@mhilbush @andan67
[zoneminder] Only update state options if they've changed (openhab#11463
614cffb 
)

* Only update state options if they've changed

Signed-off-by: Mark Hilbush <[email protected]>

* Fix update of state options

Signed-off-by: Mark Hilbush <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cweitkamp cweitkamp cweitkamp approved these changes

@lolodomo lolodomo lolodomo approved these changes

Assignees
No one assigned
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Milestone
3.2
Development

Successfully merging this pull request may close these issues.
5 participants
@mhilbush @openhab-bot @lolodomo @cweitkamp @kaikreuzer