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

Update to .Net5 #228

Merged
merged 16 commits into from
Feb 23, 2021
Merged

Update to .Net5 #228

merged 16 commits into from
Feb 23, 2021

Conversation

taooceros
Copy link
Member

@taooceros taooceros commented Nov 30, 2020

.Net 5 Upgrade.
close #197

  • Net 5 has added several improvement to System.Text.Json, and serialization with private getter and setter has been added.

@JohnTheGr8
Copy link
Member

@taooceros looking good thus far.

Remember to also rename the publish profile: Flow.Launcher/Properties/PublishProfiles/NetCore3.1-SelfContained.pubxml

@taooceros
Copy link
Member Author

@taooceros looking good thus far.

Remember to also rename the publish profile: Flow.Launcher/Properties/PublishProfiles/NetCore3.1-SelfContained.pubxml

ok

@taooceros taooceros marked this pull request as ready for review December 1, 2020 02:39
@JohnTheGr8
Copy link
Member

@taooceros you forgot to check in the renamed publish-profile file.

Comment on lines -122 to -53
<Page Include="SettingsControl.xaml">
<Generator>MSBuild:Compile</Generator>
<SubType>Designer</SubType>
</Page>
<Page Include="SearchSourceSetting.xaml">
<Generator>MSBuild:Compile</Generator>
<SubType>Designer</SubType>
</Page>
Copy link
Member

Choose a reason for hiding this comment

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

is this removal intended?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it said duplicate page file, since the default page file has included them.

@@ -109,7 +109,6 @@
</ItemGroup>

<ItemGroup>
<PackageReference Include="Microsoft.Windows.SDK.Contracts" Version="10.0.18362.2005" />
Copy link
Member

Choose a reason for hiding this comment

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

is this removal intended?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, forget to response. Yes, because .Net 5 change the way of using RT. You can see on the TargetFramework of Program plugin is different from simply net5-windows but adding the version number. That replace the nuget package.

@jjw24
Copy link
Member

jjw24 commented Dec 2, 2020

apologies if you have already done so, can we test to double check external plugins still works fine with this upgrade (i know it's just core to 5, but just to make sure)

@jjw24 jjw24 added the enhancement New feature or request label Dec 2, 2020
@taooceros
Copy link
Member Author

apologies if you have already done so, can we test to double check external plugins still works fine with this upgrade (i know it's just core to 5, but just to make sure)

Sure, no need for hurry, just put it here for checking.

@taooceros
Copy link
Member Author

@taooceros you forgot to check in the renamed publish-profile file.

Thank you for reminding!

@taooceros
Copy link
Member Author

taooceros commented Dec 5, 2020

Updating to .Net 5 and using WinRT seems cause the new PackageManager() in UWP.cs doesn't work. dotnet/runtime#45632
Not sure why that throw.

@jjw24
Copy link
Member

jjw24 commented Feb 11, 2021

is this still draft or ready for review

@taooceros
Copy link
Member Author

This should be ready now

@jjw24
Copy link
Member

jjw24 commented Feb 12, 2021

Could you please list all the tests that have been done so we can cover all basis

@taooceros
Copy link
Member Author

Could you please list all the tests that have been done so we can cover all basis

Will do that after I have my computer. 😉

@taooceros
Copy link
Member Author

Test Checked:

  1. All internal plugins work as intended.
    • Bug for new PackageManager() hasn't been fixed, but it only happens on the first time, so a try catch can fix it.
  2. External plugins work as intended, even when their Target Framework is netcoreapp 3.1
  3. Python plugin works as intended.
  4. Have used it for a while and haven't encountered other bugs.

@taooceros
Copy link
Member Author

Another things is that .Net 5 obsolete BinaryFormatter because of security issue (though I think it will be secure for us). Should we find an alternative for that?

@jjw24
Copy link
Member

jjw24 commented Feb 14, 2021

Another things is that .Net 5 obsolete BinaryFormatter because of security issue (though I think it will be secure for us). Should we find an alternative for that?

Where is it used?

@taooceros
Copy link
Member Author

Another things is that .Net 5 obsolete BinaryFormatter because of security issue (though I think it will be secure for us). Should we find an alternative for that?

Where is it used?

Only in a few place about the cache. Win32 and UWP, and image usage storage.

@jjw24
Copy link
Member

jjw24 commented Feb 14, 2021

if we can change it, lets do it. Are there any preferred alternatives?

@taooceros
Copy link
Member Author

if we can change it, lets do it. Are there any preferred alternatives?

I am not quite sure. Maybe it is quite possible to simply use JsonStorage, but not sure about whether it will be slower. Another alternative is ProtoBuf-net, and I have taken a try, v2 works fine while v3 doesn't work because of the cyclic reference for UWP.

@jjw24
Copy link
Member

jjw24 commented Feb 15, 2021

Remember to update readme as well

@jjw24
Copy link
Member

jjw24 commented Feb 23, 2021

I think that's for the whole 1.8.0 because of the migration of ISavable.

ok so we need to sort Everything out before 1.8.0 goes out then right?

@taooceros
Copy link
Member Author

ok so we need to sort Everything out before 1.8.0 goes out then right?

yes, because Everything plugin use an old Flow.Infracturature, but that hasn't been loaded to Flow because Flow exist a new one. Actually, due to the new plugin api changed, everything plugin maybe able to get rid of it.

@taooceros
Copy link
Member Author

Let me take a look on whether the xaml do have loaded by default.

@jjw24
Copy link
Member

jjw24 commented Feb 23, 2021

bookmark plugin got the same issue...

@taooceros
Copy link
Member Author

bookmark plugin got the same issue...

Will take a look on both. Maybe resource xaml is not included by default, but page xaml does😅

@jjw24
Copy link
Member

jjw24 commented Feb 23, 2021

I tested adding .Net 5 Flow.Launcher.Plugin dll to WindowWalker plugin project which is .Net Core, getting this error:
image

@taooceros
Copy link
Member Author

I tested adding .Net 5 Flow.Launcher.Plugin dll to WindowWalker plugin project which is .Net Core, getting this error:
image

Yeah they cannot add it since it is higher version. However, if they keep the current Flow.Launcher.Plugin.dll, they can be loaded successfully by flow.

@taooceros
Copy link
Member Author

taooceros commented Feb 23, 2021

bookmark plugin got the same issue...

Will take a look on both. Maybe resource xaml is not included by default, but page xaml does😅

I add the Content back, and they work. Thank you for the discovery! Seems that only the Page files are loaded by default.

@jjw24
Copy link
Member

jjw24 commented Feb 23, 2021

if they keep the current Flow.Launcher.Plugin.dll, they can be loaded successfully by flow.

but that will mean these plugins will not get the new updates to Plugin project

@taooceros
Copy link
Member Author

taooceros commented Feb 23, 2021

if they keep the current Flow.Launcher.Plugin.dll, they can be loaded successfully by flow.

but that will mean these plugins will not get the new updates to Plugin project

If they want to get the updated version, they just need to upgrade to .Net 5. It is really easy to do that within a small project. (simply change the targetframework)

@jjw24
Copy link
Member

jjw24 commented Feb 23, 2021

Plugin project will need to be a major version bump then

@taooceros
Copy link
Member Author

Plugin project will need to be a major version bump then

Isn't we bump that from 1.4.0 to 1.5.0? Or you mean to bump it to 2.0.0

@jjw24
Copy link
Member

jjw24 commented Feb 23, 2021

bump to 2.0.0, it's a breaking change for plugins

@taooceros
Copy link
Member Author

Would you please do that for me? I am not quite sure how to bump version😂

@jjw24
Copy link
Member

jjw24 commented Feb 23, 2021

Would you please do that for me? I am not quite sure how to bump version😂

yeah no problem, leave it with me. I will do that when we release

@jjw24
Copy link
Member

jjw24 commented Feb 23, 2021

are these formatting differences because you are using VSCode?
image

@taooceros
Copy link
Member Author

taooceros commented Feb 23, 2021

are these formatting differences because you are using VSCode?
image

I think so.

@taooceros
Copy link
Member Author

Well my VS will format it like this .....
image

@jjw24
Copy link
Member

jjw24 commented Feb 23, 2021

maybe when you have time have a look at how we can sync the format. I dont want to end up with too many 'fix format' commits 😛

When I get some time I will see if VSCode is a better tool than Visual Studio and switch if so, because i use it for other programming stuff.

@taooceros
Copy link
Member Author

taooceros commented Feb 23, 2021

maybe when you have time have a look at how we can sync the format. I dont want to end up with too many 'fix format' commits 😛

Yeah sure🤣🤣

When I get some time I will see if VSCode is a better tool than Visual Studio and switch if so, because i use it for other programming stuff.

I think VSCode is quite great for us to use, even though it will not be able to us to track CPU usage and Memory usage with VSCode.
Omnisharp provides similar programing experience with VS, except that it cannot give intellisence for unusing namespace (so I have to manually using System.Linq before using Select, Where, etc.).
Omnisharp will need similar time to load project as VS. However, it won't block the main thread so that if we only want to edit a few files, it would be really fast for us to do that.
For VS, we have to wait for the long startup time.... Generally I won't let VS open until I use it, so VSCode makes me feel really great when I need to only edit one or two line of code comparing to VS.

@jjw24
Copy link
Member

jjw24 commented Feb 23, 2021

except that it cannot give intellisence for unusing namespace (so I have to manually using System.Linq before using Select, Where, etc.).

No wonder occasionally you leave unused namespaces around 😆

PR LGTM, couple more testing and will merge in with 1.8.0

@jjw24 jjw24 enabled auto-merge February 23, 2021 19:05
@jjw24 jjw24 merged commit 374bb79 into Flow-Launcher:dev Feb 23, 2021
auto-merge was automatically disabled February 23, 2021 19:39

Pull request was closed

@jjw24 jjw24 removed the review in progress Indicates that a review is in progress for this PR label Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to .Net 5
3 participants