Fix initial position parsing from -pos tag (#4247)#19409
Fix initial position parsing from -pos tag (#4247)#19409carlos-zamora merged 8 commits intomicrosoft:mainfrom
Conversation
This update modifies how the -pos tag is parsed when setting the initial window position. Specifically: - A single value like "number" now sets both x and y positions to that value — similar to how padding behaves. - A value like "number," (with a trailing comma) continues to set only x, leaving y unchanged. Changes: - Updated parsing logic for -pos tag - Added unit test to verify single-value behavior - Confirmed all existing tests pass
|
@microsoft-github-policy-service agree |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Thank you so much @carlos-zamora ! This was my first PR ever and I am beyond thrilled to have it approved. This really made my day. Looking forward to contributing and learning more. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Is there any documentation on how to run the full CI test suite locally before pushing? I ran the tests as instructed in the project README, which passed on my system, but the tests here failed. |
|
Yeah! So the problem here is that you're working with an
Yeah! So, the easiest way to run the tests is to do this in PowerShell: # in project root directory
Import-Module .\tools\OpenConsole.psm1
# Use "-Test" to choose a specific DLL to load tests
# (Optional) Add something like "-TaefArgs /name:*settings*" to run all tests with "settings" in the name (you still need -Test to pick the right dll though)
# Alternatively, just use "-AllTests" to run all of the tests, though this takes much longer
Invoke-OpenConsoleTests -Test localTerminalApp -TaefArgs /name:*settings*That said, the local tests don't actually run in CI due to a limitation with TAEF. We're supposed to run them on our own, but admittedly, we haven't done a very good job of that and some of the tests are failing 😅. Instead, I suggest adding tests to something in |
| if (initialPosition.X && !initialPosition.Y && string.find(',') == std::string::npos) | ||
| { | ||
| initialPosition.Y = initialPosition.X; | ||
| } |
There was a problem hiding this comment.
Why don't we modify ParseCommaSeparatedPair to do this for us?
There was a problem hiding this comment.
I felt that function was more general purpose and could be reused elsewhere whereas the LaunchPositionFromString function is specifically for the initialPosition argument. Shall I modify ParseCommaSeparatedPair instead?
There was a problem hiding this comment.
Sorry for the delay! Yeah, let's update ParseCommaSeparatedPair instead. This file seems to be the only place it's used. Thanks! 😊
There was a problem hiding this comment.
Thanks for confirming! I’ll update ParseCommaSeparatedPair and push the change today. Should be ready by the time you’re back online 😊
There was a problem hiding this comment.
nit: undo changes to this file
There was a problem hiding this comment.
I edited the file in GitHub’s web editor — it showed no difference at the time, but now the diff shows up anyways😅
| if (initialPosition.X && !initialPosition.Y && string.find(',') == std::string::npos) | ||
| { | ||
| initialPosition.Y = initialPosition.X; | ||
| } |
There was a problem hiding this comment.
Sorry for the delay! Yeah, let's update ParseCommaSeparatedPair instead. This file seems to be the only place it's used. Thanks! 😊
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Summary of the Pull Request
Fixes #4247. Improves consistency in -pos tag parsing by supporting single-value input for both axes.
References and Relevant Issues
#4247
Detailed Description of the Pull Request / Additional comments
The change aligns -pos behavior with padding-style logic. Existing comma-separated behavior remains unchanged.
Validation Steps Performed
PR Checklist
Update --pos tag behavior for single-value input MicrosoftDocs/terminal#890