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

Feature: Use the tab key to select the next file while renaming #11063

Merged
merged 14 commits into from Feb 1, 2023

Conversation

ferrariofilippo
Copy link
Contributor

@ferrariofilippo ferrariofilippo commented Jan 23, 2023

Resolved / Related Issues
Items resolved / related issues by this PR.

Comments
There's a bug when you're at the beginning of a word and press Down Arrow. Nothing happens (also when you're at the end and press Up Arrow). Neither KeyDown nor PreviewKeyDown events are raised. GridView layout is not affected by the bug (but Tiles is)
I'll try to find a solution

Validation
How did you test these changes?

  • Built and ran the app
  • Tested the changes for accessibility

QuaintMako
QuaintMako previously approved these changes Jan 23, 2023
Copy link
Contributor

@QuaintMako QuaintMako left a comment

Choose a reason for hiding this comment

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

LGTM and works on my machine.

I do have the issue where the down arrow doesn't do anything when at the start of the name as well. It'd be nice to fix it indeed.

@yaira2
Copy link
Member

yaira2 commented Jan 24, 2023

I think key + up/down should finish the rename and start renaming the item above/below it.
Actually, that's tab/shift tab

@ferrariofilippo
Copy link
Contributor Author

Actually, that's tab/shift tab

Added. I had to use User32.dll to get Shift state because InputKeyboard.GetKeyStateForCurrentThread() caused some issues

@ferrariofilippo
Copy link
Contributor Author

I do have the issue where the down arrow doesn't do anything when at the start of the name as well.

Ctrl+Up/Down arrow works. This may be a WinUI bug as it involves all textboxes (e.g. new file modal or properties window)

@yaira2
Copy link
Member

yaira2 commented Jan 25, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ferrariofilippo
Copy link
Contributor Author

ferrariofilippo commented Jan 25, 2023

I changed CommitRename from async void to async Task to avoid a bug where files/folders got duplicated.
I need to test this new feature further, but it seems to work.

Edit. Actually there are two bugs:

  • when items are reordered rename doesn't work
  • Shift+Tab when you change completely the name bugs the whole app

@ferrariofilippo
Copy link
Contributor Author

ferrariofilippo commented Jan 26, 2023

There's a problem with ListViewBase.ContainerFromItem() which sometimes returns null even when the item exists. Once I figured out how to fix this, there should be no more bugs.
Edit. Happens only in DetailsLayout

@ferrariofilippo
Copy link
Contributor Author

Everything should work now

QuaintMako
QuaintMako previously approved these changes Jan 30, 2023
Copy link
Contributor

@QuaintMako QuaintMako left a comment

Choose a reason for hiding this comment

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

LGTM and works for me.

hecksmosis
hecksmosis previously approved these changes Jan 30, 2023
Copy link
Contributor

@hecksmosis hecksmosis left a comment

Choose a reason for hiding this comment

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

LGTM

@yaira2
Copy link
Member

yaira2 commented Feb 1, 2023

@ferrariofilippo can you confirm this resolves #6675?

@yaira2 yaira2 changed the title Fix: Fixed issue with using arrow keys while renaming a file Feature: Use the tab key to select the next file while renaming Feb 1, 2023
@ferrariofilippo
Copy link
Contributor Author

Yes

@yaira2 yaira2 dismissed stale reviews from hecksmosis and QuaintMako via 9ca0cda February 1, 2023 21:15
Copy link
Member

@yaira2 yaira2 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@hecksmosis hecksmosis left a comment

Choose a reason for hiding this comment

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

LGTM and seems to work

@yaira2 yaira2 merged commit 9782b85 into files-community:main Feb 1, 2023
@yaira2
Copy link
Member

yaira2 commented Feb 1, 2023

I tested, it not only seems to work but it's a great improvement 😅

@ferrariofilippo ferrariofilippo deleted the New_Rename_Items branch February 1, 2023 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants