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

Generate notification images #876

Merged
merged 8 commits into from
Jan 18, 2022
Merged

Conversation

EricClaeys
Copy link
Collaborator

This PR is a replacement to Chris' #569 that creates a very similar tool. I don't have the ability to issue PRs against his version so I created a new one.
This version allows other scripts to call it to create custom on-the-fly notifications - a future version of allsky.sh and the "capture" programs will likely use that capability.
This version also adds a "Restarting" image for use when allsky.sh restarts (i.e., stops then starts).

This replaces the version of "generate_images.sh" that Chris created a few months ago, and to which I don't have the ability to update.
This version allows other scripts to create custom images by passing arguments to generate_notification_images.sh.
It also adds a "Restarting" image for when allsky.sh restarts (i.e., stops, then starts).  In those cases there's no need to display a "stopping" then "starting" image - just display a single "restarting".
Also added ability to have an image border.
Copy link
Collaborator

@AndreasLMeg AndreasLMeg left a comment

Choose a reason for hiding this comment

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

I'm not sur if this is my problem:

  1. file is not executable
    -rw-r--r-- 1 pi pi 5925 Dez 29 09:01 generate_notification_images.sh

  2. Problems with CR/LF

$ ./generate_notification_images.sh
-bash: ./generate_notification_images.sh: /bin/bash^M: Defekter Interpreter: Datei oder Verzeichnis nicht gefunden
$ file generate_notification_images.sh
generate_notification_images.sh: Bourne-Again shell script, ASCII text executable, with CRLF line terminators
$ dos2unix generate_notification_images.sh
dos2unix: Datei generate_notification_images.sh wird ins Unix-Format umgewandelt …
$ file generate_notification_images.sh
generate_notification_images.sh: Bourne-Again shell script, ASCII text executable
  1. font and color changed and text is not so clear/sharp - but i am color blind ;-)
    grafik

  2. I think we should not mix files/directories. scripts for srcipts and notification_image should be the output_dir (no sh or make files included) maybe helpful for packages,.... btw. the same situation/problem we have with source -> all executeables should be in bin folder

@EricClaeys
Copy link
Collaborator Author

EricClaeys commented Dec 30, 2021

@AndreasLMeg Thanks for the feedback.

  1. I use the web-based Git tool and there's no way to do a "chmod" of a file. If you use the PC or Pi-based version of Git, can you do a commit to do a chmod? But wait until I do # 4...
  2. How did you download the file? On my system it's a UNIX file with just a LF, and I pasted it into the Git page and there were no changes.
$ file generate_notification_images.sh
generate_notification_images.sh: Bourne-Again shell script, ASCII text executable
  1. Thanks for checking that. Upwards of 10% of the population is color blind so we need to make sure our images work for everyone. Which version is hard to read, the darker background on the left, or the lighter background on the right? The script currently makes the darker-background version, but what's in Git is the lighter background. Can I ask a favor? Can you try some combinations and see what has good contrast. I only picked yellow for a text color since it matches the Sun, but it's really not important.
  2. I agree, and originally intended the .sh file to go in "scripts", but in Git I was accidently in the wrong directory when I uploaded the file. I will delete from "notification_images" and upload to "scripts".

Moving to allsky/scripts.
Actually, moved from allsky/notification_images to allsky/scripts.
@AndreasLMeg
Copy link
Collaborator

2. How did you download the file? On my system it's a UNIX file with just a LF, and I pasted it into the Git page and there were no changes.

on my pi (ssh connection with putty):

git clone [email protected]:thomasjacquin/allsky.git test_PR
git pull origin pull/872/head

Happened for the first time since this clone: ​​https://github.com/Alex-developer/allskyannotate.git
But Alex cannot understand the behavior in himself either.
I would be interested in whether this is also the case with others - if so, we get big problems with customer installations

3. Thanks for checking that. Upwards of 10% of the population is color blind so we need to make sure our images work for everyone. Which version is hard to read, the darker background on the left, or the lighter background on the right? The script currently makes the darker-background version, but what's in Git is the lighter background. Can I ask a favor? Can you try some combinations and see what has good contrast. I only picked yellow for a text color since it matches the Sun, but it's really not important.

that's a matter of taste. Possibly one should also consider the "dark mode"
I will look at the pictures again and give my comments. But that shouldn't stop the PR.

Andreas

@AndreasLMeg
Copy link
Collaborator

  1. I use the web-based Git tool and there's no way to do a "chmod" of a file. If you use the PC or Pi-based version of Git, can you do a commit to do a chmod? But wait until I do # 4...

I don't know how it works with someone else's PR. Can you give me some advice?

@AndreasLMeg
Copy link
Collaborator

