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

Spin spacing between elements after erasing #77

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

joetannenbaum
Copy link
Contributor

Ok, taking another stab at this one based on your feedback in #72.

I tested the following scenarios:

  • Question -> Spinner -> Question
  • Spinner -> Question
  • Spinner -> Spinner -> Question

and the spacing looked consistent:

CleanShot.2023-09-20.at.22.01.36-converted.mp4

I think the main question is do you like the API and the fact that this adds another method and some light logic to the ConsoleOutput class.

I thought about abstracting this further in case other components might want to use it, but it felt like overkill and I couldn't think of another scenario where it would be relevant.

Let me know what you think!

@jessarcher
Copy link
Member

jessarcher commented Sep 21, 2023

Hey @joetannenbaum,

Thanks again for contributing! Unfortunately, I don't think this approach will work with Laravel.

When Prompts are used with Laravel commands, we set the output buffer to the existing one we already have in the Laravel command to track newlines between Prompt output and other output in the command. The ConsoleOutput class is only used when using Prompts without Laravel.

There is a static method on the Prompt class called writeDirectly that will bypass newline capturing, regardless of whether the output is ConsoleOutput or Laravel's OutputStyle class. We currently only use this to write some ANSI escape sequences, like showing/hiding the cursor and erasing characters, because these sequences don't visually render anything but would cause the trailing newline capture to think there were no trailing newlines in the last output.

You could potentially add a new $direct boolean parameter to the render function that would make it use writeDirectly instead of write. That way, the trailing newline count should be unchanged from whatever the previous prompt was.

