-
Notifications
You must be signed in to change notification settings - Fork 2
feat: Add alternative confirmation dialog using QuickPick (Enable with enableAlternateConfirmation) #46
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
Conversation
|
Hi @alilaa! Thanks for opening this PR. What was the issue mentioned where the dialog fails on longer paths? I haven't tested very long paths but if their is a known issue with just the dialog with longer paths, that would be good to note as an issue as well. Possibly this is a platform specific bug. I mainly use a Mac and occasionally a Ubuntu machine so a Window bug could easily get by me if its not caught by the tests. The dialogs are not able to be tested in the automated tests so that would have to be something that I occasionally test on different platforms. I'm not sure I love the idea of using the quick pick for the confirmation. It could be a little confusing because usually a quick pick would be fore selecting a single action to perform. If you do select or filter the options and confirm the changes, all of the changes are still performed. It also looks like the quick pick doesn't prevent further actions/typing in the background and that could be a confusing experience. I don't love the existing use of the dialog so something like the change here might be a good alternative option that could be opted into with a configuration flag. The existing dialog works with enter/esc for me. Ideally we would have a Y/N confirmation like the original oil.nvim plugin. I'm not sure if that is something that could be added to the dialogs or if the quick pick would support something like that. Let me know your thoughts and I will play around with these changes on my local machine. |
|
I played with it some more after the latest changes and their are a lot of things that I like about this. I am hesitant to make this the default but I can see some value in making this an experimental feature that can be opted into. If you want to make those changes let me know otherwise I can take that on. I also looked at the original issue some more and the good/bad news is that it only impacts Windows OS. My testing on MacOS and Ubuntu showed that the text for long file paths wraps in the warning dialog. I am looking at a standalone fix for that problem that but that hopefully wouldn't really impact the changes here. |
|
Ah, that explains why it went unnoticed. Then an opt in feature seems like the way to go. I can update the PR, any preference for the name of the setting? |
I think something like "enableAlternateConfirmation" or "enableExperimentalConfirmation" would work. |
|
I have updated the code accordingly, should we keep the quickpick mock, it wont be needed since testing is performed with setting off. |
Yeah, good question. I think it's safe to remove the mock for now. Ideally I'll find a way to test the two flows but given it's a mock either way, we can stick to the single test path. |
corwinm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really great. I'm excited to have this as an option!
Fix linting
Done. Let me know if there is anything else. |
…h enableAlternateConfirmation) (#46)




Title
Use QuickPick for change confirmation
Summary
Replaces modal confirmation with a QuickPick that lists pending file operations and applies on Enter, improving UX.
Changes
Changetype for create/delete/move/rename/copy/modifyconfirmChanges()renders a read-only QuickPick with icons, relative paths, Apply/Cancel buttons, Enter=Apply, Esc/blur=CancelChange[], formats paths viaformatPath, and gates execution onconfirmChanges(...)vscode.window.createQuickPickto auto-accept for deterministic runsshowWarningMessagestub for legacy error dialogs0.0.31Why
Behavior
Verification
Files changed