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

Made an MVVM friendly example. #30

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

feinstein
Copy link

I altered the example to comply better with MVVM.
Added a Behavior for loading an HTML string as the web page, BUT this isn't working yet, so I am hoping you can explain me better what's going on so I can fix this and this code can be merged with the main project as well.

The example now complies better with MVVM.
Added a Behavior for loading an HTML [WIP].
@@ -67,8 +67,10 @@
<ApplicationManifest>app.manifest</ApplicationManifest>
</PropertyGroup>
<ItemGroup>
<Reference Include="Microsoft.Expression.Interactions, Version=4.5.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL" />
Copy link
Member

Choose a reason for hiding this comment

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

Are these dlls now part of .Net or still additional? If they're not then adding them makes deployment just that bit more complicated.

Copy link
Author

Choose a reason for hiding this comment

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

They are set through "Add Reference...", not something that I download and add as a Reference, but still not something that comes right out of the box.

I can make an Attached Property Behavior instead if you prefer, it should work just out of the box, just not something Blend could interact with nicely.

Copy link
Member

Choose a reason for hiding this comment

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

They come standard with VS these days, far as I'm aware they're not part of .Net, though been a while since I actually did any actual WPF development so things could easily have changed.

How do other projects handle using them as references? When they're included indirectly as they would be when using CefSharp would uses run into problems deploying their app with dlls not being available on non dev machines?

Copy link
Author

Choose a reason for hiding this comment

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

AFAIK they will be exported into the bin folder along with all the rest, just like a framework you might have added as a Nuget Package.

Copy link
Member

Choose a reason for hiding this comment

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

If you include them directly, they'll use Copy Local = True, if they're included indirectly, like through a 3rd party lib then are they copied?

Copy link
Member

Choose a reason for hiding this comment

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

I Cloned the original project and it doesn't build, some projects appear to be missing, I didn't read any tutorials on the process of contributing to the main project, so I am sorry if this is explained there somewhere.

You'll need to install the VC++ Development tools (from memory they are included by default with VC++, maybe someone unchecked them upon install?)

Adding this behavior into the main project would just need to add the references to these new dlls to the Nuget package, what else should be needed?

I am not understanding your concern with deployment, maybe I am not experienced enough on deployment. IMHO the dlls will be copied to the output folder and the Nuget Package will include them, the only side effect I can see is increasing the project deployment size

It's quite a bit more complicated than that. Using MahApps as an example, if I search their issue tracker e.g. https://github.com/MahApps/MahApps.Metro/search?utf8=%E2%9C%93&q=System.Windows.Interactivity.dll&type=Issues I see there are a number of versioning issues.

but I can fix this changing the Behavior from a Blend Behavior to an Attached Property Behavior

This maybe the simplest option for now.

Copy link
Author

Choose a reason for hiding this comment

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

I have Visual C++ 2015 in my Visual Studio about page and I do c++ development on this machine some times... that's weird.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like it was unable to load the VC++ projects. I usually use 2013 as the VS version is linked to the VC++ version (which VC++ 2013 is the current requirement). Last I checked it was working in 2015, would have been a few weeks ago the last time I checked.

CI server says latest build from master was build successfully https://ci.appveyor.com/project/cefsharp/cefsharp/build/55.0.0-CI2059

Copy link
Author

Choose a reason for hiding this comment

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

Project 'CefSharp.Core' could not be loaded because it's missing install components. To fix this launch Visual Studio setup with the following selections:
Install Visual C++ 2015 Tools for Windows Desktop

That's weird, I do program with C++ here...but I will install those then.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I got it to compile, there was a c++ tool missing indeed.

@amaitland
Copy link
Member

It's probably a timing issue, have a look at the source for LoadHtml, it does two things. In WPF, your best to set the Address property as it is an actual DependencyProperty. If all you care about is loading some Html (and no what the address bar url looks) then just generate a Data Uri.

As a general comment, don't you think this deviates aware from the MinimalExample implication? (I know your just trying to get your behavior working at the moment).

@amaitland
Copy link
Member

Also it looks like you've used features that aren't available in .Net 4.5.2, which should probably stay as the default as that's the minimum requirement to use the current version.

return;
}

(d as ChromiumWebBrowser)?.LoadHtml((string)e.NewValue, "http://test/page");
Copy link
Member

Choose a reason for hiding this comment

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

It would appear that DependencyObject d is actually the behavior (not really surprising).

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I have seen this, I changed the code from an Attached Property Behavior to a Blend Behavior and things got messy. I am not very experienced with Blend Behaviors and lacks tutorials on how to get the AttachedObject inside the change handler, I will bring it back to an Attached Property Behavior and save the trouble.

Copy link
Member

Choose a reason for hiding this comment

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

private static void OnHtmlChanged(DependencyObject d, DependencyPropertyChangedEventArgs e)
{
    if (e.NewValue == null || string.IsNullOrWhiteSpace((string)e.NewValue))
    {
        return;
    }

    var behaviour = (LoadHtmlBehavior)d;

    var chromiumWebBrowser = behaviour.AssociatedObject as ChromiumWebBrowser;
    if(chromiumWebBrowser != null)
    {
        chromiumWebBrowser.LoadHtml((string)e.NewValue, behaviour.HtmlUrl);
    }
}

Copy link
Author

Choose a reason for hiding this comment

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

Oh, good catch 👍

Copy link
Author

@feinstein feinstein Oct 28, 2016

Choose a reason for hiding this comment

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

Fixed as

(d as LoadHtmlBehavior)?.AssociatedObject.LoadHtml((string)e.NewValue, "about:blank");

@feinstein
Copy link
Author

If all you care about is loading some Html

What else should LoadHtml do? I don't get it's purpose for something else. Should we add a LoadHtml that only takes one parameter, the HTML as a string and render it?

In my application, using a default WPF WebBrowser, I bind a ICollectionView of strings with HTMLs to a ListBox, and the CurrentItem is bound in XAML with the WebBrowser Address, so just as the user clicks a new ListBoxItem the item is rendered automagically into the WebBrowser. I was expecting to do something similar here and through a behavior was my first idea for binding the string directly.

As a general comment, don't you think this deviates aware from the MinimalExample implication?

If we should provide a minimum WPF MVVM Example, no, but if it's meant to be a quick WPF example, than yes. But the behaviors I would remove them and transfer to the other project, I just thought a minimal example was good to test this kind of stuff. Honestly, whatever you decide on this I back you 100%, it's a bit gray area.

@feinstein
Copy link
Author

Also it looks like you've used features that aren't available in .Net 4.5.2

Like what?

@amaitland
Copy link
Member

Like what?

Null Conditional Operator e.g. _TargetExecuteMethod?.Invoke((T)parameter); won't work in .Net 4.5.2

@feinstein
Copy link
Author

Isn't the Null Conditional Operator a C# 6.0 feature and not a .NET feature? So it's up to the compiler (VS version) and not .NET (since the framework will just be interpreting the CLI compiled by VS)?

@amaitland
Copy link
Member

Isn't the Null Conditional Operator a C# 6.0 feature and not a .NET feature? So it's up to the compiler (VS version) and not .NET (since the framework will just be interpreting the CLI compiled by VS)?

It is compiler magic. If you don't happen to have .Net 4.6 installed you won't be able to compile. Also if you open in VS2013 and only target .Net 4.5.2 (Like I did on this machine) you'll have compiler errors.

@feinstein
Copy link
Author

Ok, I will remove the ? then.


(d as ChromiumWebBrowser)?.LoadHtml((string)e.NewValue, "http://test/page");
(d as LoadHtmlBehavior)?.AssociatedObject.LoadHtml((string)e.NewValue, "about:blank");
Copy link
Member

Choose a reason for hiding this comment

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

HtmlUrl property isn't being used. Also best not o use about:blank, as it's a special case. You'd be effectively registering a ResourceHandler that gets loaded every time about:blank is required.

Copy link
Author

Choose a reason for hiding this comment

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

I see...so "http://test/page" is a better fit?

Should we add a new method to load an HTML directly through one parameter?

Could you explain me briefly the intent of the LoadHtml with an URL?

@feinstein
Copy link
Author

This last update works! Should I leave it like this or should we improve the library instead an remove the URL?

@feinstein
Copy link
Author

I am reading the LoadHtml code and it appears to be registering a new Handler to a Dictionary every time a new HTML is loaded. Are the handlers removed at any point? Can't this lead to a memory leak?

@amaitland
Copy link
Member

Are the handlers removed at any point?

Automatically no. They are overwritten if you register the same Url though.

Can't this lead to a memory leak?

It can of course. It's a very simplistic implementation, just there for convenience. You can of course take full control and even implement your own IResourceHandlerFactory. The default implementation has a method to remove a Url. You'd have to call it manually,

The alternative is a Data Uri, like https://github.com/cefsharp/CefSharp/blob/a94bbdd593fb72a2edc92fcfe4987c505802e768/CefSharp.WinForms.Example/BrowserForm.cs#L475
(Base64 encoding is just one option) see https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/Data_URIs

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