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

feat: add new removeLineByKey method as a PoC #20

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions src/model/media-description.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,4 +126,48 @@ export abstract class MediaDescription implements SdpBlock {
// If it didn't match anything else, see if IceInfo wants it.
return this.iceInfo.addLine(line);
}

/**
* Remove a line from the media description by key.
*
* @param key - The key of the line to be removed.
* @returns True if a line was removed; false otherwise.
*/
removeLineByKey(key: keyof MediaDescription): boolean {
if (key === 'bandwidth' && this.bandwidth) {
delete this.bandwidth;
return true;
}
if (key === 'mid' && this.mid) {
delete this.mid;
return true;
}
if (key === 'fingerprint' && this.fingerprint) {
delete this.fingerprint;
return true;
}
if (key === 'setup' && this.setup) {
delete this.setup;
return true;
}
if (key === 'connection' && this.connection) {
delete this.connection;
return true;
}
if (key === 'content' && this.content) {
delete this.content;
return true;
}

// The 'otherLines' key would represent any other line in otherLines.
if (key === 'otherLines') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd say this doesn't work very well for the 'other lines' case--do you have any ideas on how that situation would be supported in this scheme?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I think it's important to be able to handle otherLines here. If we're going to have this API, I'm wondering if the param for this function should just be a Line, similar to the addLine API? That would allow this API to work with any line (including otherLines), not just lines that are associated with a property of MediaDescription.

Calling this API then, would look something like mediaDescription.removeLine(mediaDescription.bandwidth).

This would be tricky for some of the properties though. mid and setup, for example, are not actual lines, so we'd have to handle those a bit differently.

Copy link
Author

@antsukanova antsukanova Apr 28, 2024

Choose a reason for hiding this comment

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

@bbaldino I've discussed some current code logic with @brycetham, and this example implementation will not be a good idea due to issues with "other lines" as you mentioned. But I want to take time and think about the idea mediaDescription.removeLine(mediaDescription.bandwidth). I also had such an idea, so I just need to see how tricky it is to change some properties' implementations. I will prepare a new example and update this PR. Still, I think aligning functionality to removing likes could be a good idea, and later, it can save us time and keep in our base better abstraction.

@brycetham @bbaldino thanks you for your thoughts and discussion

// Here we can implement logic to handle removal from otherLines if needed.

// For now, we can just return false to indicate no action taken.
return false;
}

// If the key is not found, return false.
return false;
}
}
Loading