weixin_39841640
weixin_39841640
2020-12-31 12:53

Namespace Import List for VB Reference Property Page

This change enables the namespace import list for VB in Reference Property page

With the current implementation, we have an timing issue which does not reflect the current state of the import list properly after a change to the list, unless the user moves to a different active page and comes back to the Property page again. Any change performed without switching to a different page will result in a undefined behavior. The problem is, the refresh operation, to update the list, is performed before the Design time build, which is triggered by adding or removing an import. Hence the UI reflects the old state and the MSBuild Evaluation model reflects the new state.

This issue could be resolved if the change is pushed to Roslyn workspace before the refresh operation, which is performed after the change. is working on pushing some information to Roslyn Workspace based on Evaluation rather than the Design time build, which takes more time, which results in the delay. Until this change, the above mentioned issue will persist. Atleast the page is not longer broken with this change :)

Other FYIs, 1. Check to identify the Roslyn Project to query for current Global imports and namespaces uses Project file path instead of VsHierarchy in ReferencePropPage. We are introducing this change because, with Multi TFMs, the Hierarchy held by the reference page is no longer the same as the Hierarchy provided for a ConfiguredProject(innerbuild)

  1. There are no tests for VSImports methods which uses IProjectLockService since the access token returned by the service, to modify the Project file, is a struct(ProjectLockReleaser and ProjectWriteLockReleaser) with no virtual methods to mock.

Tagging /project-system for review

该提问来源于开源项目:dotnet/project-system

  • 点赞
  • 写回答
  • 关注问题
  • 收藏
  • 复制链接分享
  • 邀请回答

15条回答

  • weixin_39841640 weixin_39841640 4月前

    There's two issues here; a delay before Roslyn sees the changes, and the requirement that you need to switch pages to see changes

    The need to do the page switch is, because when the control comes back to the Property page, the page is refreshed, At this point, the Design time build is done and the list gets refreshed to the correct value.

    With your change, if the imports list change were pushed to Roslyn Workspace through evaluation then the change could reach faster. Hence the page refresh that happens at the end of UI operation could get the updated list. This eliminates the need to do a page-switch. Of course, pushing the change to Roslyn Workspace could still be not fast enough. At that point, we will have to look for other ways to make this work.

    点赞 评论 复制链接分享
  • weixin_39632467 weixin_39632467 4月前

    In other words - changing the page is just a workaround to get the page to refresh again. You could also just close and reopen the page after a pause to get the same effect I assume.

    点赞 评论 复制链接分享
  • weixin_39632467 weixin_39632467 4月前

    Actually no - regardless of how fast we make the data reach roslyn, there's an inherent race here. To fix this deterministically we need to wait on some event - probably Workspace_Changed and verify that the import has reached Roslyn before we refresh the page.

    点赞 评论 复制链接分享
  • weixin_39841640 weixin_39841640 4月前

    Actually no - regardless of how fast we make the data reach roslyn, there's an inherent race here. To fix this deterministically we need to wait on some event - probably Workspace_Changed

    Agreed. There is definitely a race here. With change, we can check how worse(or not) the race is.

    点赞 评论 复制链接分享
  • weixin_39690972 weixin_39690972 4月前

    To fix this deterministically we need to wait on some event - probably Workspace_Changed and verify that the import has reached Roslyn before we refresh the page.

    You need to be careful to avoid deadlocks when waiting on anything from the Roslyn workspace on the UI thread. All pushes from CPS to Roslyn workspace are scheduled to execute on the UI thread, and may never execute if you are waiting for it to complete from the UI itself (property pages).

    点赞 评论 复制链接分享
  • weixin_39841640 weixin_39841640 4月前

    Opened #1922 to address the race mentioned above.

    Could someone please review the change?

    点赞 评论 复制链接分享
  • weixin_39837607 weixin_39837607 4月前

    Why are you implementing IConnectionPointContainer if you never raise the events? That will be why the page never updates - it never gets told that imports have changed. I'm also not convinced on the design of working off the live project, you have major performance, consistency and multi-threading issues with that approach - I'd really, really like to know if this is the way CPS implements this. Im positive that VSLangProj.References do not work like this (they are always have a consistent state), and I think that Imports should work the same way.

    点赞 评论 复制链接分享
  • weixin_39841640 weixin_39841640 4月前

    addressed your concerns

    点赞 评论 复制链接分享
  • weixin_39837607 weixin_39837607 4月前

    Can you answer this statement from above?

    I'm also not convinced on the design of working off the live project, you have major performance, consistency and multi-threading issues with that approach - I'd really, really like to know if this is the way CPS implements this. Im positive that VSLangProj.References do not work like this (they are always have a consistent state), and I think that Imports should work the same way.

    Basically with the current design, if I write a for loop over the Imports - there's zero guarantees that the project won't change under me. Whereas, in the legacy project system - there was a guarantee. I really want this implemented like References.

    点赞 评论 复制链接分享
  • weixin_39841640 weixin_39841640 4月前

    Basically with the current design, if I write a for loop over the Imports - there's zero guarantees that the project won't change under me. Whereas, in the legacy project system - there was a guarantee. I really want this implemented like References.

    I have addressed this by introducing a list whose enumerator is returned when asked for an enumerator from VSImports. This list is actively updated as we make changes to the list. This way, the enumerator will react to the changes to the import list and the users of the enumerator can respond accordingly.

    点赞 评论 复制链接分享
  • weixin_39632467 weixin_39632467 4月前

    :shipit:

    点赞 评论 复制链接分享
  • weixin_39841640 weixin_39841640 4月前

    Tagging for approval

    点赞 评论 复制链接分享
  • weixin_39811166 weixin_39811166 4月前

    Approved pending Dave's signoff.

    点赞 评论 复制链接分享
  • weixin_39841640 weixin_39841640 4月前

    Tagging for one final review :)

    点赞 评论 复制链接分享
  • weixin_39837607 weixin_39837607 4月前

    With the current implementation, we have an timing issue which does not reflect the current state of the import list properly after a change to the list, unless the user moves to a different active page and comes back to the Property page again. Any change performed without switching to a different page will result in a undefined behavior. The problem is, the refresh operation, to update the list, is performed before the Design time build, which is triggered by adding or removing an import. Hence the UI reflects the old state and the MSBuild Evaluation model reflects the new state.

    There's two issues here; a delay before Roslyn sees the changes, and the requirement that you need to switch pages to see changes. I'm fixing the former, why would the later be fixed by pushing these changes earlier?

    点赞 评论 复制链接分享

相关推荐