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: Shell page refactoring #11034

Merged
merged 19 commits into from Feb 15, 2023
Merged

Code Quality: Shell page refactoring #11034

merged 19 commits into from Feb 15, 2023

Conversation

ferrariofilippo
Copy link
Contributor

This PR aims to refactor ColumnShellPage and ModernShellPage. They have a lot of code in common and I think we should create a base class to avoid code duplication and simplify future maintenance.
This is still a draft, I have to check that everything works as expected.
@lukeblevins I'd be glad if you could help me with this refactoring

@ferrariofilippo ferrariofilippo marked this pull request as draft January 17, 2023 13:33
@QuaintMako
Copy link
Contributor

@ferrariofilippo is there work left on this PR or could it be theorically ready for merging?

@ferrariofilippo
Copy link
Contributor Author

Theoretically it's ready but @yaira2 asked me to wait a review from @lukeblevins

QuaintMako
QuaintMako reviewed Jan 30, 2023
View reviewed changes
Copy link
Contributor

@QuaintMako QuaintMako left a comment

Choose a reason for hiding this comment

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

A first quick review of BaseShell. Looks overall promising, but I need to now look at the two existing ShellPage to make sure the code removed have lost no instruction.
Thanks for this PR again !

src/Files.App/Views/BaseShellPage.cs Outdated Show resolved Hide resolved
src/Files.App/Views/BaseShellPage.cs Outdated Show resolved Hide resolved
src/Files.App/Views/BaseShellPage.cs Outdated Show resolved Hide resolved
src/Files.App/Views/BaseShellPage.cs Outdated Show resolved Hide resolved
@yaira2
Copy link
Member

yaira2 commented Jan 30, 2023

Theoretically it's ready but @yaira2 asked me to wait a review from @lukeblevins

I think we can move forward and if Luke is able to he'll add his review.

@ferrariofilippo ferrariofilippo marked this pull request as ready for review January 30, 2023 17:39
@yaira2 yaira2 changed the title Shell page refactoring Code Quality: Shell page refactoring Feb 3, 2023
@ferrariofilippo
Copy link
Contributor Author

@yaira2 with last commit I fixed a TODO (support for Open in terminal shortcut). I did that in this PR to avoid having to rewrite the code after merging this PR. Do you think I should open a different PR?
BTW, I think I should add a setting to choose the Default Terminal (would also be useful for Terminal Integration)

@yaira2
Copy link
Member

yaira2 commented Feb 6, 2023

Do you think I should open a different PR?

Should be in a separate PR

BTW, I think I should add a setting to choose the Default Terminal (would also be useful for Terminal Integration)

We used to have this but removed it when Windows added a built-in setting.

QuaintMako
QuaintMako previously approved these changes Feb 15, 2023
View reviewed changes
Copy link
Contributor

@QuaintMako QuaintMako left a comment •
edited

Choose a reason for hiding this comment

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

LGTM, I think I haven't seen any missing code. Saw a few things that could be cleaned up that I'll fix an upcoming PR, but it's out of the scope.

Thanks Ferrario for that chunky work!

yaira2
yaira2 previously approved these changes Feb 15, 2023
View reviewed changes
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.

Hoping we can gid rid of modern shell page altogether in the near future

@QuaintMako
Copy link
Contributor

Hoping we can gid rid of modern shell page altogether in the near future

Let's open an issue for that one and write down what would be needed to. The new command system would be a step in the right direction for one.

@yaira2 yaira2 merged commit dc676d3 into files-community:main Feb 15, 2023
@ferrariofilippo ferrariofilippo deleted the Shell_Page_Refactoring branch February 15, 2023 16:06
@ferrariofilippo ferrariofilippo mentioned this pull request Feb 23, 2023
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yaira2 yaira2 yaira2 left review comments

@QuaintMako QuaintMako QuaintMako left review comments

Assignees
No one assigned
Labels
approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ferrariofilippo @QuaintMako @yaira2