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

Add input to rename internal video streams #1359

Conversation

ArturoManzoli
Copy link
Contributor

Now user can double click the stream name or press the 'pencil' icon to change its internal name:

Screenshare.-.2024-09-23.2_29_18.PM.mp4

Closes #1336

Copy link
Member

@rafaellehmkuhl rafaellehmkuhl left a comment

Choose a reason for hiding this comment

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

When the internal stream name is changed, are all the widgets that are connected to it losing their connection?

@ArturoManzoli
Copy link
Contributor Author

ArturoManzoli commented Sep 23, 2024

When the internal stream name is changed, are all the widgets that are connected to it losing their connection?

Yes, they are. I think is alright to automate that.

@ArturoManzoli ArturoManzoli marked this pull request as draft September 23, 2024 17:58
@ArturoManzoli ArturoManzoli self-assigned this Sep 23, 2024
@rafaellehmkuhl
Copy link
Member

rafaellehmkuhl commented Sep 23, 2024

When the internal stream name is changed, are all the widgets that are connected to it losing their connection?

Yes, they are. I think is alright to automate that.

There are probably two options here:

  • Option 1: just automate that, by watching for changes in the stream internal names, and reassingning all the widgets to it.
  • Option 2: adding an unique id (like a uuid4) to each stream and using that id to connect the widgets to the stream, instead of the internal stream name.

The first option is the easiest to implement, but it's much more likely to incur in bugs. The second is probably the right approach, unlikely to create any bug situations, as it removes the need for any side-effects, but involves a little more work.

@ArturoManzoli
Copy link
Contributor Author

The first option is the easiest to implement, but it's much more likely to incur in bugs. The second is probably the right approach, unlikely to create any bug situations, as it removes the need for any side-effects, but involves a little more work.

I went for option 1, but using a very controlled approach

@ArturoManzoli ArturoManzoli marked this pull request as ready for review September 30, 2024 21:38
@rafaellehmkuhl
Copy link
Member

The first option is the easiest to implement, but it's much more likely to incur in bugs. The second is probably the right approach, unlikely to create any bug situations, as it removes the need for any side-effects, but involves a little more work.

I went for option 1, but using a very controlled approach

Just to make sure, if the widgets were assigned to one stream, and this stream had its internal name changed, what is going to happen? Is it creating some error or is it auto-connecting to the first one available?

@ArturoManzoli
Copy link
Contributor Author

The first option is the easiest to implement, but it's much more likely to incur in bugs. The second is probably the right approach, unlikely to create any bug situations, as it removes the need for any side-effects, but involves a little more work.

I went for option 1, but using a very controlled approach

Just to make sure, if the widgets were assigned to one stream, and this stream had its internal name changed, what is going to happen? Is it creating some error or is it auto-connecting to the first one available?

After a stream is renamed, both the player and mini-recorder will check if the renamed stream is the one they are using. If it is, they will retrieve the new name from the video store and replace the old (renamed) one. If not, nothing happens.

Copy link
Member

@rafaellehmkuhl rafaellehmkuhl left a comment

Choose a reason for hiding this comment

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

It's all working fine for me!

I would ask you only to test it as much as possible before the next release. I will do the same here.

My main concern is if people rename streams and the widgets don't get it, specially the video recorder, so a user clicks the record button expecting things to be recorded and they are not. I didn't find any signs for this situation in the code thought.

@rafaellehmkuhl rafaellehmkuhl merged commit 6eb2eff into bluerobotics:master Oct 1, 2024
10 checks passed
@ArturoManzoli
Copy link
Contributor Author

It's all working fine for me!

I would ask you only to test it as much as possible before the next release. I will do the same here.

My main concern is if people rename streams and the widgets don't get it, specially the video recorder, so a user clicks the record button expecting things to be recorded and they are not. I didn't find any signs for this situation in the code thought.

I agree with your concerns. I'll test for that also

@ES-Alexander ES-Alexander added the docs-needed Change needs to be documented label Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-needed Change needs to be documented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

frontend: stream internal name should be editable
3 participants