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

Code Quality: Refactored the code for Quick Access #16485

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

0x5bfa
Copy link
Member

@0x5bfa 0x5bfa commented Nov 19, 2024

Resolved / Related Issues

Steps used to test these changes

  • See Quick Access widgets populate items
  • See Pinned section in SidebarView populate items
  • Unpin an item
  • Pin an item
  • Reorder items and save

@0x5bfa 0x5bfa force-pushed the 5bfa/CQ-QuickAccessRefactor branch from 0c973ed to 2ebb917 Compare November 20, 2024 23:49
@0x5bfa 0x5bfa marked this pull request as ready for review November 28, 2024 17:16
@yaira2 yaira2 self-requested a review November 28, 2024 21:30
@@ -96,7 +96,8 @@ protected override void OnNavigatedTo(NavigationEventArgs eventArgs)

if (!string.IsNullOrEmpty(pathRoot))
{
var rootPathList = App.QuickAccessManager.Model.PinnedFolders.Select(NormalizePath)
var service = Ioc.Default.GetRequiredService<IQuickAccessService>();
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the service be defined at the top of the class?

Copy link
Member

Choose a reason for hiding this comment

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

Please add comments

@0x5bfa 0x5bfa marked this pull request as draft December 10, 2024 00:18
@yaira2 yaira2 changed the title Code Quality: Improved the complicated quick access feature Code Quality: Refactored the code for Quick Access Dec 11, 2024
Task SaveAsync(string[] items);
IReadOnlyList<INavigationControlItem> Folders { get; }

event EventHandler<NotifyCollectionChangedEventArgs>? PinnedFoldersChanged;
Copy link
Member

Choose a reason for hiding this comment

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

Is this event limited to pinned folders, or does it also notify when recent folders are added to the list?

@yaira2
Copy link
Member

yaira2 commented Dec 15, 2024

The most recent test I ran on this branch had an issue where pinned folders weren't displaying in the sidebar.

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

0x5bfa commented Dec 15, 2024

This isn't really ready for review yet. I need to make the update pinned files async to return the pinned files and create a file notifier for the pinned files folder to use it in sidebar and the widget.

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: Pinned items don't show when Files is set to stay open in the background
2 participants