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

[Power Rename] Select all while filter is active #24598

Merged
merged 2 commits into from
Mar 17, 2023
Merged

[Power Rename] Select all while filter is active #24598

merged 2 commits into from
Mar 17, 2023

Conversation

sosssego
Copy link
Contributor

@sosssego sosssego commented Mar 6, 2023

Summary of the Pull Request

Fix the crash that happens on PowerRename when clicking select all while filter is active (only show files that will be renamed)

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Manual Testing

@Jay-o-Way Jay-o-Way added the Product-PowerRename Refers to the PowerRename PowerToy label Mar 7, 2023
@sosssego sosssego changed the title Check if the result from find item returns a valid item [Power Rename] Select all while filter is active Mar 8, 2023
@sosssego sosssego marked this pull request as ready for review March 8, 2023 00:20
@sosssego sosssego requested a review from stefansjfw March 9, 2023 15:31
@@ -738,7 +740,8 @@ namespace winrt::PowerRenameUI::implementation
int id = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

even better, instead of iterating here over all elements (GetItemCount), you can iterate over m_explorerItemsMap (or m_explorerItems), and check/unchech those items, as these collections are updated on ShowAll/ShowRenamed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current ToggleAll function changes the PowerRenameItem selected and the ExplorerItem selected so I thought it would make it more complex to run the loop twice.

Copy link
Collaborator

Choose a reason for hiding this comment

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

not running the loop twice. Just loop through m_explorerItemsMap and then you can get PowerRenameItem with PRManger->GetById and change it that way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It also needs to go through the ones in m_prManager that are not in m_explorerItemsMap to change its select state.

Copy link
Collaborator

@stefansjfw stefansjfw Mar 15, 2023

Choose a reason for hiding this comment

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

There are 2 options here when showing only the files that will be renamed and clicking Select All/Unselect All:

  1. Select All/Unselect All for all items that are loaded into PowerRename
  2. Select All/Unselect All only the items that can be renamed

Current behavior is 1 I think, right? But I'd prefer to go with 2, as it makes more sense from UX side. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it selects/unselects everything loaded in powerRename like in option 1.
for (auto& [key, value] : m_explorerItemsMap) { value.Checked(selected); }
https://user-images.githubusercontent.com/14083829/225715805-85d1c2a0-5415-467b-bc85-fa66acecb4c0.mp4

Option 2 may allow unchecking only what is being renamed, and selecting and unselecting it.
for (UINT i = 0; i < itemCount; i++) { CComPtr<IPowerRenameItem> spItem; if (SUCCEEDED(m_prManager->GetItemByIndex(i, &spItem))) { spItem->PutSelected(selected); int id = 0; spItem->GetId(&id); auto item = FindById(id); if (item) item.Checked(selected); } }
https://user-images.githubusercontent.com/14083829/225715823-7fc157a5-bbc8-49fe-a4f2-ed7f27e1505e.mp4

But sometimes It feels not intuitive.
I still prefer option 1, check the vídeos.

when showRenamed is checked.
@crutkas
Copy link
Member

crutkas commented Mar 16, 2023

@sosssego is this ready for another round of reviews?

Copy link
Collaborator

@stefansjfw stefansjfw left a comment

Choose a reason for hiding this comment

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

Crash fix looks good! Let's merge. We can re-think the Select/Unselect all logic as a follow-up

@stefansjfw stefansjfw merged commit 9d73734 into microsoft:main Mar 17, 2023
BLM16 pushed a commit to BLM16/PowerToys that referenced this pull request Jun 22, 2023
* Check if the result from find item returns a valid item
and repopulate ExplorerItems()

* Call all the clean and populate methods only
when showRenamed is checked.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Product-PowerRename Refers to the PowerRename PowerToy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants