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

.NET Core 3.1 + NET 5 Support #42

Merged
merged 29 commits into from
Mar 29, 2021
Merged

.NET Core 3.1 + NET 5 Support #42

merged 29 commits into from
Mar 29, 2021

Conversation

304NotModified
Copy link
Member

@304NotModified 304NotModified commented Jun 11, 2019

resolves #39

works locally in test application

todo:

  • Add .NET Core 3
  • Add .NET 5
  • fix clicking link in .NET 5
  • disabled SupportLinks for .NET Core 3, but not for .NET 5
  • run unit tests for all platforms
  • run unit tests for all platforms in CI (AppVeyor) -> started
  • fix issues in other platforms, see unit tests
  • disable the linkclick tests for .NET core 3
  • move to .NET Core 3.1 instead of 3.0

@304NotModified 304NotModified changed the title .NET Core3 .NET Core3 (WIP) Jun 11, 2019
@304NotModified 304NotModified added this to the 4.3 milestone Jun 17, 2019
@304NotModified 304NotModified changed the title .NET Core3 (WIP) .NET Core 3 Support (WIP) Jun 17, 2019
@304NotModified 304NotModified force-pushed the core3 branch 3 times, most recently from 9f1c653 to 583c841 Compare August 14, 2019 21:27
@304NotModified
Copy link
Member Author

304NotModified commented Aug 14, 2019

hyperlinks won't work well

This is an issue with the selection range?

expected:

image

without change:

image

with change in PR (still no good)

image

@Logerfo
Copy link

Logerfo commented Aug 27, 2019

Have you tried the preview 8 yet? Don't know if it will help but it might be worth a shot...

@304NotModified
Copy link
Member Author

304NotModified commented Aug 27, 2019

I think I did, as preview 8 is production ready according to MS. I have to check that.

@Logerfo
Copy link

Logerfo commented Sep 2, 2019

Take a look at dotnet/winforms#1631.

@304NotModified
Copy link
Member Author

ah thanks!

@Logerfo
Copy link

Logerfo commented Sep 4, 2019

If that was indeed the problem, dotnet/winforms#1745 was merged today and will be available through the .NET Core 3.0 GA, which will be released at .NET Conference later this month (between September 23th and 25th).

@304NotModified
Copy link
Member Author

Nice!

It isn't fixed in preview 9?

@Logerfo
Copy link

Logerfo commented Sep 5, 2019

I don't think so, the merge happened a few hours later than the release announcement. Perhaps you could try it with the master "5.0" SDK, but I don't know how to do that.

@Logerfo
Copy link

Logerfo commented Sep 5, 2019

I think that would help if you want to try: https://github.com/dotnet/coreclr#using-your-build

@Logerfo
Copy link

Logerfo commented Sep 19, 2019

.NET Core 3.0 RC1 is up, did you have a chance to try it out yet?

@304NotModified
Copy link
Member Author

304NotModified commented Sep 19, 2019

Will try to test soon. If you have time, please test it, the case is really easy to see (just start the netcore3 app) - that would be very helpful :)

I'm now on holiday so a bit limited.

(The last test was with preview 9)

@Logerfo
Copy link

Logerfo commented Sep 20, 2019

Running with RC1:
image

@Logerfo
Copy link

Logerfo commented Sep 20, 2019

I think that an issue with a proper minimum reproduction should be opened at dotnet/winforms. I'm not going to do that because I'm not fully aware of stuff. I don't use this package with RTF. Actually, I don't use RTF at all, so this hyperlink syntax seems out of place for me. Nevertheless, this will not be fixed before the GA release.

@304NotModified
Copy link
Member Author

304NotModified commented Sep 20, 2019

Unfortunately I don't know if it is a bug in our code or Microsoft's

That needs an investigation

@Logerfo
Copy link

Logerfo commented Oct 16, 2019

Tested against .NET Core 3.1 Preview 1: same behavior.

@weltkante
Copy link
Contributor

weltkante commented Dec 31, 2020

Here is what it would look like to not set a fixed font and instead branch the tests. You might have to add if (IsAppVeyor) conditionals into the .NET Core conditionals as well if font sizes differ here. Can't test that since, in fact, my setup seems to be matching the AppVeyor setup. You mave have changed your default font size on your local machine?

The \f1 that disappears in .NET Core is because on Desktop Framework there are two fonts in the font table (\f0 and \f1) while in .NET Core (with default settings) there is only one font, so there is no switching between fonts in the .NET Core markup.

@weltkante
Copy link
Contributor

Did a mistake in my original tests, the officially documented hyperlink markup actually works in .NET 5 (the native control does all the work, WinForms isn't involved) - switching the link generation to not use hidden text means we don't have to wait for a fix for WinForms to pass hidden text to the LinkClicked handler.

@kirsan31
Copy link

@304NotModified

move to .NET Core 3.1 instead of 3?

I think yes, only NET 5 and .NET Core 3.1 are actual.

@weltkante
Thank you once more, your are the best, as always 😉

@304NotModified
Copy link
Member Author

I'm not really a fan of hardcoding fonts over the OS default anyways, but I'm not sure how to test that then. Any suggestion how to proceed?

I think it would be nice to have fixed fonts for the tests, and only for the tests.

You mave have changed your default font size on your local machine?

No, but maybe the zoom factor is also related?

@weltkante
Copy link
Contributor

Since I've already been halfway there I've also ported the RichTextBox.Text accessor fix from .NET 5 (which returns hidden text instead of suppressing it and therefore messing up indexing) into a standalone method. This makes links generate properly in .NET Core 3.x as well I believe, but I didn't test it extensively, the tests certainly pass. Your choice if you want that additional interop for working around a .NET Core 3.x bug, or just declare it not supported.

Technically you can use that on .NET 4.7+ as well but considering that there is a runtime switch to decide which RTF control to use it would probably get pretty hacky to figure out which RTF control is used. Certainly not enough to have static #if compile time switching between the strategies on Desktop Framework, you have to actually check in which version the RTF control is running and then select dynamically.

@304NotModified
Copy link
Member Author

I think yes, only NET 5 and .NET Core 3.1 are actual.

Ow yes indeed, EOL 2020-03-03. At work we are already at 3.1/5.0 😅

@304NotModified 304NotModified changed the title .NET Core 3 + NET 5 Support (WIP) .NET Core 3.1 + NET 5 Support (WIP) Dec 31, 2020
@weltkante
Copy link
Contributor

weltkante commented Dec 31, 2020

I think it would be nice to have fixed fonts for the tests, and only for the tests.

Any idea how to configure the font of the dynamically generated form? Would certainly simplify the tests a lot.

but maybe the zoom factor is also related?

Perhaps, it certainly complicates the tests (I have to switch the tests to run with AppVeyor settings because whatever is the standard on your side isn't for me).

@304NotModified
Copy link
Member Author

Any idea how to configure the font of the dynamically generated form? Would certainly simplify the tests a lot.

Well we could make it an option of the target (the font size) and then set a fixed from the tests?

@304NotModified 304NotModified changed the title .NET Core 3.1 + NET 5 Support (WIP) .NET Core 3.1 + NET 5 Support Mar 29, 2021
@304NotModified 304NotModified marked this pull request as ready for review March 29, 2021 20:22
@304NotModified 304NotModified merged commit 8ba69a3 into master Mar 29, 2021
@304NotModified 304NotModified deleted the core3 branch March 29, 2021 20:38
@304NotModified
Copy link
Member Author

It took some time, but it's finally merged :)

Thanks you guys!!

@304NotModified
Copy link
Member Author

It's published as 4.5.0-rc.1: https://www.nuget.org/packages/NLog.Windows.Forms/4.5.0-rc.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Target support for .net core 3.0
5 participants