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: Show folder details when no item is selected #12549

Merged

Conversation

QuaintMako
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 Feature: Show folder details when no item is selected #6724

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?
    1. Open app ...
    2. Click settings button ...

Screenshots
6724

@yaira2
Copy link
Member

yaira2 commented Jun 6, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@hishitetsu
Copy link
Member

  • When the navigation back button is pressed, the originally opened folder is selected, but the preview shows the current folder.
  • Behavior in the column layout is unstable.
2023-06-06.234904.mp4

@yaira2 yaira2 changed the title Feature: Preview Pane now shows the current folder details when no selection Feature: Show folder details when no item is selected Jun 8, 2023
@yaira2
Copy link
Member

yaira2 commented Jun 8, 2023

This isn't working on the drive root

@QuaintMako
Copy link
Contributor Author

I will be able to address the shortcomings found tomorrow during the day.

@QuaintMako
Copy link
Contributor Author

QuaintMako commented Jun 9, 2023

This isn't working on the drive root

This is now working, but I believe we should go a bit further and display other informations. Date Modified and Date Created do not make sense here. It would be better to display the information from the general properties panel. Should we do it in this PR or another one?

image

@yaira2
Copy link
Member

yaira2 commented Jun 9, 2023

Can we display the drive name?

@QuaintMako
Copy link
Contributor Author

QuaintMako commented Jun 11, 2023

The navigation in ColumnView should now be stabler.

I still need to take care of:

  • Backward navigation
  • Displaying the name of the drive.

@yaira2 if you wish to have the feature in the next stable, maybe the current state can be enough has it is now. I do not think I'll have time today to address the two remaining issues.

On the other hand, while the initial feature is completed, it showed some shortcomings that could use some polish.

I'll let you decide whatever side you prefer. And in any case, I think I'll skip displaying the drive name and go straight for a specific drive display in preview. It will be much cleaner.

@yaira2
Copy link
Member

yaira2 commented Jun 11, 2023

if you wish to have the feature in the next stable, maybe the current state can be enough has it is now. I do not think I'll have time today to address the two remaining issues.

I'd rather wait, this change will have more of an impact when it's polished.

@QuaintMako
Copy link
Contributor Author

  • Backward navigation now focuses the selection as expected.
  • Preview in Column View is now much stabler.

@yaira2 with your permission I'll leave the display of the root drive as is and start working on a dedicated preview controller for it to save some dev time.

@yaira2
Copy link
Member

yaira2 commented Jun 13, 2023

Thanks

@hishitetsu
Copy link
Member

Backward navigation is improved, but preview in Column View seems still unstable.

@QuaintMako
Copy link
Contributor Author

Backward navigation is improved, but preview in Column View seems still unstable.

Can you give me cases you're using to make it unstable?

@hishitetsu
Copy link
Member

hishitetsu commented Jun 14, 2023

I can reproduce the strange behavior by following the steps below.

  1. Open C: drive in the column layout.
  2. Select and open User folder.
  3. Select and open Public folder.
  4. Click the first column (C:). User is selected and the preview shows User.
  5. Click the second column (User). Public is selected but the preview shows C:.

@QuaintMako
Copy link
Contributor Author

It might be linked to an asynchronous issue leading to a bad timing. The issue doesn't appear when break points are used.

@QuaintMako
Copy link
Contributor Author

This time it should be good!

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.

Great! It seems to work no longer a problem.
One minor point I would like to comment on.

src/Files.App/Views/LayoutModes/BaseLayout.cs Outdated Show resolved Hide resolved
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 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Jun 16, 2023
@yaira2 yaira2 merged commit ec5c6e6 into files-community:main Jun 16, 2023
@hishitetsu
Copy link
Member

It seems to cause a crash when opening the home page with both the dual pane and preview pane enabled.

@QuaintMako
Copy link
Contributor Author

It seems to cause a crash when opening the home page with both the dual pane and preview pane enabled.

I can have a look into it tonight if needed

@hishitetsu
Copy link
Member

I can have a look into it tonight if needed

Please.

@QuaintMako
Copy link
Contributor Author

@hishitetsu I'm not able to reproduce the crash. Tried with both the dual pane and the preview pane, with several different views, with and without selection etc... Could you tell me what is your set-up precisely? What kind of folders you're in, what layouts, with or without selection etc...

@hishitetsu
Copy link
Member

Sorry, the crash caused by #12370. I mistakenly thought that the preview pane was the cause.

@QuaintMako QuaintMako deleted the 6724_ShowFolderWhenNoSelectionPreview branch June 29, 2023 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Show folder details when no item is selected
3 participants