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

Fix: Fixed issue where you couldn't select files in columns layout #12124

Merged

Conversation

ferrariofilippo
Copy link
Contributor

Resolved / Related Issues

  • Were these changes approved in an issue or discussion with the project maintainers? In order to prevent extra work, feature requests and changes to the codebase must be approved before the pull request will be reviewed. This prevents extra work for the contributors and maintainers.
    Closes Bug: Can't select some files in column view #12090

Description

  • I simplified the code to find the last column that should remain open (I don't know why I wrote that complex code in the first place). Now it looks for the blade with the same path as the requested one.
  • As mentioned in Feature: Close unfocused blades #11898, there's a bug with double click to open

Notes

  • Please, test these changes deeply since they may cause many regressions
  • It may happen that the user creates a loop of columns (like: C:\Users\username\Desktop\shortcut_to_Users\username\Desktop\something_else). In this case, if the user clicks an item in Desktop, should we always close all columns after the first Desktop?

Validation
How did you test these changes?

  • Did you build the app and test your changes?
  • Did you check for accessibility? You can use Accessibility Insights for this.
  • Did you remove any strings from the en-us resource file?
    • Did you search the solution to see if the string is still being used?
  • Did you implement any design changes to an existing feature?
    • Was this change approved?
  • Are there any other steps that were used to validate these changes?
    Test 1
    1. Open app and a folder using colums view
    2. Use Open folders with single click in columns view
    3. Open a shortcut to a folder (try with a shortcut to a sub-subfolder of the current directory, a shortcut to a parent folder of the current and with one to a random folder)
    4. Select a file
      Test 2
    5. Repeat all the steps above using double click to open
    6. Navigate between folders using Up/Down arrows to ensure there's no regression from Fix: Fixed issue where column view jumps from left to right #11986
      Test 3
    7. Open a path such as C:\Windows\System32 in columns layout
    8. Click on Windows (Single click to open option) (Check that there's no regression from Feature: Close unfocused blades #11898)

@yaira2
Copy link
Member

yaira2 commented Apr 19, 2023

@ferrariofilippo can you target the servicing branch?

@ferrariofilippo ferrariofilippo changed the base branch from main to service/2.4.66 April 19, 2023 20:55
@hishitetsu
Copy link
Member

It may happen that the user creates a loop of columns

It is the same with Finder.
image

Copy link
Member

@hishitetsu hishitetsu left a comment

Choose a reason for hiding this comment

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

It doesn't work well if looping shortcuts.

2023-04-20.124352.mp4

@ferrariofilippo
Copy link
Contributor Author

Do you have double click to open? In that case I know that bug (is the one I mentioned in the description) and I'm working on a fix

@hishitetsu
Copy link
Member

Do you have double click to open? In that case I know that bug (is the one I mentioned in the description) and I'm working on a fix

"Open items with a single click" is off, "Open folders with a single click in the Columns Layout" is on.
If the shortcut was not looped, the file could be opened without problems.

@ferrariofilippo
Copy link
Contributor Author

ferrariofilippo commented Apr 20, 2023

"Open items with a single click" is off, "Open folders with a single click in the Columns Layout" is on. If the shortcut was not looped, the file could be opened without problems.

Ok, I just understand what you meant. I mentioned that in the Notes

It may happen that the user creates a loop of columns (like: C:\Users\username\Desktop\shortcut_to_Users\username\Desktop\something_else). In this case, if the user clicks an item in Desktop, should we always close all columns after the first Desktop?

At the moment we can't know which column the user has clicked (that's an issue when we have three loops). Because of this, we have to choose between leaving open the first or the last occourence of the path
I'll check if I can find a way to change the way we handle taps so that we can know which column has been clicked

This should be fixed now (I tested it only quickly)
Now I'll focus on the double click bug

@ferrariofilippo
Copy link
Contributor Author

Everything should be fine now

Comment on lines 456 to 458
if (relativeIndex is -1)
while (relativeIndex < ColumnHost.ActiveBlades.Count &&
(!column.NavPathParam?.Equals(GetWorkingDirOfColumnAt(++relativeIndex)) ?? false)) ;
Copy link
Member

Choose a reason for hiding this comment

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

This code is confusing and should not be handled only by the conditional clause of the while loop.
Doesn't this cause an infinite loop when NavPathParam is null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change that but it will not cause an infinite loop because of the first condition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I'm not sure how to make it more readable. Should I use a do-while?

Copy link
Member

@hishitetsu hishitetsu left a comment

Choose a reason for hiding this comment

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

File selection appears to work fine.
There is still a little weird behavior with folder selection when columns are looping, but it's a different issue and much better than not being able to open files.

@ferrariofilippo
Copy link
Contributor Author

There is still a little weird behavior with folder selection when columns are looping, but it's a different issue and much better than not being able to open files.

I can't reproduce it. Can you tell me more, please? (single or double click to open?)

@hishitetsu
Copy link
Member

I can't reproduce it. Can you tell me more, please? (single or double click to open?)

Look at this video.

2023-04-21.155207.mp4

@ferrariofilippo
Copy link
Contributor Author

ferrariofilippo commented Apr 21, 2023

Look at this video.

I can't reproduce it and, looking at the scrollbar on the bottom, it seems to work.
Do you have a loop like Documents/Downloads/Documents/Downloads/Documents? In that case clicking on the second Documents will close the following blades, but the view will also scroll and it will look like it's opening from the last column.
Can you confirm this?

@hishitetsu
Copy link
Member

I can't reproduce it and, looking at the scrollbar on the bottom, it seems to work.

Sorry, it is an oversight on my part. It works fine.

Copy link
Member

@hishitetsu hishitetsu 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 Apr 21, 2023

@ferrariofilippo @hishitetsu thank you for your work on this fix!

@yaira2 yaira2 merged commit bdc1b68 into files-community:service/2.4.66 Apr 21, 2023
1 check passed
@yaira2 yaira2 changed the title Fix: Fixed cannot select Files in columns view Fix: Fixed issue where you couldn't select files in columns view Apr 21, 2023
@yaira2 yaira2 changed the title Fix: Fixed issue where you couldn't select files in columns view Fix: Fixed issue where you couldn't select files in columns layout Apr 21, 2023
@ferrariofilippo ferrariofilippo deleted the Fix_Columns_Layout branch April 21, 2023 13:21
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.

Bug: Can't select some files in column view
3 participants