Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delegate opening files for Rider to the RiderPathLocator NuGet package #79958

Merged

Conversation

van800
Copy link
Contributor

@van800 van800 commented Jul 27, 2023

OpenFile (for Rider and Fleet) functionality is now managed by the JetBrains nuget.
When both Rider and Fleet are installed, Rider would be more preferable, which would fix #78832 (comment)

@van800 van800 force-pushed the master-rider-path-locator-fleet branch from aff533c to 1d4616b Compare July 27, 2023 17:51
@van800 van800 requested a review from RedworkDE July 27, 2023 17:52
@AThousandShips AThousandShips added this to the 4.x milestone Jul 28, 2023
Copy link
Member

@RedworkDE RedworkDE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with two caveats:

  • I don't actually have Rider so I didn't test this, but the +1 for the line got removed. Does this open the correct line (watch out for C#: Fix line in OpenInExternalEditor #79404 when testing this)
  • The name of this PR/commit is kinda non-descriptive and I think something along the lines of Delegate opening files in Rider to the Rider locator NuGet package would be better

@van800 van800 changed the title Support Rider and Fleet open file args Delegate opening files for Rider to the RiderPathLocator NuGet package Jul 28, 2023
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only some minor nitpicks, LGTM otherwise.

Also, don't forget to update the commit message, it's more important than the PR title since it's what shows up in the git history.

@van800 van800 force-pushed the master-rider-path-locator-fleet branch from 1d4616b to 369597e Compare July 28, 2023 10:23
@raulsntos
Copy link
Member

[...] the +1 for the line got removed. Does this open the correct line.

I assume they moved that to the NuGet package, but would be nice to confirm.

By the way, @van800 the NuGet package points to https://github.com/JetBrains/resharper-unity/ but it doesn't seem like that repository has the source code for the package (or maybe I'm looking at the wrong branch).

@van800
Copy link
Contributor Author

van800 commented Jul 29, 2023

[...] the +1 for the line got removed. Does this open the correct line.

I assume they moved that to the NuGet package, but would be nice to confirm.

By the way, @van800 the NuGet package points to https://github.com/JetBrains/resharper-unity/ but it doesn't seem like that repository has the source code for the package (or maybe I'm looking at the wrong branch).

It will get synchronized soon from the internal repo. I will post another message here, once that is done.

@van800
Copy link
Contributor Author

van800 commented Jul 31, 2023

That is in sync now https://github.com/JetBrains/resharper-unity/blob/net233/unity/PathLocator/PathLocator.csproj

@raulsntos
Copy link
Member

Does this still work after #79404 has been merged? @RedworkDE was correct that now the code no longer adds 1 to the line number and RiderFileOpener doesn't seem to do it either:

https://github.com/JetBrains/resharper-unity/blob/21e2393e2a3825cb4fd7035e9ef6307bc3068502/unity/PathLocator/RiderFileOpener.cs#L25

So make sure it opens the file at the correct line number.


Also, can you rename the Rider option to include Fleet? Since it seems to support both. Something like this:

diff --git a/modules/mono/editor/GodotTools/GodotTools/GodotSharpEditor.cs b/modules/mono/editor/GodotTools/GodotTools/GodotSharpEditor.cs
index 622a155d37..8bda2706a5 100644
--- a/modules/mono/editor/GodotTools/GodotTools/GodotSharpEditor.cs
+++ b/modules/mono/editor/GodotTools/GodotTools/GodotSharpEditor.cs
@@ -538,7 +538,7 @@ namespace GodotTools
                 settingsHintStr += $",Visual Studio:{(int)ExternalEditorId.VisualStudio}" +
                                    $",MonoDevelop:{(int)ExternalEditorId.MonoDevelop}" +
                                    $",Visual Studio Code:{(int)ExternalEditorId.VsCode}" +
-                                   $",JetBrains Rider:{(int)ExternalEditorId.Rider}" +
+                                   $",JetBrains Rider and Fleet:{(int)ExternalEditorId.Rider}" +
                                    $",Custom:{(int)ExternalEditorId.CustomEditor}";
             }
             else if (OS.IsMacOS)
@@ -546,14 +546,14 @@ namespace GodotTools
                 settingsHintStr += $",Visual Studio:{(int)ExternalEditorId.VisualStudioForMac}" +
                                    $",MonoDevelop:{(int)ExternalEditorId.MonoDevelop}" +
                                    $",Visual Studio Code:{(int)ExternalEditorId.VsCode}" +
-                                   $",JetBrains Rider:{(int)ExternalEditorId.Rider}" +
+                                   $",JetBrains Rider and Fleet:{(int)ExternalEditorId.Rider}" +
                                    $",Custom:{(int)ExternalEditorId.CustomEditor}";
             }
             else if (OS.IsUnixLike)
             {
                 settingsHintStr += $",MonoDevelop:{(int)ExternalEditorId.MonoDevelop}" +
                                    $",Visual Studio Code:{(int)ExternalEditorId.VsCode}" +
-                                   $",JetBrains Rider:{(int)ExternalEditorId.Rider}" +
+                                   $",JetBrains Rider and Fleet:{(int)ExternalEditorId.Rider}" +
                                    $",Custom:{(int)ExternalEditorId.CustomEditor}";
             }
 

@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Aug 2, 2023
@van800 van800 force-pushed the master-rider-path-locator-fleet branch from 369597e to 8654fe8 Compare August 3, 2023 09:21
@van800 van800 force-pushed the master-rider-path-locator-fleet branch from 8654fe8 to 7f8e3ab Compare August 4, 2023 08:31
@van800
Copy link
Contributor Author

van800 commented Aug 4, 2023

Thank you, fixed both.

Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I haven't tested since I don't have Rider.

@akien-mga akien-mga merged commit c236503 into godotengine:master Aug 4, 2023
14 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants