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

Upgrade to Crossterm 0.14 #13

Closed
zicklag opened this issue Dec 13, 2019 · 10 comments
Closed

Upgrade to Crossterm 0.14 #13

zicklag opened this issue Dec 13, 2019 · 10 comments

Comments

@zicklag
Copy link

zicklag commented Dec 13, 2019

Crossterm 0.14 was just released and it has a new event system along with support for resize events. These are features I'd like to use in my project, but Termimad would need to update to 0.14 first.

Would it help if I were to submit a PR for this? I haven't yet looked into how hard that might be, but I'm thinking it would be simple.

@Canop
Copy link
Owner

Canop commented Dec 13, 2019

Please don't do a PR for that. I follow crossterm and every release involves a lot of tests and adaptations. I'll manage that myself.

@Canop
Copy link
Owner

Canop commented Dec 15, 2019

I did the migration. I also migrated broot (there was a lot to rewrite).
I'm still not 100% sure there's no problem with crossterm 0.14 event system, though. I'll go on testing. I pushed so that you may test too. Please let me know how it works fo you.

@zicklag
Copy link
Author

zicklag commented Dec 15, 2019

Awesome, thanks. I'll try it out.

@zicklag
Copy link
Author

zicklag commented Dec 15, 2019

Worked great! Now the docs view will updated when resized.

@zicklag zicklag closed this as completed Dec 15, 2019
@zicklag
Copy link
Author

zicklag commented Dec 15, 2019

Oh, my CI build found a compile error on Windows:

error[E0382]: use of moved value: `val`
   --> /usr/local/cargo/registry/src/github.com-1ecc6299db9ec823/termimad-0.8.3/src/compound_style.rs:149:55
    |
144 |     pub fn queue<W, D>(&self, w: &mut W, val: D) -> Result<()>
    |                     -                    --- move occurs because `val` has type `D`, which does not implement the `Copy` trait
    |                     |
    |                     consider adding a `Copy` constraint to this type argument
...
149 |         Ok(queue!(w, PrintStyledContent(self.apply_to(val)))?)
    |                                                       ^^^ value moved here, in previous iteration of loop

error: aborting due to previous error

@zicklag zicklag reopened this Dec 15, 2019
@Canop
Copy link
Owner

Canop commented Dec 16, 2019

That seems to mean that the Windows version of the queue! doesn't accept non Copy values.
That's weird because I didn't expect this to have changed between crossterm 0.13.3 and crossterm 0.14 and I guess we'll have to dive into the source of crossterm.
Problem is probably there: https://docs.rs/crossterm/0.14.0/src/crossterm/macros.rs.html#37

@TimonPost
Copy link

TimonPost commented Dec 16, 2019

The val should implement both Display and Clone. https://github.com/crossterm-rs/crossterm/blob/master/src/style/content_style.rs#L20, I am not sure how this relates to the queue macro. It seems to be the problem of self.apply_to or PrintStyledContent. Can you perhaps try to call clone on val?

@Canop
Copy link
Owner

Canop commented Dec 16, 2019

I confirm the problem is related to the macro. If I comment some part, there's no more compilation problem on windows:

/// Writes/executes the given command.
#[doc(hidden)]
#[macro_export]
macro_rules! handle_command {
    ($writer:expr, $command:expr) => {{
        // Silent warning when the macro is used inside the `command` module
        #[allow(unused_imports)]
        use $crate::{write_ansi_code, Command};

        #[cfg(windows)]
        {
//            // ansi code is not always a string, however it does implement `Display` and `Write`.
//            // In order to check if the code is supported we have to make it a string.
//            let ansi_code = format!("{}", $command.ansi_code());
//
//            if $command.is_ansi_code_supported() {
//                write_ansi_code!($writer, &ansi_code)
//            } else {
                $command.execute_winapi().map_err($crate::ErrorKind::from)
//            }
        }
        #[cfg(unix)]
        {
            write_ansi_code!($writer, $command.ansi_code())
        }
    }};
}

At this point I'm not sure of how to fix it.
Should we clone the value when in Windows ? Should the argument have the Copy constraint on Windows ?
I'll propose we pursue the discussion on the crossterm discord: https://discordapp.com/channels/560857607196377088/560857607196377090

@Canop
Copy link
Owner

Canop commented Dec 16, 2019

OK, found why the macro makes my code fail on windows and how to fix it.
The problem with macros is that arguments are repeated instead of being used.
So

Ok(queue!(w, PrintStyledContent(self.apply_to(val)))?)

is very different from

let command = PrintStyledContent(self.apply_to(val));
Ok(queue!(w, command)?)

I'm fixing it.

@zicklag
Copy link
Author

zicklag commented Dec 17, 2019

CI is happy again. Thanks. 👍 🎉

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

No branches or pull requests

3 participants