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

Remoting & New Previewer #1105

Merged
merged 18 commits into from Oct 11, 2017

Conversation

Projects
None yet
3 participants
@kekekeks
Member

kekekeks commented Aug 19, 2017

See #900 for more details

kekekeks added some commits Sep 6, 2017

Merge remote-tracking branch 'upstream/master' into remote2
# Conflicts:
#	samples/ControlCatalog.NetCore/Program.cs
#	samples/ControlCatalog/MainWindow.xaml.cs
#	src/Gtk/Avalonia.Gtk3/Gtk3Platform.cs
#	src/Gtk/Avalonia.Gtk3/Interop/GlibTimeout.cs

@kekekeks kekekeks changed the title from WIP: Remoting & New Previewer to Remoting & New Previewer Oct 6, 2017

@kekekeks kekekeks requested review from grokys and jkoritzinsky Oct 6, 2017

while (true)
{
Thread.Sleep(100);
if (Debugger.IsAttached)

This comment has been minimized.

@jkoritzinsky

jkoritzinsky Oct 6, 2017

Member

Couldn't you just use the Debugger.Launch method here instead since that will automatically prompt the user to attach a debugger?

This comment has been minimized.

@kekekeks

kekekeks Oct 6, 2017

Member

It doesn't work on Linux/OSX.

This comment has been minimized.

@jkoritzinsky

jkoritzinsky Oct 6, 2017

Member

Makes sense.

@jkoritzinsky

Looks good! I have added a few questions in the comments if you can take a look.

@@ -56,6 +56,7 @@ public class AvaloniaObject : IAvaloniaObject, IAvaloniaObjectDebug, INotifyProp
/// </summary>
public AvaloniaObject()
{
CheckAccess();

This comment has been minimized.

@jkoritzinsky

jkoritzinsky Oct 6, 2017

Member

Shouldn't this be VerifyAccess instead of CheckAccess? Also, do we need this check in the first place?

This comment has been minimized.

@kekekeks

kekekeks Oct 6, 2017

Member

Yeah, that should be VerifyAccess. And yes, we need to ensure that AvaloniaObject can be created only from UI thread, there will be hard-to-debug issues otherwise.

{
private EmbeddableControlRoot _topLevel;
class TopLevelImpl : RemoteServerTopLevelImpl, IEmbeddableWindowImpl

This comment has been minimized.

@jkoritzinsky

jkoritzinsky Oct 6, 2017

Member

Why is this class separate from RemoteServerTopLevelImpl?

This comment has been minimized.

@kekekeks

kekekeks Oct 6, 2017

Member

RemoteServer is a standalone class that can be used to host widgets for remoting. Previewer uses RemoveServerTopLevelImpl directly, since it needs more customization

This comment has been minimized.

@jkoritzinsky

jkoritzinsky Oct 6, 2017

Member

Couldn't we roll this TopLevelImpl class into RemoteServerTopLevelImpl anyway and use RemoteServerTopLevelImpl in RemoteServer and still allow the previewer to make customizations as needed?

This comment has been minimized.

@kekekeks

kekekeks Oct 7, 2017

Member

As you can see, RemoteServerTopLevelImpl only implements ITopLevel. EmbeddableControlRoot class requires IEmbeddableWindowImpl to be implemented. There is no sense in implementing that in RemoteServerTopLevelImpl, since it should only care about sizing, rendering and input.

This comment has been minimized.

@jkoritzinsky

jkoritzinsky Oct 7, 2017

Member

Can we rename RemoteServerTopLevelImpl then? I just don't like having RemoteServerTopLevelImpl and RemoteServer.TopLevelImpl as separate classes. It's confusing naming for the future imo. Maybe we could do RemoteTopLevelImpl with RemoteServer.EmbeddableTopLevelImpl? Just to clarify the separation between the two classes.

@@ -86,67 +86,11 @@ private static void SetScalingFactor(double factor)
private static void UpdateXaml2(Dictionary<string, object> dic)

This comment has been minimized.

@jkoritzinsky

jkoritzinsky Oct 6, 2017

Member

Any chance we can rename this method? Or just combine it with UpdateXaml?

This comment has been minimized.

@kekekeks

kekekeks Oct 6, 2017

Member

It's a part of the public API or our current previewer - https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.DesignerSupport/DesignerApi.cs#L34

Can't touch that without breaking VS extension.

This comment has been minimized.

@jkoritzinsky

jkoritzinsky Oct 6, 2017

Member

That makes sense. Kinda sucks that we got stuck with this naming though..

This comment has been minimized.

@kekekeks

kekekeks Oct 7, 2017

Member

It's a common way of ABI versioning. Just look at visual studio and countless IVSProject9001 interfaces.

@kekekeks kekekeks merged commit e755664 into master Oct 11, 2017

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@jkoritzinsky jkoritzinsky deleted the remote2 branch Oct 11, 2017

@grokys grokys added this to the Beta 1 milestone Jan 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment