Skip to content
This repository has been archived by the owner on Aug 6, 2023. It is now read-only.

Crossterm update 0.9.2 #150

Closed
wants to merge 1 commit into from
Closed

Conversation

TimonPost
Copy link
Contributor

@TimonPost TimonPost commented Apr 12, 2019

Updated the version of crossterm, notice that it has API breaking changes. The 0.9.^ version has a lot of improvements.

.idea/vcs.xml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@@ -151,7 +129,7 @@ impl Backend for CrosstermBackend {
}
s.object_style.attrs = cell.style.modifier.into();

self.crossterm.paint(s).map_err(convert_error)?;
write!(io::stdout(), "{}", s);
Copy link
Owner

Choose a reason for hiding this comment

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

Isn't this locking stdout every time you write ? Shouldn't you lock once and write to StdoutLock ?

Copy link
Contributor Author

@TimonPost TimonPost Apr 17, 2019

Choose a reason for hiding this comment

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

I am not sure since you have to pass write! a mutable write object.

Copy link
Owner

Choose a reason for hiding this comment

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

I could misunderstand the documentation but for me locking outside the for loop would avoid the implicit synchronization. Furthermore, the struct returned by io::stdout().lock() is also writable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I will take a look at if this could be done more efficiently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the lock out of the loop, by doing this we only lock outside the loop, this lock is then used in the loop for drawing all cells.

Cargo.toml Outdated
@@ -17,8 +17,9 @@ travis-ci = { repository = "fdehau/tui-rs" }
appveyor = { repository = "fdehau/tui-rs" }

[features]
default = ["termion"]
default = ["crossterm"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

can be changed when finished

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah I would rather not change the default backend for now. termion has been stable for a longer period of time. I could reconsider this in the future when crossterm reaches a stable release.

Copy link
Contributor Author

@TimonPost TimonPost Apr 18, 2019

Choose a reason for hiding this comment

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

agreed, I have this for testing, once the changes are accepted I'll change it back.

@TimonPost TimonPost changed the title Crossterm update 9.1 Crossterm update 9.2 Apr 17, 2019
@TimonPost TimonPost changed the title Crossterm update 9.2 Crossterm update 0.9.2 Apr 17, 2019
@TimonPost
Copy link
Contributor Author

Woops, I messed up my git, I'll create an new PR later.

@TimonPost TimonPost closed this Apr 18, 2019
@TimonPost TimonPost mentioned this pull request Apr 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants