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: Moving registry database to Files.App #15357

Merged
merged 1 commit into from May 12, 2024
Merged

Code Quality: Moving registry database to Files.App #15357

merged 1 commit into from May 12, 2024

Conversation

hez2010
Copy link
Member

@hez2010 hez2010 commented May 10, 2024

Resolved / Related Issues

Fixes all COMException crashing issues caused by CsWinRT. Particularly, LayoutPreferenceDatabase and FileTagsDatabase.

@yaira2 yaira2 requested a review from hishitetsu May 10, 2024 14:53
@yaira2 yaira2 changed the title Moving registry database to Files.App Code Quality: Moving registry database to Files.App May 10, 2024
@yaira2
Copy link
Member

yaira2 commented May 10, 2024

@hez2010 can you add steps we can use to test these changes?

@yaira2 yaira2 added the needs-testing Pull request requires testing before being merged label May 10, 2024
@hez2010
Copy link
Member Author

hez2010 commented May 10, 2024 •
edited

@hez2010 can you add steps we can use to test these changes?

No behavior changes as this is basically a copy-paste from Files.App.Server to Files.App. The purpose of this PR is to get rid of WinRT call while fetching tags and layout preferences.

@yaira2 yaira2 removed the needs-testing Pull request requires testing before being merged label May 10, 2024
0x5bfa
0x5bfa approved these changes May 10, 2024
View reviewed changes
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.

Can't we have both to easily revert when the package's issue fixed instead of deleting classes?

Code changes are looking good.

@hez2010
Copy link
Member Author

hez2010 commented May 10, 2024

Can't we have both to easily revert when the package's issue fixed instead of deleting classes?

I made this change in about 20 minutes. It's almost a copy-paste and fixing namespace thing.
We are using registry now, so we don't need to move it to the server project as registry is thread safe. Thus, I don't think we want to revert this PR in the future.

hishitetsu
hishitetsu approved these changes May 11, 2024
View reviewed changes
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.

I confirmed that it works fine. LGTM

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs-code review labels May 12, 2024
@yaira2 yaira2 merged commit a69c57b into files-community:main May 12, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0x5bfa 0x5bfa 0x5bfa approved these changes

@hishitetsu hishitetsu hishitetsu approved these changes

Assignees
No one assigned
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@hez2010 @yaira2 @0x5bfa @hishitetsu