jasonjmcghee / plock

From anywhere you can type, query and stream the output of an LLM or any other script
MIT License
473 stars 31 forks source link

#10 generation skipping some characters #20

Closed kengoodridge closed 10 months ago

kengoodridge commented 10 months ago

This is a workaround for an issue with enigo with strings that start with newline (maybe macos only?)

I made sure it never passes a string to enigo.text("\nbad") that started with a newline

jasonjmcghee commented 10 months ago

I will review this soon- super excited

kengoodridge commented 10 months ago

@jasonjmcghee This project is so pretty in its usefulness to size ratio. It's like the perfect solo open source project. I'm so jealous I didn't think of it first. 🤣

jasonjmcghee commented 10 months ago

You're going to kill me, but I don't agree with the "Enter" change. That will break a lot of use cases, like typing into an input that triggers submit on enter

jasonjmcghee commented 10 months ago

@jasonjmcghee This project is so pretty in its usefulness to size ratio. It's like the perfect solo open source project. I'm so jealous I didn't think of it first. 🤣

Fwiw, definitely not the first to think of this, people reached out to me that said they built this before / 10 years ago etc.

But I built a prototype and it was immediately useful to me, so I wanted to share! And hopefully find developers like you so we could make the project better together.

kengoodridge commented 10 months ago

no killing here. Wasn't super confident with it. Tell me if you can reproduce it tho.

Feel free to edit that out of this PR. The top part fixes the missing text tho, or seem to anyway.

jasonjmcghee commented 10 months ago

no killing here. Wasn't super confident with it. Tell me if you can reproduce it tho.

Feel free to edit that out of this PR. The top part fixes the missing text tho, or seem to anyway.

I'm trying to understand why your changeset fixes it. The buffer should get joined and simulated at the end, after the !did_exit, (which should only be set if Esc is pressed)

Also, people were reporting this happening in the middle of text, not at the end... Can you spoon feed me why it fixes it? 😅

jasonjmcghee commented 10 months ago

It's streaming in blocks of text, so any time less than 4 characters streams in it just skips it. It can be anywhere in the text. Not at the end necessarily.

But it adds it to the buffer, which isn't cleared, so it should still be there for the next iteration once it has enough text 🤔 what am I missing 😂

I wonder if trying to simulate empty string it screwing with things?

jasonjmcghee commented 10 months ago

I reproduced it with your changeset:

https://github.com/jasonjmcghee/plock/assets/1522149/19746ac1-1dba-40d0-befe-d5a3d4b9b72a

jasonjmcghee commented 10 months ago

Was it the newlines that fixed it? I will try that too

jasonjmcghee commented 10 months ago

For what it's worth, my current supposition on this issue is something like the system is i/o blocking and enigo can't detect that and fails to enter the text. i plan to log an issue in the enigo repo here in a sec

kengoodridge commented 10 months ago

Something is up with my "fix". It definitely introduced the new line problem. I'm going to sleep on it.

It's weird cuz, I tested it several times and diffed the text inserted against the text in the console.

Hopefully, I've not wasted your time.

jasonjmcghee commented 10 months ago

Something is up with my "fix". It definitely introduced the new line problem. I'm going to sleep on it.

It's weird cuz, I tested it several times and diffed the text inserted against the text in the console.

Hopefully, I've not wasted your time.

Of course you haven’t. You taking the time to work on a fix is absolutely awesome- regardless of whether it ends up working!

kgoodridge commented 10 months ago

@jasonjmcghee You were on the money...

Was it the newlines that fixed it? I will try that too

I couldn't get it out of my mind, so I had a theory in the middle of the night... "I wonder if enigo has an issue if the string starts with newline (maybe macos only)"

I confirmed it this morning and hacked a workaround, but I'm at work and should wait to commit with my personal account. Not sure you want a semi-hacky workaround anyway.

                {
                    while let Some(response) = response_stream.next().await {

                        let delta_output = { 
                            if delta_buffer.len() > 4 
                               && !response.starts_with('\n') // workaround for a bug in enigo, it doesn't like leading newlines
                            {
                                let s = delta_buffer.clone().join("");
                                delta_buffer.clear();
                                s
                            } else {
                                "".to_string()
                            }
                        };

                        // move this to after the delta_output so we can not start a delta_buffer with a newline
                        whole_buffer.push(response.clone());
                        delta_buffer.push(response);

                        for step in trigger.next_steps.clone() {
                            if let Step::StreamTextToScreen = step {
                                enigo.text(&delta_output).expect("Failed to type out text");                                
                            }
                        }

                        // Exit loop if child process has finished or exit flag is set
                        if exit_flag_thread.load(Ordering::SeqCst) {
                            did_exit = true;
                            break;
                        }
                    }
                }
kengoodridge commented 10 months ago

@jasonjmcghee TLDR try out the latest version and look at the code.