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

[lgwebos] Thing actions still working after a bundle restart #8114

Merged
merged 3 commits into from
Jul 20, 2020

Conversation

lolodomo
Copy link
Contributor

Related to openhab/openhab-core#1265

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

@lolodomo lolodomo requested a review from sprehn as a code owner July 13, 2020 11:18
@lolodomo lolodomo added the work in progress A PR that is not yet ready to be merged label Jul 13, 2020
Related to openhab/openhab-core#1265

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo lolodomo force-pushed the lgwebos_thingactionsrestart branch from aca8e6f to 4effec8 Compare July 13, 2020 13:15
@lolodomo lolodomo removed the work in progress A PR that is not yet ready to be merged label Jul 13, 2020
@TravisBuddy
Copy link

Travis tests have failed

Hey @lolodomo,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

@openhab openhab deleted a comment from TravisBuddy Jul 13, 2020
@lolodomo
Copy link
Contributor Author

Related to #8116


public void launchBrowser(String url);

public List<Application> getApplications();
Copy link
Contributor

@cpmeister cpmeister Jul 13, 2020

Choose a reason for hiding this comment

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

I don't think the bugfix would work here. The proxy workaround would only work for methods that have parameters and return types that are loaded in a 3rd untouched classloader (e.g. parent or core). The Application class is loaded as part of the bundle classloader so the bug would still be present when LGWebOSActions attempts to use Application instances returned from getApplications().

One thing to note is that getApplications isn't annotated with @RuleAction which would mean the getApplications static method doesn't have a corresponding RuleAction. I'm not sure why this would be the case.
@sprehn can you give an explanation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, even if we could still have a problem with getApplications, at least it will be ok for all other actions.
As we are on a temporary workaround, I propose to accept this limit and mention it with a comment in the code.
Is it ok for you ?

For the missing @RuleAction , good catch, I hope @sprehn is still available to answer this question.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment added for getApplications

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, this method's intend is to allow users to find out what the Ids of applications are. It is not designed to work with the new rule engine. I think it is now safe to remove it all together from the action, as the binding can now be queried for the same information via karaf console.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, action removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

super, pls do me the favor and also delete the section in the readme file.
https://www.openhab.org/addons/bindings/lgwebos/#list-getapplications

Copy link
Contributor

Choose a reason for hiding this comment

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

since this is merged already, see #8161

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@TravisBuddy
Copy link

Travis tests have failed

Hey @lolodomo,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

Copy link
Contributor

@sprehn sprehn left a comment

Choose a reason for hiding this comment

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

happy with those changes. we could remove the getApplications method from the Action and from the readme file.


public void launchBrowser(String url);

public List<Application> getApplications();
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, this method's intend is to allow users to find out what the Ids of applications are. It is not designed to work with the new rule engine. I think it is now safe to remove it all together from the action, as the binding can now be queried for the same information via karaf console.

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@TravisBuddy
Copy link

Travis tests have failed

Hey @lolodomo,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

@J-N-K J-N-K dismissed cpmeister’s stale review July 20, 2020 17:34

comment addressed, action removed

@J-N-K J-N-K merged commit b74d438 into openhab:2.5.x Jul 20, 2020
@lolodomo lolodomo deleted the lgwebos_thingactionsrestart branch July 20, 2020 17:37
CSchlipp pushed a commit to CSchlipp/openhab-addons that referenced this pull request Jul 26, 2020
…#8114)

* [lgwebos] Thing actions still working after a bundle restart
* Action getApplications removed

Related to openhab/openhab-core#1265

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Signed-off-by: CSchlipp <christian@schlipp.de>
MPH80 pushed a commit to MPH80/openhab-addons that referenced this pull request Aug 3, 2020
…#8114)

* [lgwebos] Thing actions still working after a bundle restart
* Action getApplications removed

Related to openhab/openhab-core#1265

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Signed-off-by: MPH80 <michael@hazelden.me>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
…#8114)

* [lgwebos] Thing actions still working after a bundle restart
* Action getApplications removed

Related to openhab/openhab-core#1265

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
…#8114)

* [lgwebos] Thing actions still working after a bundle restart
* Action getApplications removed

Related to openhab/openhab-core#1265

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
…#8114)

* [lgwebos] Thing actions still working after a bundle restart
* Action getApplications removed

Related to openhab/openhab-core#1265

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
…#8114)

* [lgwebos] Thing actions still working after a bundle restart
* Action getApplications removed

Related to openhab/openhab-core#1265

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
DaanMeijer pushed a commit to DaanMeijer/openhab-addons that referenced this pull request Sep 1, 2020
…#8114)

* [lgwebos] Thing actions still working after a bundle restart
* Action getApplications removed

Related to openhab/openhab-core#1265

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Signed-off-by: Daan Meijer <daan@studioseptember.nl>
markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Sep 19, 2020
…#8114)

* [lgwebos] Thing actions still working after a bundle restart
* Action getApplications removed

Related to openhab/openhab-core#1265

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
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.

5 participants