Using cossterm for windows support. #99

TimonPost · 2019-10-22T19:23:04Z

Fixes: #69

This app can support windows very easily. TUI has the crossterm backend which is made to support cross-platform.

I tried to move this library over to crossterm just to experiment to how easy it was to swap termion. It was very easy. Please have a look at this fork.

Although...

Their implementation uses crossterm 0.9 and 0.12 is already out and they didn't even release my latest patch - done a few months ago - with 0.10 yet. This old version of crossterm has some problems and ideally it should use 0.12 it's way better performance-wise as well.

However, I encountered problems:

-. KeyEvent (replacement for Key) does not implement Copy however, this is going to be fixed in 0.12.2.

TUI uses crossterm ^0.9, therefore, use crossterm ^12 because TUI requires types from ^0.9
You probably don't want to have a dependency to git branch of TUI

The real blocker here was that crossterm::KeyEvent did not implement Copy and therefore it has to be wrapped in order for it to be used.

Introduced:

Key enum, this enum is used in the library and should be used instead of the termion::Key or crossterm::KeyEvent or others toughtout the crates. You probably don't want to depend on a certain library restricted to linux but have a small abstraction so that crossterm, for example, can be implemented with more ease.
CrosstermBackend this uses crossterm for its implementation and is crossplatform.

Qustions:

This is just an experiment because I wanted to listen to Spotify on windows, however, there are some concerns with git dependencies and old versions of crossterm. Do you think crossterm should be behind a feature flag, the only supported implementation, or not used at all?

TimonPost · 2019-10-22T19:27:17Z

I tried this app out, however something went wrong with my authentication URL redirect. Not sure if anything went wrong with this implementation or authentication.

Rigellute · 2019-10-23T08:31:45Z

Thanks for looking into this @TimonPost! This is great work, I'd love for spotify-tui to support Windows natively.

I think your Key abstraction is a good idea.

To respond to your questions above,

I would certainly prefer to use crate releases and not use git as the dependancy manager here.
If Crossterm delivers that same experience for all platforms as what we have already for macOS and Linux then it should be the only supported implementation, just to keep things simple.

I'm now thinking that we should wait for/push tui-rs to upgrade their crossterm dependancy to 0.12. And/or also wait for crossterm to support Copy (if that's still an issue).

But sounds like we're pretty close to supporting Windows, so once again thank you for this investigation!

TimonPost · 2019-10-23T11:38:06Z

left an issue here: fdehau/tui-rs#192

TimonPost · 2019-12-13T09:44:26Z

I updated to the newest version of crossterm 0.14. We will have to wait for TUI to merge and release this branch. Then this PR can finally be merged.

TimonPost · 2019-12-13T11:01:00Z

Windows Powershell

TimonPost · 2019-12-16T16:53:20Z

LINUX MINT

TimonPost · 2019-12-16T17:26:47Z

