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

Remove or fix "Use parent process directory" checkbox #8805

Closed
cinnamon-msft opened this issue Jan 15, 2021 · 7 comments · Fixed by #8827
Closed

Remove or fix "Use parent process directory" checkbox #8805

cinnamon-msft opened this issue Jan 15, 2021 · 7 comments · Fixed by #8827
Labels
Area-Settings UI Anything specific to the SUI Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@cinnamon-msft
Copy link
Contributor

In the main branch right now in the settings UI, there's a checkbox called "Use parent process directory" underneath Starting directory. This checkbox is intended to behave as if the Starting directory were empty. If you click the checkbox, it will empty and disable the textbox for Starting directory.

I am personally in favor of removing this checkbox and putting in the Starting directory tooltip that leaving the field blank will use the parent process directory. The checkbox adds extra confusion to the setting.

If people are adamant about keeping it, its functionality should be fixed slightly: if the Starting directory box is emptied, the checkbox should immediately be checked.

Therefore, this scenario shouldn't be possible.
image

@cinnamon-msft cinnamon-msft added the Area-Settings UI Anything specific to the SUI label Jan 15, 2021
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jan 15, 2021
@cinnamon-msft cinnamon-msft added Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. labels Jan 15, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jan 15, 2021
@carlos-zamora
Copy link
Member

Tagging @PankajBhojwani since he implemented this (PR).

@PankajBhojwani
Copy link
Contributor

PankajBhojwani commented Jan 15, 2021

Based on #8761 I thought the checkbox design was what we were going for!

Also, if you empty the textbox and hit 'Apply', the checkbox will automatically get checked - the reason it doesn't automatically get checked before you hit apply is because otherwise the user will not be able to clear out the textbox and type in a whole new path (clear out text box -> checkbox gets immediately checked -> text box gets disabled). Also, the user will not be able to uncheck the checkbox at all (uncheck check box -> textbox is empty -> check box immediately gets rechecked)

@DHowett
Copy link
Member

DHowett commented Jan 16, 2021

Right. I think we should favor more expressive settings (a checkbox for "use parent folder" instead of the empty text field means use parent folder) with some user-facing affordances for making it convenient.

Here's an idea.

The text field only emits a Change event when the user navigates off of it. Can we move focus into the text field automatically when the user UNCHECKS the box, and then CHECK the box only when the user navigates off the text field with it empty?

We can rely on the profile viewmodel's PropertyChanged notification to implement that. We have to make sure we don't move focus back into the text field unless it was explicit user action on the check, but that should be doable.

We could also make the browse button override the checkbox (choosing a folder turns off the box and fills the field) using the same notification.

how's that sound/

@PankajBhojwani
Copy link
Contributor

I like that idea, will wait on response from @cinnamon-msft before making a PR

@cinnamon-msft
Copy link
Contributor Author

:shipit:

@ghost ghost added the In-PR This issue has a related PR label Jan 19, 2021
@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jan 22, 2021
@ghost ghost closed this as completed in #8827 Jan 22, 2021
ghost pushed a commit that referenced this issue Jan 22, 2021
Make the "use parent process directory" checkbox rely on a computed
property in the ProfileViewModel. It will be enabled when the starting
directory is empty and disabled when it's not. When it's unchecked, the
last-used value will be restored. If there is no last-used value, it
will be set to %USERPROFILE%.

Closes #8805
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Jan 22, 2021
@carlos-zamora

This comment has been minimized.

@carlos-zamora carlos-zamora reopened this Jan 26, 2021
@carlos-zamora carlos-zamora removed the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Jan 26, 2021
@carlos-zamora carlos-zamora added this to the Terminal v1.7 milestone Jan 26, 2021
@DHowett
Copy link
Member

DHowett commented Jan 26, 2021

Please use a new issue for this. It was "removed or fixed" up to 1.6 standard.

@DHowett DHowett closed this as completed Jan 26, 2021
@DHowett DHowett reopened this Jan 26, 2021
@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Jan 26, 2021
@DHowett DHowett closed this as completed Jan 26, 2021
@DHowett DHowett modified the milestones: Terminal v1.7, Terminal v1.6 Jan 26, 2021
@DHowett DHowett added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Jan 26, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jan 26, 2021
mpela81 pushed a commit to mpela81/terminal that referenced this issue Jan 28, 2021
)

Make the "use parent process directory" checkbox rely on a computed
property in the ProfileViewModel. It will be enabled when the starting
directory is empty and disabled when it's not. When it's unchecked, the
last-used value will be restored. If there is no last-used value, it
will be set to %USERPROFILE%.

Closes microsoft#8805
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Settings UI Anything specific to the SUI Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants