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

[telegram] Telegram Binding initial contribution #5677

Merged
merged 52 commits into from Nov 7, 2019
Merged

[telegram] Telegram Binding initial contribution #5677

merged 52 commits into from Nov 7, 2019

Conversation

ZzetT
Copy link

@ZzetT ZzetT commented Jun 2, 2019

This binding shall work as a replacement for the old OH1 Telegram Actions.
It supports not only sending messages, but also receiving them. The base for the implementation is the TelegramBot library.

User Belgadon (Jens) initially started the work on that binding which still used the old OH1 telegram actions for sending messages. I removed this dependency by using the new OH2 actions and "tried" to migrate it to the new build environement.

The development of that binding was discussed in that thread.

Although I still didn't completly manage to get it running with the latest build environemnt, I was told in the forum to create a PR anyway.
The problems that I still have are either OSGI errors when I click "Resolve" in the bnd demo app or
when I add:

  <properties>
    <bnd.importpackage>org.telegram.telegrambots.*;resolution:="optional"</bnd.importpackage>
  </properties>  

then I get ClassNotFoundExceptions at runtime after adding a new Telegram thing.
One more thing to note here, IMHO it should not be necessary to also add a dependency to "telegrambots-meta" in the pom.xml (only telegrambots should be required), but if I remove that I get an error that this is missing.

Please note that the binding is using an apache client instead of a jetty client, but this issue was discussed already and agreed by @davidgraeff .

So, before continuing the work on more features, we need the approval that the library can be integrated into the OH2 environment and what needs to be configured to get it running again. Then, I hope that we can add a few more features in separate PRs for this binding.
Especially two things are currently missing compared to the old Telegram actions:

  • sending photos located on a webserver that requires username/password
  • support of html markup (should be easy to realize)

Currently, a beta version of that binding was released to several testers in the forum which however was only based on the old build environement.

Signed-off-by: Alexander Krasnogolowy alexkrasno@hotmail.com

@ZzetT ZzetT requested a review from a team as a code owner June 2, 2019 16:48
@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/announcing-some-improvements-to-the-homekit-plugin-for-openhab2/62729/117

Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

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

The problem with the bnd resolver may be that the libraries are not OSGi-libraries and require wrapping. I think that BND cannot do that on the fly. #5503 will help with this. This will work in KARAF because wrapping (as requested on the feature.xml) is available.

Adding optional to bnd.importpackage is wrong. You need these classes, they are not optional but required. As a workaround you can add the libraries to the /lib directory. But please do not commit these files.

The library is MIT licensed, I do not see any problem in using it. I didn't check why you use apache http client, can you add a note to this PR? It's easier to find in case someone wants to look it up.

bundles/org.openhab.binding.telegram/NOTICE Show resolved Hide resolved
bom/openhab-addons/pom.xml Outdated Show resolved Hide resolved
@ZzetT
Copy link
Author

ZzetT commented Jun 2, 2019

The library is MIT licensed, I do not see any problem in using it. I didn't check why you use apache http client, can you add a note to this PR? It's easier to find in case someone wants to look it up.

I'm not using the apache http client, but the Telegram library does. I tried to rewrite it, but there were a lot of changes necessary and it would be a big effort to keep it up to date with the original library.

@J-N-K
Copy link
Member

J-N-K commented Jun 2, 2019

Ok with me as well. Don‘t rewrite external libs.

@ZzetT
Copy link
Author

ZzetT commented Jun 3, 2019

Adding optional to bnd.importpackage is wrong. You need these classes, they are not optional but required. As a workaround you can add the libraries to the /lib directory. But please do not commit these files.

But then do I need this bnd importpackage at all? I found only very few bindings that have this. And some others also use an external library but don't have this bnd statement.

Sorry, I accidentally removed it now. Shall I just remove the optional attribute?

@J-N-K
Copy link
Member

J-N-K commented Jun 3, 2019

This is only for very special cases with embedded libraries. You don‘t need it.

@J-N-K J-N-K added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jun 3, 2019
@wborn wborn added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jun 3, 2019
@wborn wborn changed the title [telegram] Initial Contribution [telegram] Telegram Binding initial contribution Jun 3, 2019
@wborn wborn added the new binding If someone has started to work on a binding. For a new binding PR. label Jun 3, 2019
@J-N-K
Copy link
Member

