apeitheo / aletheia

Command-line media player with speed controls and voice feedback
GNU General Public License v3.0
3 stars 1 forks source link

I suggest running shellcheck on the bash script aletheia #4

Closed DidierSpaier closed 1 year ago

DidierSpaier commented 1 year ago

Hello,

having seen the thread you opened about aletheia on linuxquestions.org, I downloaded the script and ran "shellcheck aletheia". I won't post the output here, but I suggest you do the same to figure out needed fixes.

Best regards, Didier

apeitheo commented 1 year ago

Didier,

Thank you for bringing this tool to my attention. I've been learning a lot as I've progressed the last two years, and this looks like it will be invaluable in helping not just debug, but also improve the code and make it more portable. I will be sure to use this and adjust the script accordingly.

Thanks again

apeitheo commented 1 year ago

Didier,

The last two commits removed 2/3 of the shellcheck warnings due to my use of deprecated backticks and the fact that I was unnecessarily referring to variable names with a $ prefix in arithmetic operations, which isn't required. I'll be checking the rest shortly.

Thanks again

apeitheo commented 1 year ago

The last commit a few minutes ago fixes all but a couple of warnings. I'll take a look at the remaining few soon.

I believe you're the creator of Slint, the Slackware distribution with accessibility features for the visually-impaired, no? I'd be interested in your opinions and suggestions on the voice feedback as well, as Aletheia should be usable without seeing the screen. Some controls, like those for adjusting pitch/tempo and volume controls, for example, don't have voice feedback, but those few are easy enough to learn and I left them without so it would be easier to hear the changes. I'll probably have different levels of feedback at some point, with different levels of verbosity.

DidierSpaier commented 1 year ago

Hello Brad,

you are welcome. TIP: if you replace the shebang by #!/bin/sh, shellcheck will list all bashisms at no additional cost ;)

This can be useful if you want the script to be more portable, using only syntactical constructs specified by POSIX, like in https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html

Keep up the good work!

Cheers, Didier

DidierSpaier commented 1 year ago

Hi again,

I believe you're the creator of Slint, the Slackware distribution with accessibility features for the visually-impaired, no? I'd be interested in your opinions and suggestions on the voice feedback as well, as Aletheia should be usable without seeing the screen. Some controls, like those for adjusting pitch/tempo and volume controls, for example, don't have voice feedback, but those few are easy enough to learn and I left them without so it would be easier to hear the changes. I'll probably have different levels of feedback at some point, with different levels of verbosity.

Yes I am the creator an maintainer of Slint. As blind users can give you better feedback than me, I will package this software next week for Slint and request feedback from users.

apeitheo commented 1 year ago

Didier,

I took a look at the output of shellcheck with #!/bin/sh, and it's not too bad (output's as long as it was this morning for just bash), but I've used a lot syntax that wouldn't be portable (e.g. it says echo in POSIX doesn't accept flags like -n, -e, apparently, so I'm assuming I'd need to ensure I handle all newlines instead of relying on the flag, etc, assuming echo doesn't output a newline by default). I will look into what needs to be done, but it likely won't be today yet.

Another thing I saw that would reduce the output of shellcheck significantly would just be a global string replace of == to = as it didn't like the ==. I believe it works either way in bash, so it's likely worth it to switch over. It was drilled into me years ago to avoid = for comparison (albeit for different languages), but portability is more important in the end.

Hopefully I can put some finishing touches on it this week. It already works great for me, but I imagine others will have ogg or wma files, etc, so I should add support for those. I was using it earlier with my screen off and noticed I should probably say something specific, like "main screen" in lieu of saying a generic "closed" when, e.g. the queue view is closed, so as to let you know where you're currently at, rather than the fact you just left somewhere. I thought about having the feedback for pitch delayed so that it would still tell you what's set, but not for every step. Lots of little things here and there.

I would definitely recommend any users using it without a screen to listen to the manual to give you a grasp on the basic key controls, modes, etc. I hope I was thorough enough with the manpage. Jumping right into it would be a little tough as it doesn't yet provide, for example, a guided voice tutorial and is meant to give an experienced user as little feedback as possible while still retaining all the necessary information used to operate it in the fullest, so as to not disturb the music, soundscape, or video as much.

Thanks again for the invaluable feedback.

apeitheo commented 1 year ago

Fixed the last of the shellcheck warnings with the last commit.

Thanks again