Remove /src/utils.rs, moved logic to /events/*
Created Key enum to be decoupled from external libraries.
Implemented crossterm for UI driver.
Upgraded to TUI 0.8 needed for crossterm 0.14 support.

TimonPost · 2019-12-18T17:23:20Z

Debian:

Rigellute

Incredible work @TimonPost!

Looking forward to trying this out!

Rigellute · 2019-12-23T11:41:33Z

.idea/vcs.xml

@@ -0,0 +1,6 @@
+<?xml version="1.0" encoding="UTF-8"?>


Are you able to remove the .idea directory?

Rigellute

Just tested this PR on macOS and all looks good!

Perhaps we could merge your other PR merging the imports first?

We can then finally rebase this PR and it should be ready to merge to master!

TimonPost · 2019-12-23T13:22:34Z

Nice, merging #190 sounds like a good idea. I made that one because this one accidentally included those changes as well. By merging that one, we can reduce the number of changes, which will make it a bit easier to see what the actual changes are. I will do a rebase once that one is merged.

- Remove Termion dependency - Use crossterm feature flag TUI - Added event module, for event handling behaviour. - Implemented crossterm 0.14.

TimonPost · 2019-12-24T14:17:22Z

I rebased and removed the idea files. It should be ready to be merged.

Rigellute

Incredible work @TimonPost!

Thank you 👍.

It is so awesome that spotify-tui will now work on Windows!

Rigellute · 2019-12-24T18:16:30Z

@TimonPost have run into a problem after merging - pressing ctrl-c to quit, causes an error

Error: RecvError

Which then crashes that terminal session.

Any ideas?

Will revert until this is ironed out.

TimonPost · 2019-12-24T19:21:00Z

ctrl-c stops the event reading loop. When this happens, the thread will finish, and the sender's side will be disposed of. If there is no sender, then the receiving side will error.

First, this wasn't the case, because the ticking thread also had a clone of the sender: https://github.com/Rigellute/spotify-tui/blob/master/src/util.rs#L55. However, crossterm can do both ticking and event reading in one thread. This sender can probably be kept as a local field in Events. That would keep the sender around even if the input reading threads stop.

This isn't a huge deal, I can look at this tomorrow.

TimonPost · 2019-12-24T19:38:00Z

Can you try this commit: TimonPost@ca32947

It did not work at my PC yet, going look into this a bit more tomorrow. However, you might give it a try.

TimonPost changed the title ~~[wip] windows support~~ [wip] Crossterm 0.14 and TUI 0.8 Dec 13, 2019

TimonPost force-pushed the crossterm_impl branch from bcda012 to e130a83 Compare December 13, 2019 09:42

TimonPost mentioned this pull request Dec 13, 2019

Crossterm 0.14 fdehau/tui-rs#209

Merged

TimonPost changed the title ~~[wip] Crossterm 0.14 and TUI 0.8~~ Implemented windows support with crossterm. Dec 16, 2019

TimonPost changed the title ~~Implemented windows support with crossterm.~~ Using cossterm for windows support. Dec 16, 2019

Rigellute reviewed Dec 23, 2019

View reviewed changes

TimonPost force-pushed the crossterm_impl branch 2 times, most recently from ea351ca to 57170ad Compare December 24, 2019 14:07

Implemented crossterm for crossplatform compatibility.

050622e

- Remove Termion dependency - Use crossterm feature flag TUI - Added event module, for event handling behaviour. - Implemented crossterm 0.14.

TimonPost force-pushed the crossterm_impl branch from 57170ad to 050622e Compare December 24, 2019 14:09

TimonPost closed this Dec 24, 2019

Removed windows note from readme, and utils file.

cb948e1

TimonPost reopened this Dec 24, 2019

Rigellute approved these changes Dec 24, 2019

View reviewed changes

Rigellute merged commit a9db66b into Rigellute:master Dec 24, 2019

Rigellute mentioned this pull request Dec 24, 2019

Add support for Related artists and top tacks #191

Merged

Rigellute mentioned this pull request Dec 24, 2019

Revert "Using cossterm for windows support." #197

Merged

This was referenced Dec 25, 2019

Windows compatibility with crossterm #198

Closed

Windows compatibility with crossterm #199

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using cossterm for windows support. #99

Using cossterm for windows support. #99

TimonPost commented Oct 22, 2019 •

edited

TimonPost commented Oct 22, 2019

Rigellute commented Oct 23, 2019

TimonPost commented Oct 23, 2019

TimonPost commented Dec 13, 2019

TimonPost commented Dec 13, 2019 •

edited

TimonPost commented Dec 16, 2019

TimonPost commented Dec 16, 2019

TimonPost commented Dec 18, 2019

Rigellute left a comment

Rigellute Dec 23, 2019

TimonPost Dec 24, 2019

Rigellute left a comment

TimonPost commented Dec 23, 2019 •

edited

TimonPost commented Dec 24, 2019

Rigellute left a comment

Rigellute commented Dec 24, 2019 •

edited

TimonPost commented Dec 24, 2019 •

edited

TimonPost commented Dec 24, 2019 •

edited

Using cossterm for windows support. #99

Using cossterm for windows support. #99

Conversation

TimonPost commented Oct 22, 2019 • edited

TimonPost commented Oct 22, 2019

Rigellute commented Oct 23, 2019

TimonPost commented Oct 23, 2019

TimonPost commented Dec 13, 2019

TimonPost commented Dec 13, 2019 • edited

TimonPost commented Dec 16, 2019

TimonPost commented Dec 16, 2019

TimonPost commented Dec 18, 2019

Rigellute left a comment

Choose a reason for hiding this comment

Rigellute Dec 23, 2019

Choose a reason for hiding this comment

TimonPost Dec 24, 2019

Choose a reason for hiding this comment

Rigellute left a comment

Choose a reason for hiding this comment

TimonPost commented Dec 23, 2019 • edited

TimonPost commented Dec 24, 2019

Rigellute left a comment

Choose a reason for hiding this comment

Rigellute commented Dec 24, 2019 • edited

TimonPost commented Dec 24, 2019 • edited

TimonPost commented Dec 24, 2019 • edited

TimonPost commented Oct 22, 2019 •

edited

TimonPost commented Dec 13, 2019 •

edited

TimonPost commented Dec 23, 2019 •

edited

Rigellute commented Dec 24, 2019 •

edited

TimonPost commented Dec 24, 2019 •

edited

TimonPost commented Dec 24, 2019 •

edited