Script user input improvement #11281
Conversation
GitUI/CommandsDialogs/SettingsDialog/Pages/ScriptsSettingsPage.cs
Outdated
Show resolved
Hide resolved
GitUI/CommandsDialogs/SettingsDialog/Pages/ScriptsSettingsPage.cs
Outdated
Show resolved
Hide resolved
d735064 to
849353d
Compare
46aaf55 to
394ae7f
Compare
|
I think I fixed all the points raised. Changes undo a lot of changes so I have rewritten the history (sorry) so that it's clearer now. @mstv "multiple use of one input value" is now working as you expect. @mstv & @RussKie I tried to implement the syntax |
bff98e0 to
098f919
Compare
098f919 to
29d2b40
Compare
29d2b40 to
b979616
Compare
There was a problem hiding this comment.
The parser seems to work well. 👍
Cancellation of the input dialog should throw a (silent) OperationAbortedException.
Esc should deselect the selection if any, else cancel the input.
The label should be used as label (perhaps with ":" appended), but not as caption.
The caption should rather be User input for script {name}.
a087ed2 to
dd79aeb
Compare
Done. |
669e8bf to
3b27632
Compare
| DialogResult result = prompt.ShowDialog(owner); | ||
| if (result != DialogResult.OK) | ||
| { | ||
| throw new OperationCanceledException(); |
There was a problem hiding this comment.
😨 Why are we throwing here?
It looks as though we're using exceptions for the flow control, which is a code smell.
Throwing exceptions is quite expensive, and they represent an exceptional state, which user cancelling the dialog isn't.
There was a problem hiding this comment.
Throwing exceptions is quite expensive
Performance doesn't matter here because the exception is thrown only one time to display a user popup.
and they represent an exceptional state, which user cancelling the dialog isn't.
I think the cancelling of the dialog is exceptional otherwise we would have already an issue opened because the error popup is absolutely not good 🤣
I really think a refactoring here is overkill as the method is returning at the moment only a value and we will have to refactor it to return also a state enum (Success/Abort/UserAbort), handle well this state for all arguments and handle it in the calling code. I predict more than 100 lines of new code and changes just to handle the case of 2 arguments UserFile and UserInput that ask inputs from user and that a bad/no input should abort the script. That's the handling of these 2 arguments and the possibility to abort which is "exceptional" 😉
I really don't want to go this way....
| DialogResult result = prompt.ShowDialog(owner); | ||
| if (result == DialogResult.Cancel) | ||
| { | ||
| throw new OperationCanceledException(); |
| catch (OperationCanceledException) | ||
| { | ||
| MessageBox.Show(owner, TranslatedStrings.ScriptUserCanceledRun, TranslatedStrings.ScriptText, MessageBoxButtons.OK, MessageBoxIcon.Information); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Because otherwise the user have a frightening error message dialog that absolutely doesn't correspond to the reason why we abort the launch of the script:
There was a problem hiding this comment.
To me this is a confirmation that we're doing it incorrectly :)
I think this is a misplaced responsibility, please have a look at 4b772c4 (disclaimer: I just moved code around without running it).
There was a problem hiding this comment.
@RussKie Not really convinced but OK for me. Have a look at other changes required by this solution and provide feedback before I spend time to adapt tests (because with this solution, it's now less straightforward and I don't want to spend too much time on it if later adaptations are needed).
There was a problem hiding this comment.
@RussKie Friendly reminder 😉
|
Apologies, the rl has taken priority. I'll try to look into it early next
week.
|
94d2266 to
12a2088
Compare
UnitTests/GitUI.Tests/Script/ScriptsManagerScriptRunnerTests.cs
Outdated
Show resolved
Hide resolved
12a2088 to
903c2cd
Compare
by having the possibility to provide the popup a meaningful title
and a default value (that could be an argument also)
Note: UserInput with arguments replace only one specific occurrence
contrary to `{UserInput}` that ask user for a value and replace all tokens with the value provided
Example: :
* hard-coded label and default value: `{UserInput:user label=a default value}`
* hard-coded label and no default value: `{UserInput:user label}`
* use an existing token as (part of ) a default value: `{UserInput:Bundle filename={cLocalBranch}.bundle}`
* Replace all user inputs with same label like in: "{UserInput:input label1=value}_{UserInput:input label1}_{{UserInput:input label1}}"
+ add test for {UserInput} token (and similar {UserFiles} token)
that need to introduce ISimplePromptCreator
that takes care of creating the dialog
to be able to fake it in unit tests
+ SimplePrompt UX improvements
* Script run canceled if user close the form (with a friendly "Script run canceled by user." information popup)
* User is able to close the form with Esc key (1st press deselect the selection if any)
* label defined by user is no more displayed in the form caption but on the input label
* Caption display the name of the script run: "User input for script '{name}'"
+ Move handling of {UserInput} and {UserFiles} tokens, that could be cancelled by user,
earlier in the argument processing (in ScriptManager.ScriptRunner)
& handle difference between user cancel and abort for parsing error
903c2cd to
a1bc8a6
Compare
Proposed changes
In scripts command line arguments, the user is allows to:
Example:
bundle create {{UserInput:Bundle filename|{cLocalBranch}.bundle|}} -{UserInput:Nb commits||} {cLocalBranch}2 inputs will be requested.
Screenshots
Before
After
Test methodology
Test environment(s)
Merge strategy
I agree that the maintainer Rebase merge this PR (if the commit message is clear).
✒️ I contribute this code under The Developer Certificate of Origin.