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: Added support for reordering items in the sidebar #11456

Merged
merged 47 commits into from Mar 15, 2023

Conversation

hecksmosis
Copy link
Contributor

@hecksmosis hecksmosis commented Feb 24, 2023

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

Validation
How did you test these changes?

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

Screenshots (optional)
Add screenshots here.

@hecksmosis
Copy link
Contributor Author

The Glyph currently used for the icon in the items needs to be changed, does anyone know of a better icon to use there?

@yaira2
Copy link
Member

yaira2 commented Feb 24, 2023

Which icon?

@yaira2
Copy link
Member

yaira2 commented Feb 24, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@yaira2 yaira2 changed the title Feature: Add move items to sidebar Feature: Added support for reordering items in the sidebar Feb 24, 2023
Copy link
Contributor

@ferrariofilippo ferrariofilippo left a comment

Choose a reason for hiding this comment

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

Everything seems to work in debug mode, but there are two problems:

  • If I run Files without debug mode, half the time I use this feature, the app crashes. Nothing useful is logged.
  • We are wasting time (and space) writing the same things in the logs twice
    1

src/Files.App/UserControls/SidebarControl.xaml.cs Outdated Show resolved Hide resolved
src/Files.App/ServicesImplementation/QuickAccessService.cs Outdated Show resolved Hide resolved
src/Files.App/UserControls/SidebarControl.xaml.cs Outdated Show resolved Hide resolved
src/Files.Backend/Services/IQuickAccessService.cs Outdated Show resolved Hide resolved
src/Files.Backend/Services/IQuickAccessService.cs Outdated Show resolved Hide resolved
src/Files.Backend/Services/IQuickAccessService.cs Outdated Show resolved Hide resolved
@hecksmosis
Copy link
Contributor Author

Everything seems to work in debug mode, but there are two problems:

  • If I run Files without debug mode, half the time I use this feature, the app crashes. Nothing useful is logged.
  • We are wasting time (and space) writing the same things in the logs twice
    1

Yes, I forgot to remove the logs, they are totally unnecessary

@hecksmosis
Copy link
Contributor Author

hecksmosis commented Feb 25, 2023

Do you have any recent folders in the quick access widget? (I haven't tried it with folders that are not pinned because I recently reset the sidebar items)

@hecksmosis
Copy link
Contributor Author

By the way, I can't reproduce the crashes on my end, any idea why they could be happening?

@yaira2
Copy link
Member

yaira2 commented Feb 26, 2023

Two things I noticed

  1. Clicking a pinned item often triggers the drag event even when it was just a regular click
  2. It's hard to drag and drop to the right area without accidentally moving the folder into one of the other pinned items. File Explorer has a visual indicator to show where the item is being dropped but I'm not sure we can do that with the sidebar.

@drasticactions
Copy link

Since you at-replied me this ended up in my email. I looked at your PR and I'm not sure you used my workaround, which may speak to @yaira2's issue.

For microsoft/microsoft-ui-xaml#3290, I attach the "DragStarting" event to the underling "NavigationViewItemPresenter". That's a "workaround" (Hack) that lets me attach the same normal events that would be allowed for Elements to the NavigationItem. You can see it implemented here,https://github.com/drasticactions/MauiFeed/blob/main/src/MauiFeed.WinUI/Views/FeedSidebarItem.cs#L237-L252

In short, you would implement it the same way you would have to NavigationViewItem, but instead attach those events to the NavigationViewItemPresenter. Eventually, when the underling issue is fixed, you should be able to add those events back to the NavigationViewItem.

@hecksmosis
Copy link
Contributor Author

Since you at-replied me this ended up in my email. I looked at your PR and I'm not sure you used my workaround, which may speak to @yaira2's issue.

For microsoft/microsoft-ui-xaml#3290, I attach the "DragStarting" event to the underling "NavigationViewItemPresenter". That's a "workaround" (Hack) that lets me attach the same normal events that would be allowed for Elements to the NavigationItem. You can see it implemented here,https://github.com/drasticactions/MauiFeed/blob/main/src/MauiFeed.WinUI/Views/FeedSidebarItem.cs#L237-L252

In short, you would implement it the same way you would have to NavigationViewItem, but instead attach those events to the NavigationViewItemPresenter. Eventually, when the underling issue is fixed, you should be able to add those events back to the NavigationViewItem.

True, I ended up not really using your workaround (sorry for the ping)

@ferrariofilippo
Copy link
Contributor

ferrariofilippo commented Feb 27, 2023

By the way, I can't reproduce the crashes on my end, any idea why they could be happening?

I have no idea why that is happening:

Crash.mp4

It seems that Favorites section is refreshed twice when you pin/unpin an item (I don't know if this behaviour is related with this PR)

@ferrariofilippo
Copy link
Contributor

Should be done now (hopefully no more bugs)

Everything is fine now 👍

@hecksmosis
Copy link
Contributor Author

hecksmosis commented Mar 15, 2023

I changed the ListViewItem to Grid and it now works fine, this is ready

@yaira2
Copy link
Member

yaira2 commented Mar 15, 2023

Should we display a progress ring and delay closing the dialog until the changes are saved?

@hecksmosis
Copy link
Contributor Author

I don't think this is necessary, this is probably not going to be used very regularly, just occasionally

@yaira2
Copy link
Member

yaira2 commented Mar 15, 2023

The reason I think it's worth doing this (it's not a blocker) is that the app freezes and it's not exactly clear to the user what's happening in the background.

@hecksmosis
Copy link
Contributor Author

Oh ok

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

@yaira2 yaira2 merged commit 02246e4 into files-community:main Mar 15, 2023
1 check passed
@hecksmosis
Copy link
Contributor Author

Should I open a PR with the progress ring?

@yaira2
Copy link
Member

yaira2 commented Mar 16, 2023

If you don't mind, thanks!

@Wolfarelli
Copy link

The reorder dialog seems to need a scroll bar for favorites lists longer than 15 items; mouse wheel doesn't work neither.
Captura de pantalla 2023-04-10 074545

@yaira2
Copy link
Member

yaira2 commented Apr 10, 2023

@Wolfarelli can you open a feature request for this?

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.

Feature: Add back ability to reorder pinned items
6 participants