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.

Sign up for GitHub

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

Jump to bottom

Code Quality: Use view models in Widgets #14926

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Code Quality: Use view models in Widgets #14926

wants to merge 18 commits into from

Conversation

0x5bfa
Copy link
Member

@0x5bfa 0x5bfa commented Mar 9, 2024 •
edited

Summary

Stage1 and stage2. I had to do stage2 in this PR.

PR Checklist

  • 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.
    Related Code Quality: Introduce view models to widget controls #14760
  • 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. Go to Home page
    2. Check if widget will be shown if you enabled the widget in the Settings. Also, check if the setting is retained even if you open a new tab with Home page.
    3. Right click on a card and check if context menu will be shown.
    4. Click/middle-click on a card and check if the page will be navigated.

Screenshots

None

@0x5bfa 0x5bfa marked this pull request as draft March 9, 2024 15:22
@0x5bfa 0x5bfa marked this pull request as ready for review March 10, 2024 14:57
@0x5bfa 0x5bfa marked this pull request as draft March 14, 2024 03:29
@0x5bfa 0x5bfa changed the title Code Quality: Use view models in Widgets (post v3.3) Code Quality: Use view models in Widgets Mar 22, 2024
@0x5bfa 0x5bfa marked this pull request as ready for review March 22, 2024 04:48
@yaira2
Copy link
Member

yaira2 commented Mar 25, 2024

Code wise it looks good (hopefully @d2dyno1 can look it over as well since he did a lot of the original work on this) but I want to test it before merging. Which steps were taken to validate these changes?

@0x5bfa
Copy link
Member Author

0x5bfa commented Mar 25, 2024

Added steps to the body!

@yaira2
Copy link
Member

yaira2 commented Mar 25, 2024

Can you confirm if disabling a widget will no longer construct that view model?

yaira2
yaira2 previously approved these changes Mar 31, 2024
View reviewed changes
@yaira2
Copy link
Member

yaira2 commented Mar 31, 2024

There seems to be a performance regression. In the main branch, opening a new tab to the home page loads it almost instantly whereas in this branch it seems to need to reload the items.

@yaira2 yaira2 self-requested a review March 31, 2024 14:45
@yaira2 yaira2 force-pushed the main branch 3 times, most recently from 32de19b to ba56951 Compare April 4, 2024 21:47
@0x5bfa
Copy link
Member Author

0x5bfa commented Apr 6, 2024

I'll look into that, thanks for the heads up.

@0x5bfa 0x5bfa added the changes requested Changes are needed for this pull request label Apr 8, 2024
@0x5bfa
Copy link
Member Author

0x5bfa commented Apr 14, 2024 •
edited

There seems to be a performance regression. In the main branch, opening a new tab to the home page loads it almost instantly whereas in this branch it seems to need to reload the items.

This issue has been fixed by using DI for each widget view model, and this introduces some performance improvements since the app uses the same instance and event invokers among tabs within the same instance, while previous approach was using static observable item collections for each, which had caused a bit complicated codebase quality.

Ready for Review!

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

0x5bfa commented Apr 19, 2024

@hishitetsu When you got some time, would you mind if I ask you to review this PR?

@hishitetsu
Copy link
Member

@hishitetsu When you got some time, would you mind if I ask you to review this PR?

No, I'll take a look when I have time.

@0x5bfa 0x5bfa added the requires quality assurance Pull request requires QA testing before being merged label Apr 19, 2024
@hishitetsu
Copy link
Member

I've found following issues:

  • In Tags widget, Open with and Send to in the right-click menu appear in the overflow.
  • In Tags widget, the right-click menu of folders has become that of files.
  • In Recent Files widget, the right-click menu can't open.

@0x5bfa
Copy link
Member Author

0x5bfa commented Apr 22, 2024

In Tags widget, Open with and Send to in the right-click menu appear in the overflow.

Not for me. Also, Send To should be in the overflow so duplicated one which was in the primary flyout menu has been removed.

In Tags widget, the right-click menu of folders has become that of files.

What do you mean by "become that of files"?

In Recent Files widget, the right-click menu can't open.

Fixed, thanks!

@0x5bfa
Updated command constructors and UI elements in various files
9ead11b 
In `WidgetFileTagsContainerItem.cs`, updated `AsyncRelayCommand` constructors for `ViewMoreCommand` and `OpenAllCommand` to not require `CancellationToken` parameter. Also, updated related methods and simplified `SelectedTagChanged` event invocation. In `FileTagsWidget.xaml`, removed alignment properties from `Grid` element and added new `Setter` for `HorizontalAlignment` in `GridViewItem` style. Renamed `x:Name` property of `ListView` in `RecentFilesWidget.xaml` and updated event handlers accordingly. Also, renamed methods in `RecentFilesWidget.xaml.cs` to reflect this change. In `RecentItem.cs`, changed `Path` property to an override. Lastly, removed `SendTo` menu item from context menu in `FileTagsWidgetViewModel.cs` and `RecentFilesWidgetViewModel.cs`.
@yaira2
Copy link
Member

yaira2 commented Apr 22, 2024

Open with and send to should be in the main menu.

@hishitetsu
Copy link
Member

Not for me. Also, Send To should be in the overflow so duplicated one which was in the primary flyout menu has been removed.

I agree with Yair. We should keep Open with and Send to in the primary flyout and ones in the overflow should be removed.

What do you mean by "become that of files"?

The right-click menu of folders should be like this.
image

But in this PR, it is like this. This is the same menu as of files.
image

@0x5bfa
Copy link
Member Author

0x5bfa commented Apr 22, 2024

I agree upon "Open With", but "Send To" is a shell menu.
Flyout for folders and files shows it in the submenu actually.

@hishitetsu
Copy link
Member

I'm just pointing out what is different from the current release version. I don't think we should change anything in the UI with this PR.

@0x5bfa
Refactor ViewModel classes and update BaseWidgetViewModel
425871a 
Moved `ListedTagViewModel` and `TagViewModel` classes to `Files.App.Data.Models` namespace. Added workaround for file tags `isFolder` in `BaseWidgetViewModel.cs` and updated `GetItemMenuItems` method call to include a new parameter. Enhanced `ListedTagViewModel` and `TagViewModel` classes with new properties and initialized them in constructors. Decorated `TagViewModel` properties with `JsonPropertyName` attributes for JSON representation.
@0x5bfa
Merge branch '5bfa/Update-WidgetVMs' of https://github.com/0x5bfa/Files…
7f65784 
… into 5bfa/Update-WidgetVMs
@0x5bfa
Copy link
Member Author

0x5bfa commented Apr 22, 2024

You are right, sorry, I will fix it anyways.

@0x5bfa
Copy link
Member Author

0x5bfa commented Apr 22, 2024

Done.

@hishitetsu
Copy link
Member

Open with and Send to still appear in the overflow. They should be removed from the overflow and their submenus should be available from the main menu.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yaira2 yaira2 Awaiting requested review from yaira2

Assignees
No one assigned
Labels
needs-review requires quality assurance Pull request requires QA testing before being merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@0x5bfa @yaira2 @hishitetsu