search

asterisk / asterisk

The official Asterisk Project repository.
https://www.asterisk.org
Other
2k stars 929 forks source link

app_record: Add RECORD_TIME output variable. #549

Closed InterLinked1 closed 1 month ago

InterLinked1 commented 4 months ago

This adds the RECORD_TIME variable to Record(), which is set to the recording duration before the application returns.

Resolves: #548

InterLinked1 commented 4 months ago

cherry-pick-to: none

seanbright commented 4 months ago

This could clobber an existing variable. Maybe it should be master only?

jcolp commented 4 months ago

This could clobber an existing variable. Maybe it should be master only?

Ooh, excellent point I hadn't considered. That or turning it into a dialplan function which can pull info about the recording just done.

InterLinked1 commented 4 months ago

Should be a UserNote (new feature) not UpgradeNote (possible breaking change).

It was requested this be master only because it "could" be breaking, that's why I removed the cherry picks and made it an upgrade note. Should it still be a user note?

jcolp commented 4 months ago

Should be a UserNote (new feature) not UpgradeNote (possible breaking change).

It was requested this be master only because it "could" be breaking, that's why I removed the cherry picks and made it an upgrade note. Should it still be a user note?

Since there could be a conflict in variable usage, I think UpgradeNote is appropriate.

seanbright commented 4 months ago

Is there another way this information could be made available without creating a channel variable? If it was a dialplan function as @jcolp mentioned you could make other information available too (recording format, file size, etc.) and that could be easily extendable in the future.

Edit: And then it would be a candidate for cherry picking which would be good,

Edit edit: Although I suppose a dialplan function have the same naming collision problem. I'm not sure if you can have a variable and a dialplan function with the same name.

asteriskteam commented 2 months ago

This PR has been marked stale because it has been in "Changes Requested" or "submitter-action-required" state for 28 days or more. Please make the requested changes within 14 days or the PR will be closed.

InterLinked1 commented 2 months ago

Is there another way this information could be made available without creating a channel variable? If it was a dialplan function as @jcolp mentioned you could make other information available too (recording format, file size, etc.) and that could be easily extendable in the future.

Edit: And then it would be a candidate for cherry picking which would be good,

Edit edit: Although I suppose a dialplan function have the same naming collision problem. I'm not sure if you can have a variable and a dialplan function with the same name.

Yeah, aside from that, a function to get the time feels kind of disjoint (at least to me), a variable is more consistent with the other things that are saved. Making it a function would probably also mean now a datastore is now needed to keep track of that, which seems like overkill for just exposing one piece of information. A function could be useful anyways but seems like it should be its own project to expose different information.

Personally I don't really feel like changing this if it's only a suggestion, but if you feel this should absolutely be a function, then I can do so.

seanbright commented 2 months ago

Edit edit: Although I suppose a dialplan function have the same naming collision problem. I'm not sure if you can have a variable and a dialplan function with the same name.

I just tested this and you can have a variable and a function with the same name - there is no conflict:

same =     n,NoOp(${CURL})
same =     n,Set(CURL=hello-${EXTEN})
same =     n,NoOp(${CURL})
same =     n,Hangup()

Personally I don't really feel like changing this if it's only a suggestion, but if you feel this should absolutely be a function, then I can do so.

This PR is not waiting on my review as far as I can tell.

My opinion is that new functionality should not create channel variables that could trample those set by the user. It's a suggestion.

jcolp commented 2 months ago

I agree regarding not using dialplan variables. Older things use dialplan variables because at the time there was no other way. For inclusion I will require it to be a dialplan function.

asteriskteam commented 1 month ago

This PR has been marked stale because it has been in "Changes Requested" or "submitter-action-required" state for 28 days or more. Please make the requested changes within 14 days or the PR will be closed.

github-actions[bot] commented 1 month ago

Successfully merged to branch master.

seanbright commented 1 month ago

Um. What?

jcolp commented 1 month ago

@gtjoseph Yes... what?

jcolp commented 1 month ago

When did I actually approve this... why... looks back

jcolp commented 1 month ago

Oh, I never unapproved when we had our discussion. That explains it. I'm reverting this and reopening.