Alternatively, we could change the behaviour so that the spin component always renders a success (and maybe error?) state instead of trying to make it behave like it was never there. That would be a bigger change and require exposing some new APIs for developers to trigger the different states and perhaps customise the text (potentially a success and fail callback passed to the user's callback?)

As an aside, my ultimate goal for the spinner would be if you could optionally stream output from your callback and have it rendered above the spinner, pushing it down the page as needed. A bit like what npm install does. That would complicate things as far as newline tracking, though.

@jessarcher jessarcher marked this pull request as draft September 21, 2023 02:38
@jessarcher jessarcher added the bug label Sep 21, 2023
@jessarcher jessarcher self-requested a review September 21, 2023 02:38
@joetannenbaum
Copy link
Contributor Author

Ah, gotcha. Ok that makes sense, I didn't realize that it worked differently within the context of Laravel.

I made a (failed) package a while ago that included a terminal spinner and solved the issue by having a final message/displaying the result.

I was using spatie/fork to stream messages between the forked processes to allow for visual updates to the user as it spun, but I like your idea of the messages showing up above the spinner and pushing it down.

I'll cool down with the PRs for now, but I'll keep exploring this! Thanks for your guidance.

@jessarcher
Copy link
Member

Oh cool! Taylor showed me your work while I was building Prompts, as it seems we both had a similar idea around the same time! I didn't realise you were the same person!

All good with the PRs! You've definitely improved the package with both features and fixes. There are just some intricacies that are hard to know about without having all the context loaded in your head, but I'm always here to help with that 🙂

@joetannenbaum
Copy link
Contributor Author

Yeah Taylor gave me a heads up when I was making Terminalia that Prompts was coming down the pipeline, so it became more of a learning project than anything else. Ultimately I made some wrong assumptions about line tracking so it's not a very usable library anymore, but I definitely learned a lot and did a ton of source diving into all of the Symfony console stuff. And now we have Prompts! Which is my terminal dream.

I couldn't get the streaming thing out of my head, so I put together a frankenstein'ed-poor man's-happy path-oriented version of it, and it's pretty cool. I might keep playing with it and see if I can get some more consistency.

CleanShot.2023-09-21.at.09.41.55.mp4

I basically nabbed the socket pair strategy from Spatie's fork package and the messenger helper in the callback strategy from Terminalia so it looks something like this:

spin(
    function (SpinnerMessenger $messenger) {
        foreach (range(1, 10) as $i) {
            $messenger->send("<info>✔︎</info> <comment>Step {$i}</comment>");
            sleep(1);
        }

        return 'Callback return';
    },
    'Taking necessary steps...',
);

Again, this is a real happy path solution, when I tried to run something like composer update in real time the messages came fast and furious and it sort of got garbled in the output.

I also don't know if it's smarter to have the user intentionally send the output, or to auto-capture the output from whatever is running. I tend to lean intentional to give the user more control, but not sure in this case.

@jessarcher
Copy link
Member

Very cool @joetannenbaum! I don't know either on the output stuff. I haven't done a proof of concept yet to understand what would work in different scenarios.

I do think it would be great if we could handle composer update type streams, but also more simple output like your example. It could also be cool if you could change the spin message throughout the process, although that would probably be hard due to the forking. People can always just use multiple spinners, although I'm not sure how that would look if you were outputting stuff from the spinner as well.

@joetannenbaum
Copy link
Contributor Author

Awesome, I'm going to keep tinkering with this and see if I can stabilize it a bit. I also like the idea of having the ability to update the spin message as you go, given the current setup I'm using that actually should be possible.

@joetannenbaum
Copy link
Contributor Author

Before I pack it in the for the night, got it behaving much more consistently at higher output speeds. Added a little dividing line to distinguish the spinner from the streaming output. Starting to look sort of nice!

CleanShot.2023-09-21.at.23.53.21.mp4

@jessarcher
Copy link
Member

That looks incredible! 😍

Exactly what I'd envisaged when I first made the spin component but didn't have time to tinker around with. Fantastic job! I can't wait to see where the API lands and what it looks like with composer output!

Is this something you see being able to be part of Prompts itself? Would it need any other dependencies?

@joetannenbaum
Copy link
Contributor Author

I would love for it to be part of Prompts, for sure!

As of now, just to get a proof of concept going, I wholesale just grabbed the Connection class from the Spatie fork package. That's using sockets to communicate between the forked processes, so that's introducing something but I think it's generally enabled by default in PHP? We can always silently opt out of it if it's not available for some reason and just not stream the results.

I was able to get it working with a composer update call in the video above, it just zips by super fast in the beginning.

I'm running into an issue that I'm still working out where if the re-rendered lines go past the top of the terminal, the calculations all go sideways and it just starts overwriting itself in weird ways:

CleanShot.2023-09-22.at.00.07.40.mp4

Right now it's re-writing all of the buffer every time, which is a little bonkers. It should really just append anything new and re-write the spinner itself. So I'm working on that.

All in all, I would love it to the sort of thing where if you just want to use the spinner you can use it exactly as you do now. If you want to stream some feedback to the terminal, you can just opt in pretty simply.

@jessarcher
Copy link
Member

I was able to get it working with a composer update call in the video above, it just zips by super fast in the beginning.

Just noticed that! Fantastic!

Right now it's re-writing all of the buffer every time, which is a little bonkers. It should really just append anything new and re-write the spinner itself. So I'm working on that.

Nice. I'm thinking every time you need to output something, you'd erase the spinner, echo just the new output so it appears immediately after any previous output, and then redraw the spinner again underneath, so it's always at the bottom and the output would just push things up the terminal as needed.

@joetannenbaum
Copy link
Contributor Author

Ok! Update:

✅ Rendering now works as described, simply appending the streamed data and re-rendering spinner underneath
✅ You can also update the message as you go if you'd like, as shown below (changes at Step 25 and 75)

I'm still finding some edge cases and refining the API a bit, but I'll put it in a fresh PR in the next couple of days for you to review and see if you like it.

CleanShot.2023-09-22.at.15.42.47.mp4

Have a great weekend!

@Wirone
Copy link

Wirone commented Apr 17, 2024

This looks pretty neat! 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants