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 terminfo implementation for animating output #25

Merged
merged 2 commits into from
Apr 29, 2015

Conversation

asolove
Copy link
Contributor

@asolove asolove commented Apr 25, 2015

This proposal adds the terminfo implementations of three commands: cursor_up, delete_line, and carriage_return. These three commands provide the building blocks for implementing animated terminal output.

I wrote a a sample program that shows how to use these commands to create pretty console progress updates and an animated spinner.

For your consideration:

  • In More shortcut functions in crate term #11, @retep998 graciously offered to implement the Windows versions, but I may also take a crack at it tomorrow.
  • I have hard-coded the terminfo strings for these three commands rather than adding them to the Attr enum, because they are commands rather than attributes. I think there is an argument to be made for either renaming Attr to Command or adding these commands into it either way, but wanted to ask someone more knowledgeable for their advice before doing so.
  • The name of the command carriage_return is potentially confusing since \r has different behavior on different systems, but the terminfo docs make clear that terminals should implement this command by moving the cursor to the far left of the current line. It should possibly be renamed something like cursor_far_left.
  • I didn't write any tests because I wasn't sure how to do so in a meaningful way.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

@alexcrichton
Copy link
Collaborator

Unfortunately we can't merge this until there's a Windows implementation available. I think I just set up CI infrastructure for Windows, though, so we should at least know!

@asolove
Copy link
Contributor Author

asolove commented Apr 27, 2015

Absolutely understand and thanks for setting up the Windows CI build. I'm planning to look into the Windows implementation tonight but don't have a way to test it myself and am not sure how to write tests for these features. Maybe @retep998 or someone else in the Rust Windows community can help.

@retep998
Copy link
Contributor

I am writing an implementation for Windows right now (and also cleaning up a few bits as well).

@asolove
Copy link
Contributor Author

asolove commented Apr 27, 2015

Awesome, thanks! Happy to help or get any critiques.

@retep998
Copy link
Contributor

@asolove Windows implementation is ready #26

@alexcrichton alexcrichton merged commit 10cc9f2 into Stebalien:master Apr 29, 2015
alexcrichton added a commit that referenced this pull request Apr 29, 2015
@asolove
Copy link
Contributor Author

asolove commented Apr 29, 2015

Thanks @retep998! Now that I see what you did, there's no chance I would have figured all that out quickly. And thanks @alexcrichton for the quick merge.

This whole contributing as a newbie to Rust thing is pretty fun.

Now: is there a process for putting this new version into the libterm folder in the main rust repo? Should I just make the changes in a PR that uses them?

@alexcrichton
Copy link
Collaborator

This is actually somewhat of a fork from the libterm in the main repo, and this is now the "official location" for the library. What's left in the distribution is solely intended for use by the compiler itself. In other words, you're all set! (there's nothing left to do).

@retep998
Copy link
Contributor

@alexcrichton I believe the intention was to provide this implementation so that it could be used to implement rust-lang/rust#24335.

@alexcrichton
Copy link
Collaborator

Ah in that case a PR would need to be made, yes. We don't have a great story for the duplication of the in-tree libraries with out-of-tree copies in terms of API evolution, although now that we have a sense of stability it may be possible to just submodule everything.

@asolove for now feel free to just make a PR against the compiler itself (modifying the in-tree libterm with these changes) and we can sort out the duplication ourselves later :)

@retep998
Copy link
Contributor

@asolove Don't forget that this library uses winapi while in the rust repo it depends on libc so you'll have to copy over missing bindings to make it work on Windows.

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.

None yet

4 participants