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

Add Airflow, Application, Channel, and Mode Semantic Properties #4616

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jimtng
Copy link
Contributor

@jimtng jimtng commented Feb 23, 2025

Add new properties to support TV and Airconditioner points

Also upgrade groovy version to work with Java21
Adjust copyright header

@jimtng
Copy link
Contributor Author

jimtng commented Feb 23, 2025

@andrewfg fyi

@lsiepel
Copy link
Contributor

lsiepel commented Feb 23, 2025

Please separate version upgrade from semantic tags.

Not sure about Mode, might be fine but it seems not as specific as channel and application. Also wondering if adding these tags would need additional work in mainUI, @ghys ?

andrewfg added a commit to andrewfg/openhab-core that referenced this pull request Feb 23, 2025
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@jimtng
Copy link
Contributor Author

jimtng commented Feb 23, 2025

I've split them up into separate commits

@lsiepel
Copy link
Contributor

lsiepel commented Feb 23, 2025

I've split them up into separate commits

All commits are pushed? As I still see the version change and the tag together

@jlaur
Copy link
Contributor

jlaur commented Feb 23, 2025

@jimtng - can you add Airflow to the PR title as well (for the release notes)?

@jimtng jimtng changed the title Add Application, Channel, and Mode Properties Add Airflow, Application, Channel, and Mode Properties Feb 23, 2025
@jimtng
Copy link
Contributor Author

jimtng commented Feb 23, 2025

I've split them up into separate commits

All commits are pushed? As I still see the version change and the tag together

Yes they are split up properly. There are 4 commits with airflow in a separate commit to the other 3 tags. Should I split each tag into a separate commit?

@jimtng
Copy link
Contributor Author

jimtng commented Feb 23, 2025

@jimtng - can you add Airflow to the PR title as well (for the release notes)?

Title updated

@jimtng jimtng changed the title Add Airflow, Application, Channel, and Mode Properties Add Airflow, Application, Channel, and Mode Semantic Properties Feb 23, 2025
@rkoshak
Copy link

rkoshak commented Feb 24, 2025

Also wondering if adding these tags would need additional work in mainUI, @ghys ?

I can't definitively answer but given that custom tags are supported everywhere and I believe MainUI uses the REST API endpoint to get the lags there shouldn't be any wider changes required.

But I wonder if it makes sense to have a more deliberate discussion on what tags belong or not at a higher level and come up with a PR to address them all instead adding two here and four there in separate PRs without really designing the tags.

For example, Battery, online status, pressure, VoC, Radon, air quality, are missing properties. There are a bunch of car related points and properties missing and car is missing from Equipment.

On-the-other-hand, do we really need seven different types of Doors at the Equipment level and does it make sense to have smokealarm at all given the alarm/smoke point/property combo?

I worry that if we just add stuff here and there the lists will always remain inconsistent and eventually grow beyond usability.

Note, I'm all for adding the four properties listed here. Even mode makkes sense: HVAC Equipment has a Control/Mode Point to set the heating/cooling/auto status.

@andrewfg
Copy link
Contributor

andrewfg commented Feb 24, 2025

I worry that if we just add stuff here and there the lists will always remain inconsistent and eventually grow beyond usability.

See #4615 in which we are seeking to apply an XSD schema restriction so that addon developers will be able to offer one “point” tag and one “property” tag; this offer would be a hint that users could choose to accept or override.

Battery, online status, pressure, VoC, Radon, air quality, are missing properties.

@rkoshak may I therefore make a call to action on you to open a PR for those; or at least open an Issue..

do we really need seven different types of Doors at the Equipment level

PS see also #4617 ..

@rkoshak
Copy link

rkoshak commented Feb 24, 2025

@rkoshak may I therefore make a call to action on you to open a PR for those; or at least open an Issue..

That misses my whole point.

But I was already creating a PR. #4619

@lolodomo
Copy link
Contributor

Also upgrade groovy version to work with Java21
Adjust copyright header

Please create a separate PR for that to have it merged quickly even if your proposal of new properties is finally not validated.

@jimtng
Copy link
Contributor Author

jimtng commented Feb 26, 2025

Please create a separate PR for that to have it merged quickly even if your proposal of new properties is finally not validated.

#4621

@lolodomo
Copy link
Contributor

Should be rebased now that #4621 is merged.

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
@jimtng
Copy link
Contributor Author

jimtng commented Feb 26, 2025

rebased

@lsiepel
Copy link
Contributor

lsiepel commented Feb 27, 2025

I've split them up into separate commits

Bit late, but I meant PR’s not commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants