Skip to content

Fix: Fixed issue where drive folders in sidebar have wrong properies category #14540

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

Closed
wants to merge 4 commits into from
Closed

Conversation

ZTL-UwU
Copy link
Contributor

@ZTL-UwU ZTL-UwU commented Jan 24, 2024

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: Inconsistency of properties window when drives are pinned #14530

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. pin a drive to the side bar
    2. right click to open the properties window

Screenshots (optional)
image

@ZTL-UwU ZTL-UwU marked this pull request as draft January 24, 2024 01:38
@ZTL-UwU ZTL-UwU marked this pull request as ready for review January 24, 2024 05:53
@ZTL-UwU
Copy link
Contributor Author

ZTL-UwU commented Jan 24, 2024

This does NOT fix #14522, this only fixes the category issue in the Pinned items which is #14530. The Cloud Drive Section should be a separate fix.

@ZTL-UwU ZTL-UwU marked this pull request as draft January 24, 2024 10:33
@ZTL-UwU ZTL-UwU marked this pull request as ready for review January 24, 2024 13:56
@@ -202,6 +202,14 @@ public object ToolTip
{
get
{
if (Section == SectionType.Favorites)
Copy link
Member

Choose a reason for hiding this comment

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

What's this for?

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's for showing the pinned icon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1706136177063~2.png

Copy link
Member

Choose a reason for hiding this comment

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

Isn't that already handled by the sidebar?

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 believe the sidebar only shows the icon defined by the pinned item. The other items have the icon because it is defined by locationItem.

@ZTL-UwU ZTL-UwU requested a review from yaira2 January 25, 2024 07:47
@yaira2
Copy link
Member

yaira2 commented Jan 28, 2024

@ZTL-UwU can you clarify what issue this is trying to solve?

@ZTL-UwU
Copy link
Contributor Author

ZTL-UwU commented Jan 29, 2024

All the pinned items were stored as locationItems, which makes the categories of all the property window folders. See the screenshots in #14530.

This PR allows the pinned items to be driveItems as well, which fixes the property window for both pinned drives and pinned cloud drives.

@yaira2

Copy link
Member

@0x5bfa 0x5bfa left a comment

Choose a reason for hiding this comment

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

First, properties window shows correct information and fixed the issue.

However, this breaks the order of favorites, pinning/unpinning drives and brings instability of favorites section.
I recommend to investigate, re-create or abandon.

I left some reviews regarding codebase quality.

Comment on lines +17 to +19
private static DrivesViewModel DrivesViewModel { get; } = Ioc.Default.GetRequiredService<DrivesViewModel>();
private static NetworkDrivesViewModel NetworkDrivesViewModel { get; } = Ioc.Default.GetRequiredService<NetworkDrivesViewModel>();
private static readonly ICloudDetector cloudDetector = Ioc.Default.GetRequiredService<ICloudDetector>();
Copy link
Member

Choose a reason for hiding this comment

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

Practically we use private property for DI. Change as below:

private NetworkDrivesViewModel NetworkDrivesViewModel { get; } = Ioc.Default.GetRequiredService<NetworkDrivesViewModel>();
private DrivesViewModel DrivesViewModel { get; } = Ioc.Default.GetRequiredService<DrivesViewModel>();
private ICloudDetector CloudDetector { get; } = Ioc.Default.GetRequiredService<ICloudDetector>();

/// <summary>
/// Adds the item (from a path) to the navigation sidebar
/// </summary>
/// <param name="path">The path which to save</param>
/// <returns>Task</returns>
public async Task AddItemToSidebarAsync(string path)
{
var locationItem = await CreateLocationItemFromPathAsync(path);
INavigationControlItem? item;
if (PathNormalization.NormalizePath(PathNormalization.GetPathRoot(path)) == PathNormalization.NormalizePath(path) || (await cloudDetector.DetectCloudProvidersAsync()).Any(x => x.SyncFolder == path)) // Is drive or cloudDrive
Copy link
Member

Choose a reason for hiding this comment

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

Don't use brackets-omitted if-else statement.

  • Add curly brackets.
  • Split those two conditions into two lines.

@0x5bfa 0x5bfa added changes requested Changes are needed for this pull request and removed needs - code review labels Feb 15, 2024
@0x5bfa
Copy link
Member

0x5bfa commented Feb 22, 2024

Thank you for your contribution. This idea has not been planned and not been expected. Closing for now and can you describe how to fix this in the issue? And I think this issue can be fixed with the cloud one.

@0x5bfa 0x5bfa closed this Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes requested Changes are needed for this pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Inconsistency of properties window when drives are pinned
3 participants