Open mitchcapper opened 1 month ago
:x: Build CefSharp 129.0.110-CI5085 failed (commit https://github.com/cefsharp/CefSharp/commit/24888a292b by @mitchcapper)
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.
HwndHost
example could then be moved into the MinimalExample
repoThe only problem is the Nuget
package references both CefSharp.Common
and CefSharp.Common.NETCore
.
HwndHost
out into two packages like the rest.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).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?
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.
The question becomes which approach? As a sub folder in the Wpf
package? or it's own package entirely?
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.
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.
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.
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)
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
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))
Enabled when compile time define of CEFSHARP_WPF_HWNDHOST is used
Summary:
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 theCefSharp.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
Checklist: