-
Notifications
You must be signed in to change notification settings - Fork 1k
[Addin] To allow CBinding to use "Project" property #2870
base: main
Are you sure you want to change the base?
Conversation
Hello! I'm the build bot for the Mono project. I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done. Additional trigger words are available here. Contributors can ignore this message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there! Just a few changes for consistency and backwards-compatibility.
@@ -56,7 +56,7 @@ public ConnectedServicesWidget () | |||
/// <summary> | |||
/// Shows the services gallery and removes the details widget if it is visible | |||
/// </summary> | |||
public void ShowGallery(IConnectedService[] services, Project project) | |||
public void ShowGallery(IConnectedService[] services, SolutionItem owner) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an overload that takes a Project and calls the SolutionItem version, so that the public API doesn't change.
@@ -88,6 +88,12 @@ protected override void OnSetProject (MonoDevelop.Projects.Project project) | |||
content.Project = project; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to remove the OnSetProject as it's handled by OnSetOwner.
@@ -115,6 +115,12 @@ protected override void OnSetProject (Projects.Project project) | |||
content.Project = project; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to remove the OnSetProject as it's handled by OnSetOwner.
@@ -38,9 +38,9 @@ public static GettingStartedProvider GetGettingStartedProvider (this Project pro | |||
return null; | |||
} | |||
|
|||
public static GettingStartedNode GetGettingStartedNode (this Project project) | |||
public static GettingStartedNode GetGettingStartedNode (this SolutionItem owner) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a Project overload that chains to SolutionItem.
@@ -142,6 +143,20 @@ internal protected virtual void OnDeselected () | |||
protected virtual void OnSetProject (Project project) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be [Obsoleted]
; subclasses should override Owner
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the Project
property should chain to Owner
, i.e.
public Project Project {
get {
return Owner as Project;
}
set {
OnSetOwner(project);
}
}
|
||
protected virtual void OnSetOwner (SolutionItem owner) | ||
{ | ||
this.owner = owner; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also call OnSetProject() if the owner is a project.
if (owner is Project project)
{
OnSetProject(project);
}
@@ -55,6 +55,7 @@ public sealed class ParseOptions | |||
public ITextSource Content { get; set; } | |||
|
|||
public MonoDevelop.Projects.Project Project { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also forward to Owner
.
@@ -92,7 +92,7 @@ public abstract class ViewContent : BaseViewContent | |||
public virtual object GetDocumentObject () | |||
{ | |||
string path = IsUntitled ? UntitledName : ContentName; | |||
if (IsFile && !string.IsNullOrEmpty (path) && Project != null) { | |||
if (IsFile && !string.IsNullOrEmpty (path) && Owner != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be changed, since we use the Project property just below, so Project must be != null.
It looks good overall! |
@@ -524,6 +533,18 @@ internal Task<Document> OpenDocument (FilePath fileName, Project project, int li | |||
return OpenDocument (openFileInfo); | |||
} | |||
|
|||
internal Task<Document> OpenDocument (FilePath fileName, SolutionItem owner, int line, int column, OpenDocumentOptions options, Encoding encoding, IViewDisplayBinding binding) | |||
{ | |||
var openFileInfo = new FileOpenInformation (fileName, owner as Project) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mhutch , here owner can only ever be Project .Is it ok to do so ? Or do we need to create all the referred methods for SolutionItem as well ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It needs to be propagated...
If you follow the logic -
- RealOpenFile reads the Project from the FileOpenInformation
Project project = openFileInfo.Project ?? GetProjectContainingFile (fileName); - The project gets passed to LoadFileWrapper
var fw = new LoadFileWrapper (monitor, workbench, viewBinding, project, openFileInfo); - it's used to set the Project on the ViewContent:
newContent.Project = project;
I would suggest adding an Owner property to FileOpenInformation and having FileOpenInformation.Project forward to that, just like the other owner/project forwarding pairs.
Then RealOpenFile and LoadFileWrapper can use the FileOpenInformation.Owner instead of the project, and sent the Owner on the ViewContent.
Fortunately RealOpenFile and LoadFileWrapper are not public so do not have to keep API compatibility.
@@ -9,6 +9,7 @@ type TestDocument(name, parsedDocument, editor: TextEditor) = | |||
|
|||
override x.Name = name | |||
override x.Project = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setting Project is unnecessary here as it's forwarded from Owner
@@ -524,6 +533,18 @@ internal Task<Document> OpenDocument (FilePath fileName, Project project, int li | |||
return OpenDocument (openFileInfo); | |||
} | |||
|
|||
internal Task<Document> OpenDocument (FilePath fileName, SolutionItem owner, int line, int column, OpenDocumentOptions options, Encoding encoding, IViewDisplayBinding binding) | |||
{ | |||
var openFileInfo = new FileOpenInformation (fileName, owner as Project) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It needs to be propagated...
If you follow the logic -
- RealOpenFile reads the Project from the FileOpenInformation
Project project = openFileInfo.Project ?? GetProjectContainingFile (fileName); - The project gets passed to LoadFileWrapper
var fw = new LoadFileWrapper (monitor, workbench, viewBinding, project, openFileInfo); - it's used to set the Project on the ViewContent:
newContent.Project = project;
I would suggest adding an Owner property to FileOpenInformation and having FileOpenInformation.Project forward to that, just like the other owner/project forwarding pairs.
Then RealOpenFile and LoadFileWrapper can use the FileOpenInformation.Owner instead of the project, and sent the Owner on the ViewContent.
Fortunately RealOpenFile and LoadFileWrapper are not public so do not have to keep API compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple small changes but otherwise looking good
if (info.Project != null && doc.Project != info.Project) { | ||
doc.SetProject (info.Project); | ||
if (info.Owner != null && doc.Owner != info.Owner) { | ||
doc.SetProject (info.Owner as Project); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be SetOwner :)
app.Launch (fileName); | ||
} | ||
|
||
Counters.OpenDocumentTimer.Trace ("Adding to recent files"); | ||
DesktopService.RecentFiles.AddFile (fileName, project); | ||
DesktopService.RecentFiles.AddFile (fileName, owner as Project); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RecentFiles.Add could also have a SolutionItem owner overload.
|
||
if (openFileInfo.DisplayBinding != null) { | ||
binding = viewBinding = openFileInfo.DisplayBinding; | ||
} else { | ||
var bindings = DisplayBindingService.GetDisplayBindings (fileName, null, project).ToList (); | ||
var bindings = DisplayBindingService.GetDisplayBindings (fileName, null, owner as Project).ToList (); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add an overload to DisplayBindingService.GetDisplayBindings:
internal static IEnumerable<IDisplayBinding> GetDisplayBindings (FilePath filePath, string mimeType, SolutionItem owner)
{
//FIXME: this cannot be yet implemented without breaking IDisplayBinding.CanHandle API.
// it can be fixed when default interface methods are added to the C# language.
// for now, just forward to the Project version.
return GetDisplayBindings (filePath, mimeType, owner as Project);
}
That way there's a single place to fix it, when it's eventually possible to do so.
await fw.Invoke (fileName); | ||
} else { | ||
var extBinding = (IExternalDisplayBinding)binding; | ||
var app = extBinding.GetApplication (fileName, null, project); | ||
var app = extBinding.GetApplication (fileName, null, owner as Project); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as for IDisplayBinding.
build |
public void ShowGallery(IConnectedService[] services, Project project) | ||
public void ShowGallery (IConnectedService [] services, Project project) | ||
{ | ||
ShowGallery (services, project as SolutionItem); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a hard cast here, i.e. (SolutionItem)project
The "hard" cast is a little clearer in intent. Generally, you should use a soft ("as") cast when you want to get null if the cast fails. In this case the cast cannot fail, as Project is derived from SolutionItem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, no cast is required here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore my comment.
@@ -212,6 +212,12 @@ public EmptyDocumentContext (Project project) | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not need to override both Project and Owner, as they are chained. Overriding Owner is enough.
@@ -2394,7 +2394,7 @@ void CorrectIndenting () | |||
var formatter = CodeFormatterService.GetFormatter (Document.MimeType); | |||
if (formatter == null || !formatter.SupportsCorrectingIndent) | |||
return; | |||
var policies = Project != null ? Project.Policies : null; | |||
var policies = Owner != null ? Owner.Policies : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW this can be simplified using C# 6 syntax to
var policies = Owner?.Null;
|
||
public void AddFile (string fileName, SolutionItem owner) | ||
{ | ||
var projectName = owner != null ? owner.Name : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, this can be owner?.Name
@@ -40,7 +40,12 @@ public static GettingStartedProvider GetGettingStartedProvider (this Project pro | |||
|
|||
public static GettingStartedNode GetGettingStartedNode (this Project project) | |||
{ | |||
return project.GetService<GettingStartedProjectExtension> ()?.ProjectPadNode; | |||
return GetGettingStartedNode (project as SolutionItem); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard cast here too.
@@ -40,6 +40,7 @@ public abstract class BaseViewContent : IDisposable | |||
{ | |||
IWorkbenchWindow workbenchWindow; | |||
Project project; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this variable as it's no longer used.
} | ||
set { | ||
OnSetProject (value); | ||
OnSetOwner (project); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct, the incoming value is value
not project
. Also, chaining would be cleaner than duplicating, i.e. Owner = value
.
@@ -115,7 +123,7 @@ public static IEnumerable<FileViewer> GetFileViewers (FilePath filePath, Project | |||
yield return new FileViewer (vb); | |||
} else { | |||
var eb = (IExternalDisplayBinding) b; | |||
var app = eb.GetApplication (filePath, mimeType, ownerProject); | |||
var app = eb.GetApplication (filePath, mimeType, ownerProject as SolutionItem); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hard cast here too
@@ -219,7 +219,7 @@ public object GetDocumentObject () | |||
Solution adhocSolution; | |||
|
|||
public override Project Project { | |||
get { return (Window != null ? Window.ViewContent.Project : null) ?? adhocProject; } | |||
get { return (Window != null ? Window.ViewContent.Owner as Project : null) ?? adhocProject; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should chain to Owner instead of duplicating it
} | ||
|
||
void ListenToProjectLoad (Project project) | ||
{ | ||
ListenToProjectLoad (project as SolutionItem); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard cast here too
@slluis - do you think it would make sense to use |
@@ -60,7 +60,7 @@ public interface IViewDisplayBinding : IDisplayBinding | |||
///<summary>A display binding that opens an external application.</summary> | |||
public interface IExternalDisplayBinding : IDisplayBinding | |||
{ | |||
DesktopApplication GetApplication (FilePath fileName, string mimeType, Project ownerProject); | |||
DesktopApplication GetApplication (FilePath fileName, string mimeType, SolutionItem owner); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an API break
@mhutch I think |
@slluis I was just wondering about things like items in solution folders. It would be painful to have to change the type of "Owner" later. |
@mhutch there isn't much functionality we can use at this abstraction level, it could as well be an Object. In any case solution folder items is a real use case, so I'm ok with using WorkspaceObject. |
@gitexperience could you change There may be some things that will not work because they access members that are not present on WorkspaceObject, but those can conditionally cast to the derived types that do have this functionality.
could become
and
could become
|
@gitexperience also, please rebase onto master |
@@ -81,6 +81,7 @@ type FsiDocumentContext() = | |||
override x.ReparseDocument() = () | |||
override x.GetOptionSet() = TypeSystemService.Workspace.Options | |||
override x.Project = project :> Project |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is redundant, as you're setting Owner too
@@ -135,7 +136,13 @@ public override Microsoft.CodeAnalysis.Options.OptionSet GetOptionSet () | |||
|
|||
public override MonoDevelop.Projects.Project Project { | |||
get { | |||
return originalContext.Project; | |||
return originalContext.Owner as Project; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency/clarity of intent, this should chain to Owner rather than accessing originalContext directly.
Can you please rebase this onto master? The changes tab shows a lot of changes from master that appear to have been somehow cherry-picked. |
f2f76cd
to
0dbd452
Compare
@mhutch, rebasing done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just one little issue remaining.
@@ -155,7 +156,7 @@ void UpdateStyleParent (MonoDevelop.Projects.Project styleParent, string mimeTyp | |||
var mimeTypes = DesktopService.GetMimeTypeInheritanceChain (mimeType); | |||
|
|||
if (styleParent != null) | |||
policyContainer = styleParent.Policies; | |||
policyContainer = (styleParent as IPolicyProvider).Policies; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will throw a NRE if the styleParent does not implement IPolicyProvider.
I think this entire if/else could be simplified to
policyContainer = (styleParent as IPolicyProvider)
?.Policies
?? policyContainer = MonoDevelop.Projects.Policies.PolicyService.DefaultPolicies
71dd503
to
c3a2d64
Compare
@mhutch , Done! |
c3a2d64
to
ce82be8
Compare
It looks good to me but I'd like @slluis to take a look too |
@@ -257,7 +257,14 @@ public override void DiscardChanges () | |||
protected override void OnSetProject (MonoDevelop.Projects.Project project) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not necessary to override OnSetProject if OnSetOwner is overriden.
if (window.ViewContent.Project != null) { | ||
documentUrl = "file://" + window.ViewContent.Project.FileName; | ||
if (window.ViewContent.Owner != null) { | ||
documentUrl = "file://" + window.ViewContent.Owner.BaseDirectory.FileName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct, this will return the directory name, not the file name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@slluis, window.ViewContent.Project.FileName
is working fine here so I left it as it is. Can you please review?
InitializeExtensionChain (); | ||
ListenToProjectLoad (project); | ||
ListenToProjectLoad (owner); | ||
} | ||
|
||
void ListenToProjectLoad (Project project) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is not necessary.
345afe7
to
ce82be8
Compare
6f070b2
to
2e503dd
Compare
@slluis do we need more changes in this PR ? |
CMakeProject being derived from SolutionItem is unable to access the "Project" property. The PR is intended to test the changes needed to make the "Project" property available to the CBinding's CMakeProject.