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 optimizations #2698

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

Conversation

s17534
Copy link

@s17534 s17534 commented Feb 3, 2023

Hello all, first of all, must say that motioneye is very advanced piece of software. Thanks to everyone involved to it.

As a user and beginning programmer wanted to share with everybody my code in which I changed the way TG notifications work. Let me say in a few words what's changed:

  1. No longer missing pictures from notifications
  2. Ability to send in one batch many pictures regarding one specific motion detection, user can specify how much they want in a GUI (max 10 per event)
  3. No need for searching picture files on disk, the list of them is gathered while current event is happening
  4. Slight changes to motioneye's web components in regards to some descriptions, js etc
  5. Changes are reflected in camera config, no need for user edit for TG to work
  6. Maybe more, but now I don't remember :)

Tested on two different motioneye's installs. Seems fine to me.

While working on the changes I found some bugs in motion and motioneye, but I probably should open issue as well.

…ble to precisely set number of images attached(max 10 in album instead of single and separated messages), if telegrams max pictures is specified as 0, text notifications only are applied, motioneye's config procedures, html, javascript and relayevent.sh files are updated to include mentioned changes
@MichaIng MichaIng added this to the v0.43.0 milestone Feb 3, 2023
@MichaIng MichaIng linked an issue Mar 2, 2023 that may be closed by this pull request
@s17534
Copy link
Author

s17534 commented Mar 4, 2023

It seams that nobody is interested, :sadFace

@MichaIng
Copy link
Member

MichaIng commented Mar 4, 2023

It is more that no one found time to really review and test the PR. What makes it difficult/raises efforts is that you did some major structural changes, so that the Telegram action now significantly differs form other actions, which may have implications that are hard to see now, but more importantly means that logic changes for all motionEye users, regardless whether Telegram is used or not. I think most of these changes are not needed for the actual purpose/issue you want to solve, and it would be much easier if you kept it hence minimal.

If there is a general problem the way how motion events/actions are handled right now, then we better think about generally changing this, instead of changing it for a single action and create inconsistency more difficulty for others to understand the general logic of the code.

@Nuke142
Copy link
Contributor

Nuke142 commented Apr 18, 2023

I want to test. Don't know how to. I'am using dev docker. Telegram notifications a little broken.
I use Attached Images Time=1; Capture mode=motion triggered (one); Motion Gap=1.

Image created when motion is started and telegram sends notification immediately, but without photo, because motion record still going. And this is a bug.
If I set Attached Images Time=60 and record lasts few seconds - no problem, telegram shows photo too.

Thus, the notification comes without a photo if recording continues at the moment when the notification should come. Although the image is created in the first second, before the notification is sent.

@s17534
Copy link
Author

s17534 commented Apr 27, 2023

I want to test. Don't know how to. I'am using dev docker. Telegram notifications a little broken. I use Attached Images Time=1; Capture mode=motion triggered (one); Motion Gap=1.

Image created when motion is started and telegram sends notification immediately, but without photo, because motion record still going. And this is a bug. If I set Attached Images Time=60 and record lasts few seconds - no problem, telegram shows photo too.

Thus, the notification comes without a photo if recording continues at the moment when the notification should come. Although the image is created in the first second, before the notification is sent.

Hi, it is a known limitation of current state of the send telegram part of the motioneye. That is why I made my own version. As you probably see in the above discussion, my pull request was denied and changes to the source code will not be merged into main branch.

So in a such situation, the only (known to me) way to try out my approach to telegram notifications is to clone my repo and continue from there. I know there could be trust issues here, you may see for yourself what has been changed in the source and if it is ok with you.

If after such deliberation you still want to try my proposal, feel free to drop me a message here. I will then assist you with said preparations.

@MichaIng
Copy link
Member

As you probably see in the above discussion, my pull request was denied

We'll, I did not deny it, nor am I the only person in charge 😉. But I would indeed prefer the changes to be more minimal/targeted to the actual aim. This would make it also easier (faster) to properly review and test.

Logan-Fouts
Logan-Fouts approved these changes May 14, 2023
Copy link

@Logan-Fouts Logan-Fouts left a comment

Choose a reason for hiding this comment

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

Worked on my instance but yes, Lots of changes happening all at once

Copy link
Collaborator

@zagrim zagrim left a comment

Choose a reason for hiding this comment

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

I tested this and took a quick peek at the code, too. It seems to work ok in the basic scenario at least (I didn't test any edge cases).

There are some things to improve, which I tried to point out in the specific code parts. And there's one more, as this doesn't migrate or clean up the old Telegram notification config, and so the user is left with an extra "Run A Command" notification which does nothing. It's easy for the user to clean that up (and it kinda makes it easy to copy-paste the token and chat-id from there to the new one if the user is tech-savvy), but I think we should deal with that automatically within ME, since it shouldn't be too hard. That said, I don't know if there's any suitable commonly used mechanism for migrating web hooks so far...

(edit: ditch comment about splitting functionality to separate files since that is not really the case here)

What I can't really judge is the inclusion of httpx. If it is really superior to the old ways (pycurl, or urllib + plenty of wrapper code) like it looks to me on the quick look, it might be a good addition and something to go for. But not being a Python dev I can't judge.

from datetime import datetime
from io import BytesIO

import httpx
Copy link
Collaborator

@zagrim zagrim Jul 3, 2023

Choose a reason for hiding this comment

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

It seems that having this added dependency needs to be reflected in setup.cfg under install_requires in order to pip install to download httpx and avoid having to do that manually.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like all the Telegram stuff is now here, so the original motioneye/sendtelegram.py should also be deleted as obsolete?

@@ -1515,17 +1515,14 @@ msgstr ""

#: motioneye/templates/main.html:1119
msgid "Alkroĉitaj Bildoj Tempo"
msgstr "Attached Images Time"
msgstr "Max images per event"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not quite sure why, but at least recompiling ME from this PR locally with python3 -m pip install . didn't update the text in the UI, not even with a hard-refresh in the browser. The same applies to the tooltip text below.

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

Successfully merging this pull request may close these issues.

Telegram motion notifications not working
5 participants