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

refactor/Hide-Show #304

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

elmarsan
Copy link
Contributor

@elmarsan elmarsan commented May 15, 2023

This pull request adds two new commands, Pause and Resume.

All commands between Pause and Resume will be executed but not include into the output gif.
In order to achieve this behavior I did the following:

  • Create new terminal in 'hidden mode', every time Pause command is executed, all the commands will be processed into new instance of ttyd.
  • Encapsulate Page, browser, textCanvas, cursorCanvas, tty and close VHS fields into new Terminal struct.
  • VHS now has currentTerm, mainTerm and hiddenTerm which are pointers to Terminal instances.
    • mainTerm: Styling is only applied to this instance.
    • hiddenTerm: Simple ttyd terminal with the default configuration.
    • currentTerm: It's a mirror of mainTerm when execution is not pause and mirror of hiddenTerm when execution is paused.

Demo

Output pause-resume.gif

Pause

Type "mkdir folder"
Enter

Resume

Type "echo 'See secret folder'"
Sleep 1s
Enter

Type "ls | grep folder"
Sleep 1s
Enter

Sleep 2.5s

Output
Example of pause and resume

Closes #130

@elmarsan
Copy link
Contributor Author

Hey, I'm having issues with the workflow. The tests are running fine on my local machine, but I suspect there might be a problem related to ttyd.

After reviewing the code, I noticed that I'm the first one using the New() method in the tests. This could be the root of the issues because the CI might not be ready for that yet."

@maaslalani
Copy link
Contributor

maaslalani commented May 17, 2023

What are your opinions on making this the behaviour of Hide / Show instead of introducing new keywords Pause / Resume?

I think this type of behaviour fits quite well with Hide / Show IMO. Since you are essentially performing hidden commands (on a different terminal).

@elmarsan
Copy link
Contributor Author

@maaslalani I'm complete agree with that!

If we add these new commands, users will probably get confused. And could be hard to explain the difference in the documentation between Hide/Show and Pause/Resume.
Moreover I'm not sure if users find useful the current behavior of Hide and Show. It's weird to see how frames stack up and pop out all at once in the GIF after Show command.

But yes, I would just move this logic to Hide/Show

@elmarsan elmarsan changed the title Feat/pause and resume Refactor/Hide-Show May 25, 2023
@elmarsan elmarsan changed the title Refactor/Hide-Show refactor/Hide-Show May 25, 2023
@elmarsan
Copy link
Contributor Author

Hey @maaslalani

I've removed Pause & Resume commands. Now the behavior is on Hide & Show. I've also rename the pull request because we're not adding new commands.
As far as I can tell, everything is working as expected. Could you give it a look, please?

@elmarsan
Copy link
Contributor Author

@maaslalani conflicts solved

@maaslalani
Copy link
Contributor

Thanks so much @elmarsan, I'm excited for this one.

@elmarsan
Copy link
Contributor Author

@maaslalani is everything ok?

@helpermethod
Copy link

What has happened to this PR? It would make VHS infinitely more useful!

@maaslalani
Copy link
Contributor

Hey! Sorry about forgetting about this PR, I will take a look since it is a great feature that I think would make VHS more useful!

@elmarsan
Copy link
Contributor Author

@maaslalani let me know if you need any help.

@patniemeyer
Copy link

What are your opinions on making this the behaviour of Hide / Show instead of introducing new keywords Pause / Resume?

I think this type of behaviour fits quite well with Hide / Show IMO. Since you are essentially performing hidden commands (on a different terminal).

I agree that this should be the default behavior of Hide / Show. As a new user I was very confused when "Hide" did not actually seem to hide anything. Breaking changes are always hard, but it won't get any easier later :)

Maybe rename the old functionality to "fast forward" "start/stop"? :)

@osterman
Copy link

@maaslalani would love to see this feature! It's very hard to set up the demo environment before recording, since any rc files are disabled (otherwise this would be a great alternative). The current behavior, showing all the pre-set up commands, detracts from the demo itself (using hacks like Ctrl+L still cause a flicker). Also, when wanting to record demos of specifically one aspect of a tool, all the steps to get to that part are irrelevant. I've resorted to using tools like ffmpeg to cut off the first part of the recording, but it's not ideal.

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.

Hide is not working
5 participants