J-N-K commented Jun 6, 2019

Is there anything missing from me here?

@kaikreuzer kaikreuzer added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jun 7, 2019
@kaikreuzer
Copy link
Member

Is there anything missing from me here?

@J-N-K There is still a "changes requested" review from you here - so yes, if you are satisfied, you should change it to an approval.

@J-N-K
Copy link
Member

J-N-K commented Jun 8, 2019

Mhm. Is there any better way to remove "requests changes"? I only looked at the infrastructure part, not at the code.

@kaikreuzer
Copy link
Member

There is usually also a "dismiss review" button as an option.

@TravisBuddy
Copy link

Travis tests were successful

Hey @ZzetT,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@clinique
Copy link
Contributor

Why is lastMessageDate a String channel ?

- converted lastMessageDate channel type from String to DateItem
- exceptions are now reported to the logger as a warning
- in case of a 401 error during getUpdates the thing will go offline because it usually means that the thing configuration (botToken) is wrong
- fixed mixed tabs and spaces

Signed-off-by: Alexander Krasnogolowy <alexkrasno@hotmail.com>
@TravisBuddy
Copy link

Travis tests were successful

Hey @ZzetT,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@ZzetT
Copy link
Author

ZzetT commented Nov 5, 2019

Why is lastMessageDate a String channel ?

Thanks for the hint, I changed that now.

From my side I would be ready to merge it to the master unless you guys want to have another look. Since the last review a few things have changed (library replacement).

- removed gson as a dependency/feature.xml
- removed dead code comment
- fixed thing labels
- code style

Signed-off-by: Alexander Krasnogolowy <alexkrasno@hotmail.com>
@TravisBuddy
Copy link

Travis tests were successful

Hey @ZzetT,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Copy link
Member

@Hilbrand Hilbrand left a comment

Choose a reason for hiding this comment

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

LGTM; @J-N-K You approved some time ago. If you're ok we can merge this binding.

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.

We already have 3 approvals and there was still a pending one from me - so let's get it merged! Thanks to everyone involved!!!

@kaikreuzer kaikreuzer merged commit 8b3922b into openhab:master Nov 7, 2019
thewiep pushed a commit to thewiep/openhab-addons that referenced this pull request Nov 23, 2019
Signed-off-by: Alexander Krasnogolowy <alexkrasno@hotmail.com>
roieg pushed a commit to roieg/openhab-addons that referenced this pull request Nov 28, 2019
Signed-off-by: Alexander Krasnogolowy <alexkrasno@hotmail.com>
@wborn wborn added this to the 2.5 milestone Dec 7, 2019
Pshatsillo pushed a commit to Pshatsillo/openhab-addons that referenced this pull request Dec 8, 2019
Signed-off-by: Alexander Krasnogolowy <alexkrasno@hotmail.com>
roieg pushed a commit to roieg/openhab-addons that referenced this pull request Dec 25, 2019
Signed-off-by: Alexander Krasnogolowy <alexkrasno@hotmail.com>
roieg pushed a commit to roieg/openhab-addons that referenced this pull request Dec 26, 2019
Signed-off-by: Alexander Krasnogolowy <alexkrasno@hotmail.com>
tmrobert8 pushed a commit to tmrobert8/openhab-addons that referenced this pull request Jan 21, 2020
Signed-off-by: Alexander Krasnogolowy <alexkrasno@hotmail.com>
Signed-off-by: Tim Roberts <timmarkroberts@gmail.com>
hww3 pushed a commit to hww3/openhab2-addons that referenced this pull request Feb 22, 2020
Signed-off-by: Alexander Krasnogolowy <alexkrasno@hotmail.com>
Hans-Reiner pushed a commit to Hans-Reiner/openhab2-addons that referenced this pull request Apr 11, 2020
Signed-off-by: Alexander Krasnogolowy <alexkrasno@hotmail.com>
Signed-off-by: Hans-Reiner Hoffmann <hans-reiner.hoffmann@gmx.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new binding If someone has started to work on a binding. For a new binding PR. oh1 migration Relates to migrating an openHAB 1 addon to openHAB 2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet