Skip to content

Conversation

@ryzngard
Copy link
Contributor

@ryzngard ryzngard commented Jun 8, 2022

Fixes #60294

The flyout view model isn't recreated, so if the session options change in some way (such as being reset) it won't get updated between showing/hiding. This fixes so there's no intermediate variables and binds directly to the session options.

@ryzngard ryzngard requested a review from a team as a code owner June 8, 2022 21:14
@ryzngard ryzngard requested a review from a team June 8, 2022 21:14
@ghost ghost added the Area-IDE label Jun 8, 2022
@ryzngard ryzngard changed the title Use session options for all checkboxes directly Use inline rename session options for all checkboxes directly Jun 8, 2022
private void ComputeRenameFile()
{
// If replacementText is invalid, we won't rename the file.
RenameFileFlag = _isReplacementTextValid && AllowFileRename && _session.Options.RenameFile;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the check for "thing being renamed is actually a class that represents the file name"? Is that part of the InlineRenameFileRenameInfo.Allowed check on line 49?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. That portion is calculated based on the session start

Copy link
Contributor

@sharwell sharwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would generally expect the check boxes to not persist if the user dismisses the dialog with Esc. Is there a reason this UI needs to be treated differently from other UIs in handling of this key?

The flyout view model isn't recreated

I believe it's trivial to reassign the data context at the time the UI is going to be shown. This allows for explicit control of save/discard.

@ryzngard
Copy link
Contributor Author

ryzngard commented Jun 9, 2022

I would generally expect the check boxes to not persist if the user dismisses the dialog with Esc. Is there a reason this UI needs to be treated differently from other UIs in handling of this key?

The flyout view model isn't recreated

I believe it's trivial to reassign the data context at the time the UI is going to be shown. This allows for explicit control of save/discard.

Looking deeper into the issue, I conflated two things and misspoke. The ViewModel is binding to a new session each time, so it has to be recreated. The issue was that the session options are stored and loading across sessions. We could address this in the ViewModel or at time of session creation to reset the file rename option.

As for escape behavior, this follows what the current inline rename experience does. I'd prefer to have a second issue and follow up if we want a behavior change here. As of now that should not block this fix.

@ryzngard ryzngard requested a review from sharwell June 10, 2022 18:59
@ryzngard ryzngard dismissed sharwell’s stale review June 14, 2022 22:43

Dismissing for now. If we need these changes it should be a follow up PR

@ryzngard ryzngard merged commit 4b4dcaa into dotnet:main Jun 14, 2022
@ryzngard ryzngard deleted the issues/60294_rename_flyout_checkboxes branch June 14, 2022 22:54
@ghost ghost added this to the Next milestone Jun 14, 2022
@RikkiGibson RikkiGibson modified the milestones: Next, 17.3 P3 Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

New rename dialog always renames file once checkbox is selected

6 participants