Bump mac editor and implement CocoaTextBufferVisibilityTracker #65568
Conversation
ghost
added
the
Community
src/EditorFeatures/Core.Cocoa/Workspaces/CocoaTextBufferVisibilityTracker.cs
Outdated
Show resolved
Hide resolved
|
I tried multiple ways of fixing the duplication, but I end up with an issue where Can we take this as-is for now? |
This should help reduce the number classification marks triggering in the editor in VSMac. Fixes https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1654358
Therzok
force-pushed
the
dev/therzok/cocoa-text-buffer-visibility-tracker
branch
2 times, most recently
from
aa12f5a
to
60505e5
Compare
|
@Therzok Sorry, i should have been more clear on how to do it. I've made the changed and pushed to your branch. Let me know how it looks for you. |
eng/Versions.props
Outdated
| @@ -28,7 +28,7 @@ | |||
| <VisualStudioEditorPackagesVersion>17.5.42-preview</VisualStudioEditorPackagesVersion> | |||
| <!-- This should generally be set to $(VisualStudioEditorPackagesVersion), | |||
| but sometimes EditorFeatures.Cocoa specifically requires a newer editor build. --> | |||
| <VisualStudioMacEditorPackagesVersion>17.3.133-preview</VisualStudioMacEditorPackagesVersion> | |||
| <VisualStudioMacEditorPackagesVersion>17.5.152-preview-g356e9d7141</VisualStudioMacEditorPackagesVersion> | |||
There was a problem hiding this comment.
@genlu @allisonchou what branch should this go into? main-vs-deps?
There was a problem hiding this comment.
We should be able to bump this to a standard daily build version, yes (won't have a commit hash suffix)?
There was a problem hiding this comment.
Yeah, this is a package from a PR build. Let's change this to 17.5.157-preview (or 17.5.170-preview if we want latest).
There was a problem hiding this comment.
Actually VisualStudioEditorPackagesVersion should be bumped and this property's value should be changed to $(VisualStudioEditorPackagesVersion). But that can happen in another PR.
There was a problem hiding this comment.
Given this (hopefully?) only affects Mac we shouldn't need to worry about vs-deps. If we did bump VisualStudioEditorPackagesVersion then we might
| { | ||
| private readonly ITextBufferAssociatedViewService _associatedViewService; |
There was a problem hiding this comment.
all this moved to abstract base class.
| where TTextView : ITextView | ||
| where TVisibilityChangedCallback : System.Delegate | ||
| { | ||
| private readonly ITextBufferAssociatedViewService _associatedViewService; |
There was a problem hiding this comment.
all this state and logic is the same. just pulled up into base class. i have marked where they differ.
| protected abstract bool IsVisible(TTextView view); | ||
| protected abstract TVisibilityChangedCallback GetVisiblityChangeCallback(VisibleTrackerData visibleTrackerData); | ||
| protected abstract void AddVisibilityChangedCallback(TTextView view, TVisibilityChangedCallback visibilityChangedCallback); | ||
| protected abstract void RemoveVisibilityChangedCallback(TTextView view, TVisibilityChangedCallback visibilityChangedCallback); |
There was a problem hiding this comment.
abstract functionality for where wpf and cocoa need to specialize.
| if (views.Any(static v => v is not TTextView)) | ||
| return true; | ||
|
|
||
| return views.OfType<TTextView>().Any(v => IsVisible(v)); |
There was a problem hiding this comment.
defer to IsVisible here to do platform specific determination.
| { | ||
| _tracker = tracker; | ||
| _subjectBuffer = subjectBuffer; | ||
| _visibilityChangedCallback = tracker.GetVisiblityChangeCallback(this); |
There was a problem hiding this comment.
defer to platform specific code to get the callback with which to register to visibility changed event.
| foreach (var addedView in addedViews) | ||
| { | ||
| if (addedView is TTextView genericView) | ||
| _tracker.AddVisibilityChangedCallback(genericView, _visibilityChangedCallback); |
There was a problem hiding this comment.
defer to platform specific code to attach/detach the visibility changed listener.
|
@jasonmalinowski ptal. |
| { | ||
| _tracker._threadingContext.ThrowIfNotOnUIThread(); | ||
| protected override EventHandler GetVisiblityChangeCallback(VisibleTrackerData visibleTrackerData) | ||
| => (sender, args) => visibleTrackerData.TriggerCallbacks(); |
There was a problem hiding this comment.
Ah, heh, I didn't think of wrapping in the derived impl.
There was a problem hiding this comment.
Thanks for the changes!
No worries! The problem I was having is that I didn't have a plain EventHandler. My WIP work was similar to this, except that I was doing generic work over the event args, rather than the eventhandler delegate itself. Thanks for fixing it up. |
|
Yeah, sorry, I was using a PR build to be able to validate the changes first. |
This should help reduce the number classification marks triggering in the editor in VSMac. Fixes https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1654358 Backport of #65568 without the refactoring.
|
@CyrusNajmabadi @davidwengier can we merge this now? |
|
yup yup! |
This should help reduce the number classification marks triggering in the editor in VSMac.
Uses newly added API in VS-Platform, 0f93736f636b83a30c3a0b393b89afbe882de5e3.
Fixes https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1689011