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

Generate OnIdle event only if the editing buffer is empty #2934

Merged
merged 2 commits into from
Oct 27, 2021

Conversation

daxian-dbw
Copy link
Member

@daxian-dbw daxian-dbw commented Oct 27, 2021

PR Summary

Generate OnIdle event only if the editing buffer is empty. But if there is any other event subscribers, we still run the dummy pipeline to allow potential event handling.

OnIdle event by definition is not an event that need urgent processing, and sometimes it's used by module/script to output a message, e.g. Az.Predictor uses it to write out a ask-for-survey message. Having this event to fire when the user already input something in the buffer is not a good experience for the user. See the following screenshot as an example.

image

With this PR, we generate the OnIdle event only if the editing buffer is empty. But for any other event subscribers, we still run the dummy pipeline to allow potential event processing for them even if the editing buffer is not empty.

PR Checklist

Microsoft Reviewers: Open in CodeFlow

@daxian-dbw daxian-dbw merged commit cdc164c into PowerShell:master Oct 27, 2021
@daxian-dbw daxian-dbw deleted the onidle branch October 27, 2021 20:08
@ghost
Copy link

ghost commented Oct 28, 2021

🎉 v2.2.0-beta4 has been released which incorporates this pull request. 🎉

@lzybkr
Copy link
Member

lzybkr commented Oct 28, 2021

I understand the issue you are trying to address, I'm not sure this is the right fix. For example, without PSReadLine, the experience is still ugly.

I think it'd be better to find a better solution than using OnIdle for any sort of UI experience.

@daxian-dbw
Copy link
Member Author

Right, totally agree that using OnIdle event for UI experience is far from desired. The good news is that Az.Predictor depends on PSReadLine to work :)

@lzybkr
Copy link
Member

lzybkr commented Oct 28, 2021

Fine, but this change will probably affect people not using Az.Predictor. People will randomly see their OnIdle events not firing and not understand the root cause, or waste a lot of time tracking down it down.

@daxian-dbw
Copy link
Member Author

daxian-dbw commented Oct 28, 2021

Yes, that's absolutely possible. But I think the impact should be quite low, as OnIdle event will be fired when there's a timeout and the buffer is empty, and that situation happens a lot. I will keep an eye on the feedback, and I can revert this change if there is a scenario that got blocked by this change.

@daxian-dbw
Copy link
Member Author

I also opened the doc issue MicrosoftDocs/PowerShell-Docs#8270 to have this behavior documented (not now, but I will submit a doc PR after a while if there is no feedback about this change).

@lzybkr
Copy link
Member

lzybkr commented Oct 28, 2021

I would also mention it as a low risk breaking change in the release notes for better visibility.

@daxian-dbw
Copy link
Member Author

Done, updated the GitHub release page: https://github.com/PowerShell/PSReadLine/releases/tag/v2.2.0-beta4

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.

2 participants