pi@allsky:~/test_PR_872/scripts $ ls -la
total 96
drwxr-xr-x 2 pi pi 4096 Dec 30 09:37 .
drwxr-xr-x 9 pi pi 4096 Dec 30 09:35 ..
-rwxr-xr-x 1 pi pi 2603 Dec 30 09:35 allsky_mfs.sh
-rwxr-xr-x 1 pi pi 3309 Dec 30 09:35 copy_notification_image.sh
-rwxr-xr-x 1 pi pi 2155 Dec 30 09:35 darkCapture.sh
-rwxr-xr-x 1 pi pi 3665 Dec 30 09:35 darkSubtract.sh
-rwxr-xr-x 1 pi pi  555 Dec 30 09:35 endOfNight_additionalSteps.repo
-rwxr-xr-x 1 pi pi 3056 Dec 30 09:35 endOfNight.sh
-rwxr-xr-x 1 pi pi  173 Dec 30 09:35 filename.sh
-rwxr-xr-x 1 pi pi 7166 Dec 30 09:35 generateForDay.sh
-rw-r--r-- 1 pi pi 5783 Dec 30 09:37 generate_notification_images.sh
-rw-r--r-- 1 pi pi  925 Dec 30 09:35 Makefile
-rwxr-xr-x 1 pi pi  960 Dec 30 09:35 postData.sh
-rwxr-xr-x 1 pi pi 6099 Dec 30 09:35 removeBadImages.sh
-rwxr-xr-x 1 pi pi 3123 Dec 30 09:35 saveImageDay.sh
-rwxr-xr-x 1 pi pi 3923 Dec 30 09:35 saveImageNight.sh
-rwxr-xr-x 1 pi pi 4975 Dec 30 09:35 timelapse.sh
-rwxr-xr-x 1 pi pi  507 Dec 30 09:35 uploadForDay.sh
-rwxr-xr-x 1 pi pi 5765 Dec 30 09:35 upload.sh
pi@allsky:~/test_PR_872/scripts $ file generate_notification_images.sh
generate_notification_images.sh: Bourne-Again shell script, ASCII text executable
pi@allsky:~/test_PR_872/scripts $ chmod +x generate_notification_images.sh
pi@allsky:~/test_PR_872/scripts $ file generate_notification_images.sh
generate_notification_images.sh: Bourne-Again shell script, ASCII text executable
pi@allsky:~/test_PR_872/scripts $ ls -la
total 96
drwxr-xr-x 2 pi pi 4096 Dec 30 09:37 .
drwxr-xr-x 9 pi pi 4096 Dec 30 09:35 ..
-rwxr-xr-x 1 pi pi 2603 Dec 30 09:35 allsky_mfs.sh
-rwxr-xr-x 1 pi pi 3309 Dec 30 09:35 copy_notification_image.sh
-rwxr-xr-x 1 pi pi 2155 Dec 30 09:35 darkCapture.sh
-rwxr-xr-x 1 pi pi 3665 Dec 30 09:35 darkSubtract.sh
-rwxr-xr-x 1 pi pi  555 Dec 30 09:35 endOfNight_additionalSteps.repo
-rwxr-xr-x 1 pi pi 3056 Dec 30 09:35 endOfNight.sh
-rwxr-xr-x 1 pi pi  173 Dec 30 09:35 filename.sh
-rwxr-xr-x 1 pi pi 7166 Dec 30 09:35 generateForDay.sh
-rwxr-xr-x 1 pi pi 5783 Dec 30 09:37 generate_notification_images.sh
-rw-r--r-- 1 pi pi  925 Dec 30 09:35 Makefile
-rwxr-xr-x 1 pi pi  960 Dec 30 09:35 postData.sh
-rwxr-xr-x 1 pi pi 6099 Dec 30 09:35 removeBadImages.sh
-rwxr-xr-x 1 pi pi 3123 Dec 30 09:35 saveImageDay.sh
-rwxr-xr-x 1 pi pi 3923 Dec 30 09:35 saveImageNight.sh
-rwxr-xr-x 1 pi pi 4975 Dec 30 09:35 timelapse.sh
-rwxr-xr-x 1 pi pi  507 Dec 30 09:35 uploadForDay.sh
-rwxr-xr-x 1 pi pi 5765 Dec 30 09:35 upload.sh

@AndreasLMeg
Copy link
Collaborator

on my second pi - after clone and checkout:

pi@allsky:~/test_PR_876/scripts $ file generate_notification_images.sh
generate_notification_images.sh: Bourne-Again shell script, ASCII text executable, with CRLF line terminators

@EricClaeys
Copy link
Collaborator Author

@AndreasLMeg What does "file" say if you grab a different .sh file? Try renaming filename.sh and grab it. I'm curious if you're getting CRLF for all files, or just generate_notification_images.sh.
The scripts/Makefile does a "chmod 775 *.sh" so

I just noticed you did a commit for a "chmod"? How did you do that?

@AndreasLMeg
Copy link
Collaborator

maybe git config "core.autocrlf=true" is the reason, But i'm not sure yet.

