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

#10 generation skipping some characters #20

Conversation

kengoodridge
Copy link
Contributor

@kengoodridge kengoodridge commented Feb 2, 2024

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

@kengoodridge kengoodridge changed the title Issue10 generation skipping some characters #10 generation skipping some characters Feb 2, 2024
@jasonjmcghee
Copy link
Owner

I will review this soon- super excited

@kengoodridge
Copy link
Contributor Author

@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
Copy link
Owner

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
Copy link
Owner

@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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Owner

jasonjmcghee commented Feb 2, 2024

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?

src-tauri/src/main.rs Outdated Show resolved Hide resolved
@jasonjmcghee
Copy link
Owner

I reproduced it with your changeset:

Screen.Recording.2024-02-01.at.6.29.43.PM.mov

@jasonjmcghee
Copy link
Owner

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

@jasonjmcghee
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link

kgoodridge commented Feb 2, 2024

@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
Copy link
Contributor Author

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

@jasonjmcghee jasonjmcghee merged commit 7f41a06 into jasonjmcghee:main Feb 3, 2024
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.

None yet

3 participants