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.

Sign up for GitHub

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

Jump to bottom

fix: determine custom terminal ids on spawn #488

Merged
merged 2 commits into from
Sep 6, 2023
Merged

fix: determine custom terminal ids on spawn #488

merged 2 commits into from
Sep 6, 2023

Conversation

willothy
Copy link
Contributor

@willothy willothy commented Sep 6, 2023

This PR sets IDs at spawn-time for terminals that were created without an id/count specifies. See linked issue for more info.

closes #487

@willothy willothy mentioned this pull request Sep 6, 2023
Closed
1 task
akinsho
akinsho reviewed Sep 6, 2023
View reviewed changes
Copy link
Owner

@akinsho akinsho left a comment

Choose a reason for hiding this comment

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

Small change to the logic otherwise this LGTM, thanks 👍🏾
I don't recall off the top of my head but presumably spawn is always called right so we aren't going to miss giving some terminal an ID now?

@willothy
Copy link
Contributor Author

willothy commented Sep 6, 2023

Small change to the logic otherwise this LGTM, thanks 👍🏾 I don't recall off the top of my head but presumably spawn is always called right so we aren't going to miss giving some terminal an ID now?

Yes, exactly. Usually spawn would be called right away, but for cases where it isn't IDs will now be assigned "lazily" so that they don't conflict. Any terminal with a predefined ID will of course have the same behavior as before.

@akinsho akinsho merged commit 8572917 into akinsho:main Sep 6, 2023
@github-actions github-actions bot mentioned this pull request Sep 6, 2023
Merged
akinsho added a commit that referenced this pull request Sep 7, 2023
@akinsho
revert: determine custom terminal ids on spawn (#488)
0e4dcb8 
This reverts commit 8572917.
@akinsho akinsho mentioned this pull request Sep 7, 2023
Closed
1 task
@willothy
Copy link
Contributor Author

willothy commented Sep 7, 2023

Any ideas for a better way of doing this since it caused some issues?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akinsho akinsho akinsho left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Terminal id collisions with custom terminals
2 participants
@willothy @akinsho