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

Eliminate compiler warning. #11

Merged
merged 1 commit into from
Feb 16, 2016
Merged

Eliminate compiler warning. #11

merged 1 commit into from
Feb 16, 2016

Conversation

waynr
Copy link
Contributor

@waynr waynr commented Feb 14, 2016

No description provided.

@withoutboats
Copy link
Owner

I think the handle can actually be eliminated from the return value of this function, and from the code in general (tty is a crate under scaffolding which is a sort of weak abstraction of the tty waiting until rust-lang/rfcs#1359 actually lands and a more rusty abstraction of the tty can be written). I was using the Handle for setting the window size when the write head of the tty was separated from the terminal window, but now I think that's unnecessary.

If you'd like to make that change, would be cool. :-) Just eradicate the handle type and we'll see if it works.

@waynr
Copy link
Contributor Author

waynr commented Feb 15, 2016

@withoutboats I think this is what you wanted. Would it also make sense to increment the major version on the tty crate since this changes a function signature and removes a type?

@withoutboats
Copy link
Owner

So it turns out I was wrong about not needing Handle, sorry :-. Handle's drop implementation is necessary to close the tty when both the reader and writer are dropped (and not before).

It also looks like I actually introduced a bug when I stopped using handle, and the set winsize ioctl is no longer being called. My double-bad! This is maybe why the bash prompt doesn't show up properly at first (which is a bug that didn't used to exist).

So in order to merge this I need for you to bring Handle back, but don't return it as part of the tuple. Then I'll work on figuring out the best way to make sure the winsize setting call gets made again.

Sorry for giving bad info earlier, forgot what my own code did. 😆

Would it also make sense to increment the major version on the tty crate since this changes a function signature and removes a type?

This isn't necessary since tty is checked into the repo and not on crates.io (the tty on crates.io is someone else's crate). All of these crates are being treated as pre-semver right now, and it would be very surprising and a little nuts if a project other than notty were depending on this tty crate.

@waynr
Copy link
Contributor Author

waynr commented Feb 15, 2016

And I just realized that maybe I should have left the winsize code in also as a reference, let me know if that's the case.

For what it's worth, I can run bash as the top-level shell on my machine. It even lets me run the ls command.

@withoutboats
Copy link
Owner

You might as well save all of the code on Handle, the winsize stuff is going to be used eventually. The Drop implementation is absolutely necessary - otherwise the shell won't exit when the terminal closes.

If you could do that and then squash it to one commit, I'll merge it in. Thanks so much! :-)

@withoutboats withoutboats self-assigned this Feb 15, 2016
@waynr waynr changed the title Eliminate compiler warning, set default scaffolding window size. Eliminate compiler warning. Feb 16, 2016
@waynr
Copy link
Contributor Author

waynr commented Feb 16, 2016

Okay I just went with the original commit here.

I opened another PR with the gtk window changes, including a handler for the window delete event that ensures the process will exit properly (previously it was hanging).

@waynr
Copy link
Contributor Author

waynr commented Feb 16, 2016

Also, even without the Drop implementation on Handle I see the scaffolding shell terminate when I kill the scaffolding process but maybe I am misunderstanding what you are describing.

@withoutboats
Copy link
Owner

I see the scaffolding shell terminate when I kill the scaffolding process but maybe I am misunderstanding what you are describing.

You're right, the system will close file handles for killed processes, but if for example scaffolding supported tabbed terminals, the shells would remain up after you close a tab.

withoutboats added a commit that referenced this pull request Feb 16, 2016
Eliminate compiler warning.
@withoutboats withoutboats merged commit 00bed32 into withoutboats:master Feb 16, 2016
@waynr
Copy link
Contributor Author

waynr commented Feb 16, 2016

Ah, gotcha thanks for the explanation.

@withoutboats
Copy link
Owner

I wanted to follow up that I have made it so that resizing the window correctly resizes the tty's windows with the TIOCSWINSZ ioctl. I should probably start feature branching and PRing my changes.

@waynr waynr deleted the maint branch April 1, 2016 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants