Adder timer function and option for filename #39

Dark-Kernel · 2024-07-13T11:48:42Z

Developed a small timer function, which will run after running the script to check the length during recording.
Added -o option which takes the filename as an argument, to save the file with the name you want
Included DIR entry in configuration to change the path, where the file will be saved.

…in configuration.

belden

Some questions and minor concerns about possible unintended changes.

belden · 2024-07-15T15:10:33Z

bin/ffscreencast

@@ -40,7 +40,7 @@ TIME=$(date +%H.%M.%S)
 NAME="Screencast ${DATE} at ${TIME}"

 # Where to save it
-DIR="${HOME}/Desktop"
+DIR="${HOME}/Videos/"


@Dark-Kernel Is this a mistaken commit? I imagine it might be left over from your development, since it's not called out in the PR description.

Sorry but, No it's not mistaken. Is there something wrong?

To be very clear: I think this is a good change to make, and might resolve #34 (or might not, perhaps that user doesn't keep a ~/Videos/ either).

This is a change in prior behavior from how the tool used to operate. It might be worth making a separate PR just for changing the default save directory, so @cytopia can either approve or reject it as they see fit, independent of the timer functionality your PR is adding.

Videos directory is most likely available on all Desktop Linux distributions, and if it's not there that means most probably that user will neither have Desktop directory. (Like me)
BTW thanks.

belden · 2024-07-15T15:13:06Z

bin/ffscreencast

+			echo 
+
+			echo "# Default Directory to save video files" 
+			echo "DIR=\"$HOME/Videos\"" 


Again I wonder whether this is an intended change?

Having DIR in configuration, one can easily change it to intended location.

Just to reply here as well, changing default behavior of tools can surprise users. Though I personally think ~/Videos/ is a more sensible default location than ~/Desktop/, I have also gotten used to the existing behavior of the tool.

I agree with you, so I think I should change it to ~/Desktop again.

Sorry for the inconvenience.

belden · 2024-07-15T16:28:45Z

bin/ffscreencast

+			echo "CHOSEN_A_NUM=\"\""
+			echo "CHOSEN_C_NUM=\"\""
+			echo 
+		} >> "${conf}"


@Dark-Kernel FYI there's a subtle change in behavior here. Previously passing through this code would do

echo "foo" > $conf # clobber

and then a series of

echo "bar" >> $conf # append new file

Changing this to

{ echo "foo" echo "bar" } >> $conf

means we no longer clobber the old config, instead we'll append a new config to it.

I think you can just use

{ echo "foo" echo "bar" } > $conf # note, just a single `>`

to achieve the prior behavior.

Yes, it's small mistake, i didn't noticed that one > is extra, sorry 😅.

@Dark-Kernel cool, it seems like an easy fix - but this PR doesn't contain it (yet - maybe you haven't had a chance to make it yet: totally understandable!)

I've opened Dark-Kernel#1 which targets your branch - if merged it will resolve this >> -> > issue.

belden · 2024-07-15T16:32:40Z

bin/ffscreencast

@@ -1053,7 +1093,7 @@ if [ "${RECORD_S}" = "yes" ]; then
 	if [ ${#CHOSEN_S_NUM} -gt 0 ]; then
 		if ! screen_device_exists "$CHOSEN_S_NUM"; then
 			echo "Screen recording device: '${CHOSEN_S_NUM}' does not exist."
-			exit -1
+			exit 1


@Dark-Kernel I'm curious about this change? Again, it's a slight change, but it's not commented in the PR so I'm uncertain as to the rationale.

$ bash -c 'exit -1' $ echo $? 255 $ bash -c 'exit 1' $ echo $? 1

what difference it makes?

Again, this is just me noticing an unacknowledged change in default behavior.

My interest in this project is to be able to easily record my screen on Linux. I use i3wm, which is a very customizable window manager.

Right now it is nice that ffscreencast has three different exit values:

0 -- no errors; my screen recording is where I want it to be

1 -- some sort of error occurred, probably while running

255 -- the script couldn't even start doing its work

Changing from exit -1 to exit 1 here means I get a little less information from the exit value of ffscreencast.

Cool! Learned something new. Will change it.

belden · 2024-07-15T16:34:15Z

bin/ffscreencast

+		-o*)
+			shift
+			NAME="$1" 
+            run_timer &


Nit, this line is indented with spaces rather than tabs. The file in general uses tabs.

This I saw in git diff, I always use tabs. I don't know how this occurred.

I've opened Dark-Kernel#1 which targets your branch - if merged, it will resolve this issue.

This preserves the prior behavior of the script. See cytopia#39 (comment)

No logic change * delete trailing spaces * swap a space-indent to a hard-tab indent

bin/ffscreencast

Tidy whitespace and clobber config

…r -o option

Dark-Kernel · 2024-10-11T13:54:57Z

@belden
I request you to complete this please.

belden · 2024-10-17T23:05:12Z

@Dark-Kernel I wish that I could! I am not a contributor on this project - just another programmer out in the wild who also has some open PRs for this project.

I've tried to help this PR along to make sure there aren't any broken backwards compatibility issues, but it's up to @cytopia to ultimately review and approve/reject this work.

@cytopia can you have a look at this PR please?

Dark-Kernel added 2 commits July 13, 2024 16:44

Developed -o option for output filename & included DIR for file path …

03bdd0a

…in configuration.

Added timer to recording.

94941f0

belden reviewed Jul 15, 2024

View reviewed changes

belden added 2 commits July 15, 2024 15:15

Ensure writing config clobbers old config

d7a75f7

This preserves the prior behavior of the script. See cytopia#39 (comment)

Whitespace cleanup

667c064

No logic change * delete trailing spaces * swap a space-indent to a hard-tab indent

belden mentioned this pull request Jul 15, 2024

Tidy whitespace and clobber config Dark-Kernel/ffscreencast#1

Merged

belden reviewed Jul 15, 2024

View reviewed changes

bin/ffscreencast Outdated Show resolved Hide resolved

Dark-Kernel added 2 commits July 16, 2024 08:46

Merge pull request #1 from belden/pr-tidy

1c4f76d

Tidy whitespace and clobber config

changed exit status back to -1, Default dir o desktop. Added usage fo…

78bf462

…r -o option

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adder timer function and option for filename #39

Adder timer function and option for filename #39

Dark-Kernel commented Jul 13, 2024

belden left a comment

belden Jul 15, 2024

Dark-Kernel Jul 15, 2024

belden Jul 15, 2024

Dark-Kernel Jul 16, 2024

belden Jul 15, 2024

Dark-Kernel Jul 15, 2024

belden Jul 15, 2024

Dark-Kernel Jul 16, 2024

belden Jul 15, 2024

Dark-Kernel Jul 15, 2024

belden Jul 15, 2024

belden Jul 15, 2024

belden Jul 15, 2024

Dark-Kernel Jul 15, 2024

belden Jul 15, 2024

Dark-Kernel Jul 16, 2024

belden Jul 15, 2024

Dark-Kernel Jul 15, 2024

belden Jul 15, 2024

Dark-Kernel Jul 16, 2024

Dark-Kernel commented Oct 11, 2024

belden commented Oct 17, 2024

Adder timer function and option for filename #39

Are you sure you want to change the base?

Adder timer function and option for filename #39

Conversation

Dark-Kernel commented Jul 13, 2024

belden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dark-Kernel commented Oct 11, 2024

belden commented Oct 17, 2024