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

Add netcore support to OxyPlot.GtkSharp3 #10

Merged
merged 9 commits into from
Oct 13, 2021
Merged

Conversation

hol430
Copy link
Contributor

@hol430 hol430 commented Jun 30, 2020

Fixes #9

  • using https://github.com/GtkSharp/GtkSharp for netcore support
  • Had to tweak mouse scroll event handler, as the api is a bit different between gtk2/gtk3
  • tested ExampleBrowserGtk3 under net461 and netcoreapp2.1, on windows and Linux, seems to work ok

@VisualMelon
Copy link
Contributor

Thanks for putting this together. Snyk always fails when the csprojs change.

Other notes:

  • probably can't merge until the GtkSharp dependency releases with signed DLLs, but it looks like they release quite regularly
  • now that I think about it, it may be necessary to preserve NET Framework 4.5.1 support (this is done in OxyPlot.Core at the moment by either conditonally targeting gtk-sharp or trying to get GtkSharp to target NET Framework 4.51 which seems easy enough and I'll follow up with the maintainers

@hol430
Copy link
Contributor Author

hol430 commented Jul 1, 2020

Sweet, thanks for following that up for me. Let me know if you want any changes made/commits squashed/etc.

@d-buchmann
Copy link
Contributor

I tested (kind of) this setup on both Windows and Linux.
Although compiling and the mouse wheel fix are working well, there are other issues (to be referenced), so I would recommend to postpone the merge.

FYI: I ran ExampleBrowserGtk3 with the following changes:

  • target framework .NET Core 3.1
  • NuGet package GtkSharp 3.24.24.34
  • NuGet package OxyPlot.ExampleLibrary 2.0.0

@hol430
Copy link
Contributor Author

hol430 commented Jun 23, 2021

Thanks @d-buchmann - what other issues did you encounter? We've been using this in our development build for a number of months and haven't encountered any problems, although we aren't really doing anything too outlandish.

@d-buchmann
Copy link
Contributor

Actually, the problems I encountered were basically #11, and I'm quite sure that's not related to this PR.
Therefore no more objections from my side :)

@VisualMelon VisualMelon mentioned this pull request Jul 5, 2021
@hol430
Copy link
Contributor Author

hol430 commented Aug 25, 2021

Hi @VisualMelon, I've been looking into this again recently, as I've been wanting to use this .net core version in conjunction with recent changes in oxyplot (specifically, the skiasharp rendering capabilities). With that in mind, I think it would be useful to get this merged at some point.

I've looked into targeting net451 with gtk3. Judging by the lack of response on GtkSharp repo, it seems there is not much interest from them in supporting this framework version. However, by using conditional references in the csproj, I'm able to successfully build against the gtk-sharp-3 nuget package with net451, which is what we're currently doing on master branch in this repo. This almost works, but the scrolling bug is present - scrolling in or out with mouse wheel will cause a zoom in. This is happening because the bindings in gtk-sharp-3 package don't include the DeltaY property on the EventScroll struct, but the bindings do support smooth scrolling. The result is that we don't really have any way of knowing the scroll direction (as far as I can see). Note that this bug also appears on master branch of this repo for me.

I could follow this up in the gtk-sharp-3 repo, however I don't know if they would publish a nuget fix for this, as the repo claims to be deprecated in favour of GtkSharp (which doesn't officially support net451). Or we could drop net451 support for gtk3 (note that it would still be possible to target net451/gtk2). Or we could continue to ignore this bug - like I said, it seems to exist on master branch as well. Or we could roll with your net451 branch of GtkSharp.

None of these seem like great options to me - any thoughts?

@VisualMelon
Copy link
Contributor

VisualMelon commented Aug 25, 2021

Would it be possible to do the conditional compilation on our side for net451 vs core&net461? Given the simplicity of this PR and the low maintenance on this repo in general, it probably wouldn't be a big deal. I may be missing something, but I think that means that everything should keep working as it does already, but with .net core support provided by GtkSharp?

I think that counts as 'ignoring the bug', but it doesn't remove or break any existing functionality, and it'll be easy to jump over to GtkSharp if they do add framework support for Gtk3 in the future.

Happy to help with this if you think it's a sensible idea.

(Note: OxyPlot still targets .NET 4.0 (from 2010); .NET 4.5 is from 2012).

Note that Skia support appears in OxyPlot with 2.1 which I still haven't got around to releasing.

(via gtk-sharp-3 from nuget).
@hol430
Copy link
Contributor Author

hol430 commented Aug 25, 2021

Yep, that makes sense. I've just added support for conditional compilation against gtk-sharp-3 in the net45 builds vs GtkSharp in the netsatndard builds. This isn't ready for merging yet, as there are a few issues outlined below, which I'll look into when I find time (hopefully this weekend or early next week).

  • GtkSharp3Demo doesn't run on net45X target frameworks for some reason
  • ExampleBrowserGtk3 runs on all target frameworks in the csproj, although it has the scrolling bug on the netfx versions
  • I spot some concerning Gtk-CRITICAL log messages when closing the ExampleBrowserGtk3 app (only on netcore)

Drew Holzworth added 2 commits September 20, 2021 16:04
Not sure that this is the best way to handle this. In practice it will
be up to the user of this library to determine how to they are going to
target gtk3. I don't imagine many people will be writing a new library
in net45 anyway.
...upon closing the demo application. These were caused by the errant
call to gtk_window_set_focus() - I suspect because the plot widget is
being disposed of before the window. Calling gtk_widget_grab_focus()
seems to do the job just fine without the implications of assigning the
plotview as the window's focus widget. As far as I can tell.
@hol430
Copy link
Contributor Author

hol430 commented Sep 20, 2021

Ok, it took longer than I expected before I fit this in, but I've just looked at the points I mentioned above.

  • GtkSharp3Demo now runs on net45X just fine - I just needed to target the correct gtk sharp nuget for these framework versions. My solution is perhaps a little clunky but it may be fine for demonstrative purposes; I'm open to alternatives of course - I'm not really much of a csproj/msbuild wizard :)
  • Scrolling bug is present on all versions targeting gtk-sharp-3, which is the net45X versions. This issue is also present on master branch, so I guess you could consider it to not be a regression (relative to master). Scrolling works ok on versions targeting GtkSharp
  • Fixed the Gtk-CRITICAL warnings being generated by the example browser. These were appearing when targeting GtkSharp but not gtk-sharp-3

On a side note, not really related to this PR but I've just made another patch to allow using this repo with oxyplot 2.1.0-Preview1. It may save someone some effort when you guys get around to releasing oxyplot 2.1.0.

Copy link
Contributor

@VisualMelon VisualMelon left a comment

Choose a reason for hiding this comment

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

Apart from maybe needing entries for netcoreapp2.1/3.1 in the examples (I may be missing something) this looks good to me. I'll try to find time to test it properly (need to find and wake up an old box, so takes a bit of effort), though I am going to be pretty busy for the rest of this month.

FWIW, I think we can drop netcoreapp2.1 in the examples unless you want to keep it; it should be the same as 3.1 for the purpose of testing, and isn't 'customer facing'.

@VisualMelon
Copy link
Contributor

I also took a quick look at your 2.1-preiew branch, and that looks good. I'll try to test that at the same time so we can get that merged (if you are happy) soon as well.

- Combine net framework package references using OR operator
- Remove netcoreapp2.1 targets (using netcoreapp3.1 - logic for
  conditional includes should be the same either way)
- Remove conditional imports for netstandard targets in the example
  files. Not sure why I added these in the first place :).
@hol430
Copy link
Contributor Author

hol430 commented Sep 20, 2021

Thanks for the comments - I've just tidied up the example project files a bit; combined the net4* references using an OR operator, and removed the netstandard and netcoreapp2.1 targets (from the examples) in favour of just using netcoreapp3.1.

There's no real rush for this to be merged on our end - so take as much time as you need for any manual testing.

I did a quick test with my 2.1.0 changes and they seemed to work ok. The only part I wasn't sure about was where I manually specified an edge rendering mode in PlotView.cs - perhaps this is supposed to come from somewhere else, or be a property of the PlotView; I wasn't sure how it was handled in other IPlotView implementations (maybe they don't need it). I'm happy to make a PR for that if you want, so as to not mix up the discussion of those changes with these ones :).

@VisualMelon
Copy link
Contributor

VisualMelon commented Sep 21, 2021

Thanks for those changes; it all looks good now; as I said, I'll try to test it all soon. It'll probably be sometime next week when my workload should fall off sharply and I'll be looking for things to keep me busy.

Regarding the 2.1 patch, please do open that as PR if you are happy to!

For the rectangle in PlotView it probably makes sense to go with PreferSpeed (since the box will be grid aligned) but we can work that out later; doesn't make any difference until the render context supports the different modes (e.g. by setting the graphics context SmoothingMode)

@VisualMelon
Copy link
Contributor

VisualMelon commented Sep 25, 2021

I can't seem to get the framework builds to run at all now, but I think that's my GTK install going wrong (doesn't work on master either). Net core stuff seems to be fine, though to add to the madness, my mouse scroll-wheel has stopped working, so I've not tested that bit either.

Otherwise, everything builds fine, so I think this can be merged, but I'll leave it open a little longer; I should have a lot more time soon, so I'll try to have another jab at testing everything.

@VisualMelon VisualMelon merged commit 8dc0aa7 into oxyplot:master Oct 13, 2021
@VisualMelon
Copy link
Contributor

VisualMelon commented Oct 13, 2021

Merged so that we can look at #16 and #19

Thanks for your hard work on this!

@hol430 hol430 deleted the netcore branch October 13, 2021 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Any plans to support .net core?
3 participants