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

Refactor ThingHandlerService to an OSGi component prototype #3957

Merged
merged 16 commits into from
Jan 2, 2024

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented Dec 24, 2023

Supersedes #1941

Since Connor is no longer available I decided to pick up the PR and enhance it. A very good example why we should go this way is openhab/openhab-addons#16037. It implements a TheKeyTranslationProvider.java just to pass it from the handler factory to the thing handler and then to the ThinghandlerService for localization of the discovery results. There is already similar code in the repository. This is only necessary because we can't inject OSGi services directly to the discovery services.

I started with two implementations in openhab-addons (openhab/openhab-addons#16107) and would like to hear comments from @openhab/core-maintainers before I start a bigger refactoring in openhab-addons.

@J-N-K J-N-K added enhancement An enhancement or new feature of the Core API breaking labels Dec 24, 2023
@J-N-K J-N-K requested a review from a team as a code owner December 24, 2023 14:15
@J-N-K J-N-K changed the title Thinghandlerservice osgi Refactor ThingHandlerService to an OSGi component prototype Dec 24, 2023
@wborn
Copy link
Member

wborn commented Dec 28, 2023

Thanks for picking it up! It looks good to me so far. 👍
Would be nice to get rid of all the workarounds being used for this in the add-ons code. 🙂

Connor Petty and others added 5 commits December 28, 2023 17:34
Signed-off-by: Connor Petty <mistercpp2000+gitsignoff@gmail.com>
Signed-off-by: Connor Petty <mistercpp2000+gitsignoff@gmail.com>
Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Jan N. Klug <github@klug.nrw>
@J-N-K
Copy link
Member Author

J-N-K commented Dec 31, 2023

@wborn @kaikreuzer I would like to see this merged "soon", because the corresponding PR in openhab-addons will create a lot of merge conflicts each time something is changed in a discovery service. I'll now create a PR for the documentation how to do thing discovery right.

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up! Lgtm as far as I can tell.
I leave it to @wborn to have another look and merge it, if he is happy with it.

Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

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

LGTM except for these few minor comments:

J-N-K and others added 8 commits January 2, 2024 12:20
…enhab/core/config/discovery/AbstractThingHandlerDiscoveryService.java

Co-authored-by: Wouter Born <github@maindrain.net>
Signed-off-by: J-N-K <github@klug.nrw>
…enhab/core/config/discovery/AbstractThingHandlerDiscoveryService.java

Co-authored-by: Wouter Born <github@maindrain.net>
Signed-off-by: J-N-K <github@klug.nrw>
…enhab/core/config/discovery/AbstractThingHandlerDiscoveryService.java

Co-authored-by: Wouter Born <github@maindrain.net>
Signed-off-by: J-N-K <github@klug.nrw>
…enhab/core/config/discovery/AbstractThingHandlerDiscoveryService.java

Co-authored-by: Wouter Born <github@maindrain.net>
Signed-off-by: J-N-K <github@klug.nrw>
…enhab/core/config/discovery/AbstractThingHandlerDiscoveryService.java

Co-authored-by: Wouter Born <github@maindrain.net>
Signed-off-by: J-N-K <github@klug.nrw>
…enhab/core/config/discovery/AbstractThingHandlerDiscoveryService.java

Co-authored-by: Wouter Born <github@maindrain.net>
Signed-off-by: J-N-K <github@klug.nrw>
…thing/binding/BaseThingHandlerFactory.java

Co-authored-by: Wouter Born <github@maindrain.net>
Signed-off-by: J-N-K <github@klug.nrw>
Signed-off-by: Jan N. Klug <github@klug.nrw>
@J-N-K
Copy link
Member Author

J-N-K commented Jan 2, 2024

Done

Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

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

Thank you!

@wborn wborn merged commit a5316f9 into openhab:main Jan 2, 2024
3 checks passed
@wborn wborn added this to the 4.2 milestone Jan 2, 2024
wborn pushed a commit to openhab/openhab-addons that referenced this pull request Jan 3, 2024
Related to: openhab/openhab-core#3957

Signed-off-by: Jan N. Klug <github@klug.nrw>
@lolodomo
Copy link
Contributor

lolodomo commented Jan 3, 2024

Thank you @J-N-K , I will adjust the openweathermap binding to take benefit of this new feature.

Just one question: what will happen for non official bindings already using ThingHandlerService, will they be broken ?

@J-N-K J-N-K deleted the thinghandlerservice-osgi branch January 3, 2024 12:51
@J-N-K
Copy link
Member Author

J-N-K commented Jan 3, 2024

They need to be adjusted accordingly.

@lolodomo
Copy link
Contributor

lolodomo commented Jan 3, 2024

They need to be adjusted accordingly.

So something to clearly announce when releasing version 4.2...

Cybso pushed a commit to Cybso/openhab-addons that referenced this pull request Jan 5, 2024
Related to: openhab/openhab-core#3957

Signed-off-by: Jan N. Klug <github@klug.nrw>
wborn added a commit to wborn/openhab-core that referenced this pull request Jan 5, 2024
Caused by openhab#3957

Signed-off-by: Wouter Born <github@maindrain.net>
@wborn wborn mentioned this pull request Jan 5, 2024
J-N-K pushed a commit that referenced this pull request Jan 5, 2024
Caused by #3957

Signed-off-by: Wouter Born <github@maindrain.net>
cipianpascu pushed a commit to cipianpascu/openhab-core that referenced this pull request Jan 17, 2024
…3957)

Also-by: Connor Petty <mistercpp2000+gitsignoff@gmail.com>
Signed-off-by: J-N-K <github@klug.nrw>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
cipianpascu pushed a commit to cipianpascu/openhab-core that referenced this pull request Jan 17, 2024
Caused by openhab#3957

Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
austvik pushed a commit to austvik/openhab-addons that referenced this pull request Mar 27, 2024
Related to: openhab/openhab-core#3957

Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
joni1993 pushed a commit to joni1993/openhab-addons that referenced this pull request Oct 15, 2024
Related to: openhab/openhab-core#3957

Signed-off-by: Jan N. Klug <github@klug.nrw>
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