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

Replaced LayoutSerializer with Async variant #265

Closed
wants to merge 11 commits into from
Closed

Conversation

X39
Copy link

@X39 X39 commented Apr 19, 2021

See #264

@X39
Copy link
Author

X39 commented Apr 19, 2021

It looks like there is a quite blocking issue with adding in async serialization ...

System.Threading.Task.GetAwaiter is nowhere to be found (as can be seen in the appveyor logs)
This is fixable by upping the version supported with .net framework from 4.0 to 4.5 (see https://docs.microsoft.com/en-us/dotnet/api/system.threading.tasks.task.getawaiter)

The potential side effects are ... i guess ... quite obvious.

This thus is out of my hand to decide, i would recommend towards the change though.

@eriove
Copy link
Contributor

eriove commented Apr 20, 2021

This will be tricky to get right in terms of avoiding dead-locks for all use cases but I like the idea. If I had the time it is something I might have tried for fun.

Out of curiosity I did a quick profiling of the startup of the application we are developing. I focused on the time the UI was frozen since that's what async might solve (I don't expect the total time to change). At first it looked like a large percentage of the time was spent in AvalonDock's methods, but after excluding all time spent loading windows around 10% of the total time was spent in actual AvalonDock code.

profiling result

What's your main reason for making it async? It feels like I have missed the point or that my application differs significantly from yours.

@X39
Copy link
Author

X39 commented Apr 20, 2021

During initialization I have the need to poll Informations from a Server in some of my documents and cannot decide wether a document should exist or not Prior

So i made the whole call chain async to be able to await the possibly non existing documents 🤷‍♂️😂

@eriove
Copy link
Contributor

eriove commented Apr 20, 2021

During initialization I have the need to poll Informations from a Server in some of my documents and cannot decide wether a document should exist or not Prior

So i made the whole call chain async to be able to await the possibly non existing documents 🤷‍♂️😂

Make sense. I suspect I could make our call-chain more async as well. Wouldn't make much of a difference on a fast developer machine but it might make a difference with a slow hard drive. We are lucky enough not to have any network calls when opening docked windows.

@Dirkster99
Copy link
Owner

I am getting compile time errors when I try to compile this. Can you please check if you can fix this?

Also, is it possible to keep both options:

  1. Load Async
  2. Load non-Async

There is another PR in the repository that introduces a new property for serialization - do we have to adjust your code for the new property introduced there or will this be covered with your code as well (trying to figure out a sequence in which these PRs should be merged best).

Thanx Drk

Error CS1061 'Dispatcher' does not contain a definition for 'InvokeAsync' and no accessible extension method 'InvokeAsync' accepting a first argument of type 'Dispatcher' could be found (are you missing a using directive or an assembly reference?) AvalonDock (net40) C:\Users\NOP\Downloads\AvalonDock-master\source\Components\AvalonDock\Layout\Serialization\LayoutSerializerBase.cs 123

Error CS1061 'Task' does not contain a definition for 'GetAwaiter' and no accessible extension method 'GetAwaiter' accepting a first argument of type 'Task' could be found (are you missing a using directive or an assembly reference?) AvalonDock (netcoreapp3.0) C:\Users\NOP\Downloads\AvalonDock-master\source\Components\AvalonDock\Layout\Serialization\AsyncXmlLayoutSerializer.cs 76

@X39
Copy link
Author

X39 commented Apr 30, 2021

@Dirkster99 as I mentioned here #265 (comment)
GetAwaiter is not supported in the dot net version used and hence compilation issues arise

@Dirkster99
Copy link
Owner

hmm, I don't mind upping this to Net 4.5 but I am pretty sure a lot of people using this library will not like the idea :-( even though Net 4.0 is quit old in programing terms ...so, I am not really sure what we should do here?

@X39
Copy link
Author

X39 commented May 1, 2021

I would argue for upgrading.
Tho... I might not be the best person to ask for, because imo people should keep their software updated.

@Dirkster99
Copy link
Owner

Dirkster99 commented May 1, 2021

Yes I see it the same way in favor of upgrading - an additional argument is that I reviewed the 147 dependent projects (avoiding duplicate forks) and found the counts below. So, to make this also a data driven decision we could go for:

  • net5
  • netcore 3.1 (since 3.0 is out of support) and
  • Net4.5.2

What do you think about that?

Projects Framework
25 Net5
25 netcore 3.1
6 netcore 3.0
Projects Framework
8 .Net4.8
23 .Net4.7.2
1 .Net4.7.1
2 .Net4.7
8 .Net4.6.1
4 .Net4.5.2
3 .Net4.5
1 .Net4.0

@Dirkster99
Copy link
Owner

@X39 Do you think you can finish this PR? Otherwise, would you please close it?

@X39
Copy link
Author

X39 commented Nov 3, 2021

The main issue, and the reason i did not continued with with PR, was the .Net dependency thing.
That is nothing i particularry want to touch due to possibly choosing the wrong version here for the project.

If that changed already (and i missed it) i will gladly finish any remaining problems :) @Dirkster99

@Dirkster99
Copy link
Owner

Looks like we have chicken-egg problem here. You don't want to make a decision on the platform version but its required to make the suggested change. I don't want to change the platform version without gaining anything in the process :-( ...

So how about this.

Provided, I update a branch to:

  • net5
  • netcore 3.1 (since 3.0 is out of support) and
  • Net4.5.2

Could you then finish the Async PR to complete the feature and make the update in terms of releasing a new version of AvalonDock worthwhile?

@X39
Copy link
Author

X39 commented Dec 16, 2021

Sure thing 👍
Will be done ~by monday probably.

Feel free to raise further concerns regarding the implementation 😉

@Dirkster99
Copy link
Owner

Just, for clarification:

  1. You are fine creating the branch with the suggested framework versions or did you want me to go ahead and create the branch so you can commit into it?

  2. I also accepted PR #308 today which cleans up the serialization code to the extend that unnecessary (handled) exception displays by the XmlSerializer are no longer shown in the VS output window. Would you be able to integrate this change to keep this behavior?

Thanks a lot for your time and effort :-)

@X39
Copy link
Author

X39 commented Dec 27, 2021

Hey there, sorry for the late reply

yes, willco (tho a little bit later then anticipated by me due to real life :3)

@X39
Copy link
Author

X39 commented Dec 27, 2021

just wanted to add that this is still WIP

will notify Dirkster99 again, once done 😉

@X39
Copy link
Author

X39 commented Jan 16, 2022

@Dirkster99 i consider this now to be complete,
One important note tho: I marked the old serializer as Obsolete now ... not sure if that might be a tad too much

@Dirkster99
Copy link
Owner

@X39
I briefly looked at the PR with the intent to test it but found no call in any demo program to invoke the new AsyncXmlLayoutSerializer :-(

I don't mind making the old serializer obsolete since 1 serializer should be used in the long run.

  1. I understand the AsyncXmlLayoutSerializer could also be used in a blocking fashion so as to emulate the behavior of the old serializer?

I think it would be useful for everyone using the library if we could have same for both cases:

  1. Calling the new serializer in an async fashion (eg. MLibTest, AvalonDockTest, MVVMTestApp)
  2. Calling the new serializer in a blocking fashion so as to emulate the old serializer (eg.TestApp, CaliburnDockTestApp, WinFormsTestApp, VS2013Test)

Either way I am thinking that the demo apps should be using the new serializer only to demonstrate their usage and make this simple to test and be consistent with the fact that the old serializer is obsolete and will be removed if everything goes well :-)

What do you think about this? Could you please add invocations from the demo apps to the new async serializer?

@X39
Copy link
Author

X39 commented Jan 19, 2022

Blocking is actually problematic due to the UI calls and the whole serializer probably then running on STA thread, making the dispatcher calls cause a deadlock

maybe i should undo the obsolete 🙈


regarding the demo, expect that to arrive ~end of next week at earliest

@Dirkster99
Copy link
Owner

Yeah, I guess we should undo the obsolete then and provide both versions at least for some time but it would still be great if we could provide a few async samples so people can use this without too much tinkering and guess work :-) thanks a lot

@Dirkster99
Copy link
Owner

@X39 Can you complete the demo and remove the obsoletes so we can get this released, please?
@eriove Maybe you can help with some async demo code in 2-3 of demo client apps so we can get this out the door?

@X39
Copy link
Author

X39 commented Mar 10, 2022

Hey @Dirkster99
sorry, totally forgot about this

Removed the obsolete marker, fixed the naming scheme of the async methods and added samples for the serializer to both the docs and the MVVM sample :)
Can do the others too ... but it would be less clear there 🤔 as those require more refactoring

@Dirkster99
Copy link
Owner

Hey @X39

I looked at the MVVMTestApp sample but it does not work because:

  1. the Window Unloaded event never fires - and so
  2. the .\AvalonDock.config file is never serialized - and so
  3. the .\AvalonDock.config file can never be Deserialized on window load

This known problem was already there before you PR :-( but it can be fixed by using the line below with the corresponding signature change:

		public MainWindow()
		{
			InitializeComponent();

			this.DataContext = Workspace.This;

			this.Loaded += new RoutedEventHandler(MainWindow_LoadedAsync);
			//this.Unloaded += new RoutedEventHandler(MainWindow_Unloaded);
			this.Dispatcher.ShutdownStarted += MainWindow_Unloaded;
		}
		private void MainWindow_Unloaded(object sender, EventArgs e)
		{
			Serialize();
		}

These changes ensure that the Serialization/DeSerialization code is actually invoked as expected but now I am running into a NULL pointer exception somewhere in LayoutSerializerBase.cs if I use:

		private async void MainWindow_LoadedAsync(object sender, RoutedEventArgs e)
		{
			// Deserialize();
			await DeserializeAsync();
		}

image

Can you see what I am doing wrong?

I guess the particular MVVMTestApp sample is a little bit confusing because it can load 2 different layouts:

  1. Via Window Start-Up and Shut-Down time (see comments above)
  2. Via LoadLayoutCommand and SaveLayoutCommand time (see regions in MainWindow.cs)

Can we make 1. and 2. Async please? There are plenty of other non-async samples in the project so please don't worry removing non-async code in this sample...

There is this LayoutSerializationCallback code below which is currently commented out here - is it possible to use this code in the Async version as well or are this callbacks not supported in the Async version of the serializer?

		private void OnLoadLayout()
		{
			var layoutSerializer = new XmlLayoutSerializer(dockManager);

			// Here I've implemented the LayoutSerializationCallback just to show
			//  a way to feed layout desarialization with content loaded at runtime
			// Actually I could in this case let AvalonDock to attach the contents
			// from current layout using the content ids
			// LayoutSerializationCallback should anyway be handled to attach contents
			// not currently loaded
			layoutSerializer.LayoutSerializationCallback += (s, e) =>
				{
					//if (e.Model.ContentId == FileStatsViewModel.ToolContentId)
					//    e.Content = Workspace.This.FileStats;
					//else if (!string.IsNullOrWhiteSpace(e.Model.ContentId) &&
					//    File.Exists(e.Model.ContentId))
					//    e.Content = Workspace.This.Open(e.Model.ContentId);
				};
			layoutSerializer.Deserialize(@".\AvalonDock.Layout.config");
		}

@X39
Copy link
Author

X39 commented Mar 16, 2022

I am actually not sure why that null reference exception is happening and sadly cannot reproduce it either
probably best to work out a proper async example

There is this LayoutSerializationCallback code below which is currently commented out here - is it possible to use this code in the Async version as well or are this callbacks not supported in the Async version of the serializer?

The way the async serializer works is similar to the normal xml serializer (that is: it also uses an event, but an async one)
image

@Dirkster99
Copy link
Owner

Mhh, I am confused because I assumed the sample that you added was a proper async serializer :-(

@X39
Copy link
Author

X39 commented Mar 21, 2022

I assumed the sample that you added was a proper async serializer

I am not entirely sure what you mean by that, it is properly async deserializing
only serializing is done in sync (as XmlSerializer is not supporting async serialization)

The event itself is properly awaited

protected async Task<LayoutRestoreEventArgs> RaiseLayoutRestoreAsync(
LayoutContent layoutContent,
object content)
{
var eventArgs = new LayoutRestoreEventArgs(layoutContent, content);
foreach (var callback in _layoutRestore)
{
await callback(this, eventArgs);
if (eventArgs.Cancel || eventArgs.Handled)
{
break;
}
}
return eventArgs;
}

@Dirkster99
Copy link
Owner

Dirkster99 commented Mar 22, 2022

I was referring to you saying this previously:

I am actually not sure why that null reference exception is happening and sadly cannot reproduce it either
probably best to work out a proper async example

and I was confused because I thought the sample code we added was a 'proper async example'. Can we add a proper async example using the sample application or is it indeed too difficult? Or is there another issue with that?

@X39
Copy link
Author

X39 commented Mar 22, 2022

Not really difficult, just need a proper (and hence fully) async sample with async loading and initialization of the entire window
but need to find some good day of spare time to do that

@Dirkster99 Dirkster99 closed this Nov 4, 2022
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.

3 participants