Switch to use MSWebBrowser as default lib display by QilongTang · Pull Request #11957 · DynamoDS/Dynamo

QilongTang · 2021-08-23T14:06:49Z

Please Note:

Before submitting the PR, please review How to Contribute to Dynamo
Dynamo Team will meet 1x a month to review PRs found on Github (Issues will be handled separately)
PRs will be reviewed from oldest to newest
If a reviewed PR requires changes by the owner, the owner of the PR has 30 days to respond. If the PR has seen no activity by the next session, it will be either closed by the team or depending on its utility will be taken over by someone on the team
PRs should use either Dynamo's default PR template or one of these other template options in order to be considered for review.
PRs that do not have one of the Dynamo PR templates completely filled out with all declarations satisfied will not be reviewed by the Dynamo team.
PRs made to the DynamoRevit repo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after a LGTM label is added to the PR.

Purpose

Switch to use MSWebBrowser as default:

LibraryViewExtensionMSWebBrowser will build to Dynamo build folder
LIbraryViewExtension will build to its own bin folder
When launching Dynamo, Dynamo will load LibraryViewExtensionMSWebBrowser as default lib view extension
Dynamo folder will be missing from a few files including all the CEFSharp dependencies and locales

What it looks like, notice the scroll bar

What the build contains for LibraryViewExtension which is no longer part of DynamoCore:

Declarations

Check these if you believe they are true

The codebase is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning and are documented in the API Changes document.
This PR modifies some build requirements and the readme is updated

Reviewers

@DynamoDS/dynamo

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

Switch to use MSWebBrowser as default: 1. LibraryViewExtensionMSWebBrowser will build to Dynamo build folder 2. LIbraryViewExtension will build to its own bin folder

src/LibraryViewExtensionMSWebBrowser/LibraryViewExtensionMSWebBrowser.csproj

mjkkirschner · 2021-08-23T14:49:33Z

are you concerned about the missing files for patch installers? Will you be manually copying them back in after build?

QilongTang · 2021-08-23T14:53:31Z

are you concerned about the missing files for patch installers? Will you be manually copying them back in after build?

Since this would be for Global launch, I am not worried about the missing files for patch installers yet. I do remember Civil3D may back port updated Dynamo there but since they were already using MSWebBrowser, seems no issue should be raised on that end?

mjkkirschner · 2021-08-23T18:11:25Z

@QilongTang I'm not sure why I didn't do this in the past - maybe there is a reason - but can you checkout why layoutspec.json is duplicated in both projects? It would be nice if one was a link to the other so we only had to update it once in one spot when adding new nodes - or do you intend to drop the CEF project completely?

I mean adding existing item to project, and then using add file link:

QilongTang · 2021-08-24T12:15:38Z

@QilongTang I'm not sure why I didn't do this in the past - maybe there is a reason - but can you checkout why layoutspec.json is duplicated in both projects? It would be nice if one was a link to the other so we only had to update it once in one spot when adding new nodes - or do you intend to drop the CEF project completely?

I mean adding existing item to project, and then using add file link:

@mjkkirschner I found that using linked layoutSpec will break the library display so I left it untouched

mjkkirschner · 2021-08-24T15:00:09Z

@QilongTang I'm not sure why I didn't do this in the past - maybe there is a reason - but can you checkout why layoutspec.json is duplicated in both projects? It would be nice if one was a link to the other so we only had to update it once in one spot when adding new nodes - or do you intend to drop the CEF project completely?
I mean adding existing item to project, and then using add file link:

@mjkkirschner I found that using linked layoutSpec will break the library display so I left it untouched

yeah - odd, the file needs to be embedded into the binary for it to be found at runtime, but maybe that doesn't make sense with a linked file - anyhow lets worry about it later - eventually we'll only have one of these projects anyway.

mjkkirschner · 2021-08-24T15:01:21Z

@QilongTang I'm confused - in your image above you show a bunch of CEF dependencies in the bin folder, I thought you were getting rid of them as per this comment: Dynamo folder will be missing from a few files including all the CEFSharp dependencies and locales

mjkkirschner · 2021-08-24T15:02:27Z

src/LibraryViewExtension/web/library/layoutSpecs.json

      "elementType": "section",
      "showHeader": false,
-      "include": [ ],
+      "include": [],


what are these changes about?

Visual Studio Code Json formatting

QilongTang · 2021-08-24T15:02:38Z

@QilongTang I'm confused - in your image above you show a bunch of CEF dependencies in the bin folder, I thought you were getting rid of them as per this comment: Dynamo folder will be missing from a few files including all the CEFSharp dependencies and locales

Yeah, please check the path, these are under LibraryViewExtension sub folder for future nuget wrapping

mjkkirschner · 2021-08-24T15:03:20Z

src/LibraryViewExtensionMSWebBrowser/ViewExtension.cs

        }

-        public static readonly string ExtensionName = "LibraryUI2";
+        public static readonly string ExtensionName = "LibraryUI - MSWebBrowser";


my reasoning for this funny naming was to avoid exposing the implementation in the name - not a big deal either way.

I can revert this or do you have an alternative naming option?

nah, it's fine - lgtm.

mjkkirschner

looks good - just a few questions.

QilongTang · 2021-08-25T06:59:12Z

There are lots of regressions, investigating

src/AssemblySharedInfoGenerator/AssemblySharedInfo.cs

QilongTang · 2021-08-25T13:59:37Z

The previous failures all pass locally for me. Merging.

QilongTang added 2 commits August 23, 2021 22:03

Switch to use MSWebBrowser as default

0661b46

Switch to use MSWebBrowser as default: 1. LibraryViewExtensionMSWebBrowser will build to Dynamo build folder 2. LIbraryViewExtension will build to its own bin folder

Update extension name to make it more obvious

293655c

mjkkirschner reviewed Aug 23, 2021

View reviewed changes

src/LibraryViewExtensionMSWebBrowser/LibraryViewExtensionMSWebBrowser.csproj Outdated Show resolved Hide resolved

QilongTang added 3 commits August 24, 2021 17:07

Use CS.props

669a12c

Format Clean

8509a33

Do not copy dependencies

f2871b2

QilongTang marked this pull request as ready for review August 24, 2021 12:08

QilongTang requested review from mjkkirschner, reddyashish and zeusongit August 24, 2021 12:16

mjkkirschner reviewed Aug 24, 2021

View reviewed changes

mjkkirschner approved these changes Aug 24, 2021

View reviewed changes

reddyashish reviewed Aug 25, 2021

View reviewed changes

src/AssemblySharedInfoGenerator/AssemblySharedInfo.cs Outdated Show resolved Hide resolved

Revert build number change

e51d6c2

QilongTang merged commit db4c123 into master Aug 25, 2021

QilongTang deleted the SwitchDefaultLibrary branch August 25, 2021 13:59

mjkkirschner mentioned this pull request Aug 25, 2021

Add more info to CrashPrompt #11974

Merged

Conversation

QilongTang commented Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Declarations

Reviewers

FYIs

Uh oh!

Uh oh!

mjkkirschner commented Aug 23, 2021

Uh oh!

QilongTang commented Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjkkirschner commented Aug 23, 2021

Uh oh!

QilongTang commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjkkirschner commented Aug 24, 2021

Uh oh!

mjkkirschner commented Aug 24, 2021

Uh oh!

mjkkirschner Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

QilongTang Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

QilongTang commented Aug 24, 2021

Uh oh!

mjkkirschner Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

QilongTang Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

mjkkirschner left a comment

Choose a reason for hiding this comment

Uh oh!

QilongTang commented Aug 25, 2021

Uh oh!

Uh oh!

QilongTang commented Aug 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

QilongTang commented Aug 23, 2021 •

edited

Loading

QilongTang commented Aug 23, 2021 •

edited

Loading

QilongTang commented Aug 24, 2021 •

edited

Loading