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

Convert TerminalSettingsEditor into a DLL #7675

Merged
3 commits merged into from
Sep 22, 2020

Conversation

leonMSFT
Copy link
Contributor

This PR's only purpose is to convert the TSE project into a DLL while getting rid of unnecessary files in the meantime, namely the App.* files and moving its ResourceDictionary into a file called CommonResources.xaml.

In the process, I needed to move all the Model classes under Editor since it would have been a huge pain to try to add a Model dll alongside the Editor dll. I figured it also wouldn't matter too much since we'll be deleting the Model namespace in favor of using the incoming TSM.

While this doesn't contain any code of putting the Settings UI in a tab, I figured it would be nice to push this particular part as its own standalone PR.

@DHowett
Copy link
Member

DHowett commented Sep 18, 2020

(That's the extent of my review.)

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

A part of me doesn't like that you replaced Microsoft.Terminal.Settings.Model references with Microsoft.Terminal.Settings.Editor.Model, even though I know they'll be replaced soon. Any way you can keep them as it was before? If it's too much work, I'll give up on this one.

Also want to see if we can rename the dll.

@leonMSFT
Copy link
Contributor Author

Yeah I don't like it either 😢, I still don't think it's worth the effort to keep it as .Settings.Model, especially since it's gonna be replaced soon.

It looks like to rename the dll, changing <ProjectName> was sufficient, but it left a wacky residual empty Microsoft.Terminal.Settings.Editor folder, and the guts were all in a TerminalSettingsEditor folder. I fixed it by changing the vcxproj name as well, and along with that the references to the vcxprojs in OpenConsole.sln and other vcxprojs.

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

\azp run

@leonMSFT leonMSFT added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 22, 2020
@ghost
Copy link

ghost commented Sep 22, 2020

Hello @leonMSFT!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 51ba054 into feature/settings-ui Sep 22, 2020
@ghost ghost deleted the dev/lelian/settingsinatab branch September 22, 2020 21:00
leonMSFT added a commit that referenced this pull request Sep 24, 2020
This PR's only purpose is to convert the TSE project into a DLL while getting rid of unnecessary files in the meantime, namely the `App.*` files and moving its `ResourceDictionary` into a file called `CommonResources.xaml`.

In the process, I needed to move all the `Model` classes under `Editor` since it would have been a huge pain to try to add a Model dll alongside the Editor dll. I figured it also wouldn't matter too much since we'll be deleting the `Model` namespace in favor of using the incoming TSM.

While this doesn't contain any code of putting the Settings UI in a tab, I figured it would be nice to push this particular part as its own standalone PR.
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants