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

Port to .NET 5 #49

Closed
billpeet opened this issue Nov 5, 2020 · 11 comments
Closed

Port to .NET 5 #49

billpeet opened this issue Nov 5, 2020 · 11 comments

Comments

@billpeet
Copy link

billpeet commented Nov 5, 2020

The future of .NET is .NET 5 :)
It appears the DG.AdvancedDataGridView NuGet package actually works on a .NET 5 project (even if NuGet complains), it looks like it's at least partly compatible.
Any chance of officially targeting .NET 5 in a future release?

@davidegironi
Copy link
Owner

Hello @billpeet ,
unluckily I've no plan for .NET 5 or .NET Core, time to develop this open project is reduced. Also, up to now NET.5 is still RC, i think I will in the future skip .NET Core and go to .NET 5, when it will be stable. Of course hope I've time to perform this task.

@kirsan31
Copy link

kirsan31 commented Mar 31, 2021

@davidegironi if your still interested, I made it here. And it's working at first look.

What done:

  • SDK projects format.
  • Multitargeting with net5.0-windows and net40.

What was not checked:

  • _DevTools dir.
  • NuGet stuff.

@davidegironi
Copy link
Owner

That's sound interesting, let me check it in the weekend. Meanwhile if you can check the nuget and _DevTools let me know.

@davidegironi
Copy link
Owner

I'm working on the net5 version. I think I will make a new branch. NET5 works, but I want to write the compiling script for dll and nuget package. I'm almost done with both, I think next week I'll have time to work on this.

@davidegironi
Copy link
Owner

Hello, check this out: https://www.file.io/download/lRRkuJowXmFN
Target net40 and net5.0-windows.
Updated the AutoBuild script and the NuGet builder, now both works (find AutoBuild updated version here: http://davidegironi.blogspot.com/2014/04/autobuild-is-build-automation-tool-for.html)
I've done my porting from nonSDK to SDK project, cause in your one there are a few things changed in the sln file also.
Check it out and let me know. Once you have done I'll post this no nuget, then I'll add the other changes requested here: #60

@kirsan31
Copy link

Hi @davidegironi. Sorry I have very hard time at work for now. I will back as soon as I can.

@davidegironi
Copy link
Owner

As a side note, if you want to check also the menustrip mod, you can find it here: #58 (comment)

This update contains the rewrite of the checklist filter logic, it's quite important. So It's better to check it a lot before publishing.

@kirsan31
Copy link

@davidegironi

Hello, check this out: https://www.file.io/download/lRRkuJowXmFN

Hi, I looked at the code, and use .net5 version for some time and all seems ok (in terms of porting to .net5).

As a side note, if you want to check also the menustrip mod, you can find it here: #58 (comment)

File is not available any more.

P.s. While testing, I found some not serious memory problems in demo app and in lib too. But fixes are trivial. More serious problem is memory leak in ColumnHeaderCell :(
In OnColumnAdded we replace existing HeaderCell with our ColumnHeaderCell. So, in ColumnHeaderCell constructor we need to do a full clone of existing HeaderCell to dispose it after cloning. But this doesn't happens. Same in Clone method.
With current implementation we simple reference elements of old HeaderCell and this lead to impossibility of GC it.

In constructor we need to clone base DataGridViewColumnHeaderCell and if HeaderCell is ColumnHeaderCell clone new elements too (including MenuStrip).
This is not very trivial task and not possible without reflection (needed for cloning event handlers) :(

I'll be back as soon as I can (still troubles at work), hopefully in a few days.

@davidegironi
Copy link
Owner

Thank you @kirsan31 .

You can find the file here: https://file.re/2021/04/19/advanceddatagridview/

About the Memory Leak, I've built after this issue #41 the "Memory Test" button.
I've checked it with net40, no problem at all. The behaviour under net5.0 indeed is different. This is a bit wired.
I thought

protected override void OnColumnRemoved(DataGridViewColumnEventArgs e)

should fix the problem but it's not like this.
I have to investigate on this. If you find a solution please share it with me, using the last code I send you. Thanks!

@kirsan31
Copy link

kirsan31 commented Apr 23, 2021

@davidegironi Sorry, I haven't checked this yet :( Still have very little free time.
Demo app is leaking itself, net40 after 1 memory test:
image
I have fixed it here (with some small additional improvements).
Also some small memory fixes.

About huge memory leak on .Net5. The main problem of it I described in previous post. Now why on net4 we have only small leak and on .Net5 huge. This workaround:

protected override void OnHandleDestroyed(EventArgs e)
{
foreach (DataGridViewColumn column in Columns)
{
ColumnHeaderCell cell = column.HeaderCell as ColumnHeaderCell;
if (cell != null)
{
cell.SortChanged -= Cell_SortChanged;
cell.FilterChanged -= Cell_FilterChanged;
cell.FilterPopup -= Cell_FilterPopup;
MenuStrip menustrip = cell.MenuStrip;
menustrip.Dispose();
}
}
base.OnHandleDestroyed(e);
}

Not working on .Net5 because Columns already empty here. They are cleared here, with disposing of bindingSource_main.

We need properly dispose ColumnHeaderCell in OnColumnRemoved as your originally discuss. But the main problem prevent doing it. I will think of some workaround, because doing it right a bit challenging :)
Will be back as soon as I can.

----UPD----

It seems to me that the memory leak has been fixed. Your can check it here.

You can find the file here: https://file.re/2021/04/19/advanceddatagridview/

This file is gone too :( Why don't you just push a new branch?

@davidegironi
Copy link
Owner

Hello @kirsan31
Thank you for your job,
I wasn't that much interested in the sample project FormMain investingation, but as you pointed out it turns out it has big memory leak.
In a few days I'll publish an update containing

  • net40 and net5.0 update
  • SDK solution format
  • leak fixed
  • MenuStrip new logic, with max checklist nodes enabler

It will be a major number update.
Up to my test all works. Here is the version I'll publish, If you want to check it out: https://file.io/NI8xeXxT2RVg
Next version will come out in a few weeks. I'll add the fix you suggested here: #60

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

No branches or pull requests

3 participants