pi@allsky:~/test_PR_872/scripts $ git config -l
core.repositoryformatversion=0
core.filemode=true
core.bare=false
core.logallrefupdates=true
[email protected]:thomasjacquin/allsky.git
remote.origin.fetch=+refs/heads/*:refs/remotes/origin/*
branch.master.remote=origin
branch.master.merge=refs/heads/master
branch.generate_notification_images.remote=origin
branch.generate_notification_images.merge=refs/heads/generate_notification_images
pi@allsky:~/test_PR_876/scripts $ git config -l
core.filemode=false
core.autocrlf=true
core.repositoryformatversion=0
core.filemode=true
core.bare=false
core.logallrefupdates=true
[email protected]:thomasjacquin/allsky.git
remote.origin.fetch=+refs/heads/*:refs/remotes/origin/*
branch.master.remote=origin
branch.master.merge=refs/heads/master
branch.generate_notification_images.remote=origin
branch.generate_notification_images.merge=refs/heads/generate_notification_images

@AndreasLMeg
Copy link
Collaborator

FYI: https://docs.github.com/en/get-started/getting-started-with-git/configuring-git-to-handle-line-endings

After "git config --global core.autocrlf input" and fresh clone/checkout CR/LF problem is gone....


pi@allsky:~/test_PR_876/scripts $ git config -l
core.filemode=false
core.autocrlf=input
core.repositoryformatversion=0
core.filemode=true
core.bare=false
core.logallrefupdates=true
[email protected]:thomasjacquin/allsky.git
remote.origin.fetch=+refs/heads/*:refs/remotes/origin/*
branch.master.remote=origin
branch.master.merge=refs/heads/master
branch.generate_notification_images.remote=origin
branch.generate_notification_images.merge=refs/heads/generate_notification_images

@AndreasLMeg
Copy link
Collaborator

@AndreasLMeg What does "file" say if you grab a different .sh file? Try renaming filename.sh and grab it. I'm curious if you're getting CRLF for all files, or just generate_notification_images.sh. The scripts/Makefile does a "chmod 775 *.sh" so

I just noticed you did a commit for a "chmod"? How did you do that?

history:

  357  git clone [email protected]:thomasjacquin/allsky.git test_PR_872
  358  cd test_PR_872/
  359  git branch --all
  360  git checkout generate_notification_images
  361  ls -la
  362  cd scripts/
  363  ls -la
  364  file generate_notification_images.sh
  365  chmod +x generate_notification_images.sh
  366  file generate_notification_images.sh
  367  ls -la
  368  git status
  369  git add generate_notification_images.sh
  370  git status
  371  git commit -m "chmod"
  372  git status
  373  git push

Copy link
Collaborator

@AndreasLMeg AndreasLMeg left a comment

Choose a reason for hiding this comment

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

now the new images are saved in srcipts directory

@EricClaeys
Copy link
Collaborator Author

@AndreasLMeg What do you mean by "the new images are saved in the scripts directory"?

The generate_notification_images.sh script saves the files to whatever directory you are in when you run it.
We'll probably stop distributing the notification images and instead have them created at installation time by changing the scripts/Makefile command to run the script and save the output to the allsky/notification_images directory.

@AndreasLMeg
Copy link
Collaborator

@AndreasLMeg What do you mean by "the new images are saved in the scripts directory"?

The generate_notification_images.sh script saves the files to whatever directory you are in when you run it. We'll probably stop distributing the notification images and instead have them created at installation time by changing the scripts/Makefile command to run the script and save the output to the allsky/notification_images directory.

@EricClaeys: ok, then you should revert my commit, or change only the part with the relative path

The invoker (probably a Makefile) will ensure it's in the $ALLSKY_NOTIFICATION_IMAGES when it invokes this script.
Reset the formatting of the table - it's too hard to read when it's all on one line.
@EricClaeys EricClaeys requested a review from AndreasLMeg January 5, 2022 10:47
Copy link
Collaborator

@AndreasLMeg AndreasLMeg left a comment

Choose a reason for hiding this comment

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

Please check my commit: commit 7745588

scripts/generate_notification_images.sh Outdated Show resolved Hide resolved
AndreasLMeg
AndreasLMeg previously approved these changes Jan 5, 2022
@AndreasLMeg AndreasLMeg dismissed their stale review January 5, 2022 14:01

not tested the changes, maybe tonight

@EricClaeys EricClaeys requested a review from AndreasLMeg January 5, 2022 20:55
Put the table entries on one line, with each field separated by spaces, so it's easy to read regardless of the user's tab stop settings.
@AndreasLMeg
Copy link
Collaborator

@EricClaeys: The next days I have no time to make tests, but I think ist's ok to merge.

@linuxkidd linuxkidd merged commit d2b1f7c into master Jan 18, 2022
@EricClaeys EricClaeys deleted the generate_notification_images branch January 21, 2022 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants