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

CefSharp.Wpf.HwndHost support added to sample app and master solution #4964

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mitchcapper
Copy link
Contributor

Enabled when compile time define of CEFSHARP_WPF_HWNDHOST is used

Summary:

  • Given the larger divergence between CefSharp.Wpf.HwndHost and the normal Wpf library for what they support, expanding the test ability on HwndHost seems like a good idea. Functionally this is a no-op unless #CEFSHARP_WPF_HWNDHOST compile time define is set. Then it uses HwndHost rather than CefSharp.Wpf.

Changes: [specify the structures changed]
The biggest change of debate is ChromiumWebBrowser and xaml. I wanted to minimize the changes but don't want things to lead to too much confusion. Right now that is done by adding a new
ChromiumWebBrowser class in the CefSharp.Wpf.Example.Controls namespace. Originally I named it SampleChromiumWebBrowser but that results in further xaml changes. I don't think possible to get it under the CefSharp.Wpf namespace without the xaml assembly reference when not using the HwndHost.

One option would be to add the fake ChromiumWebBrowser class under the CefSharp namespace in the sample app. This would avoid all the additional includes of the example control namespace. That could easily cause more confusion for users if they did copy the namespace given ChromiumWebBrowser is not under that namespace normally and its not obvious that is the issue.

So, in short right now you can copy the sample xaml and just have to add the proper namespace include at the top (which vs will auto suggest) but can't copy that part directly.

I moved to using the hwndhost on the sample for awhile now, I feel most airspace issues can be worked around with regions and otherwise it seems HwndHost is fairly amazing. Haven't quite been able to move over to using it for most apps as I work around some of the crashes I am able to run into but I am still hopeful for it. Hopefully expanding the sample can get some others to see the lack of issues with HwndHost.

Should also make it easier for tickets without having to switch to WinForms for easier repro for Chrome style with extension support.

Aside from just the define it is possible to add another Configuration beyond Debug/Release (ie Debug Wpf.HwndHost) that would auto enable the define.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • [ x ] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Updated documentation

Checklist:

  • Tested the code(if applicable)
  • Commented my code
  • Changed the documentation(if applicable)
  • New files have a license disclaimer
  • [ x ] The formatting is consistent with the project (project supports .editorconfig)

Enabled when compile time define of CEFSHARP_WPF_HWNDHOST is used
@AppVeyorBot
Copy link

@amaitland
Copy link
Member

Adding a submodule for a project that is effectively a dependency of this project seems like it will be problematic.

I'd actually prefer to bring the HwndHost source into this repository instead.

  • Then I could just tag a single release
  • Upgrade script would deal with everything.
  • Could use the same partial class and remove code duplication
  • The HwndHost example could then be moved into the MinimalExample repo
  • We could look at a way for the WPF example to load the different control based on a UI option or something like that

The only problem is the Nuget package references both CefSharp.Common and CefSharp.Common.NETCore.

  • Could split the HwndHost out into two packages like the rest.
  • Another option would be to have three packages, one HwndHost.NETCore, HwndHost.NETFramework and the HwndHost package that references both (I'd toyed with this idea for all the packages, so you just reference one and VS picks the dependency based on the .net version).
  • Could move into a subfolder of the Wpf project, the namespace is already effectively a subfolder, there would be advantages to this, they could implement the same interface, easily share code.

Thoughts?

@mitchcapper
Copy link
Contributor Author

Yeah adding hwndhost to the repo would make the most sense I was going for minimal impact. It is possible to merge the history in as well with allow-unrelated-histories without any negative effects (if so desired). May need to strip some root items out but that might be better than an additional sub dir and git move in terms of easier history.

Given everything else is split .netcore and Framework would seem to make logical sense to split it like the rest. Certainly a fan of the code de-duplication especially given how similar it is to the WPF native control.

It may make sense to offer the combined nuget that references both netcore and framework nuget packages so the current users of https://www.nuget.org/packages/CefSharp.Wpf.HwndHost don't have an upgrade issue.

@amaitland
Copy link
Member

The question becomes which approach? As a sub folder in the Wpf package? or it's own package entirely?

@mitchcapper
Copy link
Contributor Author

I like the fact with the separate package you know for sure you won't be referencing the wrong type somewhere I uninstall Wpf and install Wpf.HwndHost and there is no way for me to be using the wrong one. Then again their feature sets are so close one could do some magic and make it a CefSharpSettings. toggle that would throw an exception for the few methods not avail in HwndHost.

I think a single assembly probably gives the most seamless user experience. The biggest problem would be the CefSharp.WPF namespace contains the non-hwnd control along with many other things so you would almost certainly have to alias the using for ChromiumWebBrowser for hwnd.

There is already IWpfWebBrowser which is basically an interface abstraction based on the HwndHost version given it mostly being the lowest common denominator. That could be expanded to allow a TogglableWebBrowser (no idea on a good name) where you could switch between WPF/HwndHost while still maintaining the two separate controls for users who prefer that.

@amaitland
Copy link
Member

I like the fact with the separate package you know for sure you won't be referencing the wrong type somewhere I uninstall Wpf and install Wpf.HwndHost and there is no way for me to be using the wrong one

Do you see this as being a major concern?

Based on the discussion I'm inclined to merge into the existing Wpf library, removing the code duplication would be a big win for me.

@mitchcapper
Copy link
Contributor Author

Do you see this as being a major concern?

No, just the advantage i saw of a separate package when migrating. The many perks of migrating it into the existing WPF seems great.

@amaitland
Copy link
Member

I tried a subtree merge as described in https://mirrors.edge.kernel.org/pub/software/scm/git/docs/howto/using-merge-subtree.html

Result is at https://github.com/cefsharp/CefSharp/tree/merge/hwndhost with the project going into a HwndHost subfolder (temporarily was the plan so didn't get clash with duplicate existing files)

I can see the commits in the history of the branch, there's no individual file history thought.

Might have to try another option, otherwise just ignore the history (somewhat leaning towards this as we can refer to the old repo)

@amaitland
Copy link
Member

I don't want to rewrite the history of the main CefSharp repo, looks like that will limit the options.

Might just skip trying to bring over the history unless someone has a better suggestion

@mitchcapper
Copy link
Contributor Author

mitchcapper commented Nov 9, 2024

Sorry I use a helper for the git complexity as no way I want to lookup all these commands.

You have to rewrite the CefSharp.Wpf.hwndHost history, (aka the hashes for it will change) but the master repo history is left clean.

As you are going to delete the separate repo though I don't think there is a reason not to do it.

Here is the merged:
https://github.com/mitchcapper/_ceftest

Below are the commands, I do have the slew of unix commands in my primary PATH but I think the ones it uses are in gits own shell on windows so should be fine either way. Some commands are c# but im assuming it is obvious enough.

Note the directory move and move back is not required it just does that to make sure there isn't anything holding a lock on the directory prior to it needing to move it later on.
The filter branch command is rewriting all the commits in Wpf.HwndHost and changing every file to have the new root of Wpf.HwndHost

Note, if you want, you could use a more complex set of filter commands to not do the subdir and instead remove all the root files and .github dir (leaving only the CefSharp.Wpf.HwndHost and CefSharp.Wpf.HwndHost.Example subdirs).
This would give you the cleanest merge.

mkdir cefmerge_test
cd cefmerge_test
git clone https://Github.com/cefsharp/CefSharp .
git clone https://Github.com/cefsharp/CefSharp.Wpf.HwndHost
() => Directory.Move(full_path, $"{full_path}-SRC") (0.1 Directory.Move(x:\cefmerge_test\CefSharp.Wpf.HwndHost, x:\cefmerge_test\CefSharp.Wpf.HwndHost-SRC))
() => Directory.Move($"{full_path}-SRC", full_path) (0.3 Directory.Move(x:\cefmerge_test\CefSharp.Wpf.HwndHost-SRC, x:\cefmerge_test\CefSharp.Wpf.HwndHost))
() => Directory.SetCurrentDirectory(directory) (directory=x:\cefmerge_test\)
() => Directory.Move(full_path, $"{full_path}-SRC") (0.1 Directory.Move(x:\cefmerge_test\CefSharp.Wpf.HwndHost, x:\cefmerge_test\CefSharp.Wpf.HwndHost-SRC))
() => Directory.SetCurrentDirectory(directory) (directory=x:\cefmerge_test\CefSharp.Wpf.HwndHost-SRC)
git filter-branch --index-filter "git ls-files -s | sed \"s/\t/\tCefSharp.Wpf.HwndHost\//\" | GIT_INDEX_FILE=$GIT_INDEX_FILE.new git update-index --index-info && if [ -f \"$GIT_INDEX_FILE.new\" ]; then mv ${GIT_INDEX_FILE}.new $GIT_INDEX_FILE; fi" HEAD
() => Directory.SetCurrentDirectory(directory) (directory=x:\cefmerge_test\)
git remote add -f loc_merge "x:\cefmerge_test\CefSharp.Wpf.HwndHost-SRC"
git merge --allow-unrelated-histories loc_merge/master
git remote remove loc_merge
() => Directory.SetCurrentDirectory(directory) (directory=x:\cefmerge_test\CefSharp.Wpf.HwndHost-SRC)
() => ForceDeleteWithRODirectory($"{repo_name}") (5 ForceDeleteWithRODirectory(CefSharp.Wpf.HwndHost))
Will force delete even r/o dir: CefSharp.Wpf.HwndHost
() => ForceDeleteWithRODirectory(".git") (15 ForceDeleteWithRODirectory(.git))
Will force delete even r/o dir: .git
() => Directory.SetCurrentDirectory(directory) (directory=x:\cefmerge_test\)
() => ForceMoveRODirectory($"{full_path}-SRC", repo_name) (10 ForceMoveRODirectory(x:\cefmerge_test\CefSharp.Wpf.HwndHost-SRC, CefSharp.Wpf.HwndHost))

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