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

Update PowerShell plugin #947

Merged
merged 15 commits into from
Mar 17, 2025
Merged

Conversation

alexis-opolka
Copy link
Contributor

@alexis-opolka alexis-opolka commented Feb 28, 2025

This PR intends to fix #946 by removing any relative path used in the install and to do so the PowerShell module has been moved to its own repository (https://github.com/alexis-opolka/navi.plugin.pwsh) is now printed as-is and can then be "evaled" in a profile or in the session.

@alexis-opolka alexis-opolka marked this pull request as ready for review February 28, 2025 00:10
@alexis-opolka
Copy link
Contributor Author

alexis-opolka commented Feb 28, 2025

@kit494way, I won't be able to test if the fix is effective or not until next monday, if you need to do not hesitate to grab my version of navi on the pwsh-plugin branch in my fork, build the binary and test it.

@alexis-opolka alexis-opolka changed the title Updates PowerShell plugin Update PowerShell plugin Feb 28, 2025
@kit494way
Copy link
Contributor

@alexis-opolka I have not tried to run the command because I do not have an environment that can build a binary for windows.

navi widget for other shells outputs a script that defines functions and key bindings.
But navi widget power-shell outputs a script that installs a module and defines key bindings.
It is better to output the function definition embedded inside navi than to install a module from internet, because if the version of navi is same, the behavior of the scripts printed by navi widget will also be the same.

If separating the widget into a third-party module, I think it is enough to describe the installation instruction in a document like the widget for xonsh.

navi/docs/installation.md

Lines 130 to 132 in 90a29ed

# xonsh
# xpip install xontrib-navi # ← run in your xonsh session to install xontrib
xontrib load navi # ← add to your xonsh run control file

If installing the 3rd party module that is not managed by this project, it is better to notify users about it.

@alexis-opolka
Copy link
Contributor Author

Then it will wait before I can use a windows system to compile and see.

Since widgets are integrating with 3rd-party, it might not have to be necessarily tied to a given Navi's version.
It can indeed be a way to handle it but should you release a new version of navi only because of a fix / update you made in a code that integrates navi and a 3rd-party? I'm talking about a case where we wouldn't have to modify navi's code a bit.

I don't know if it's actually a good idea to have the PowerShell module separated from navi, I did this PR as a quick fix but now that you raised some good points, I need to think about it. 🤔

@alexis-opolka alexis-opolka marked this pull request as draft March 4, 2025 13:16
@alexis-opolka
Copy link
Contributor Author

alexis-opolka commented Mar 5, 2025

After some more thoughts @kit494way , it would indeed be good for navi to have the command print out the PowerShell module but I think it would somewhat go against PowerShell's philosophy with integrating 3rd-party modules.

I think that we can do it in two different ways.

The first one would be to continue to treat the powershell plugin as an internal dependency of the project and modify the installation process by printing out the module's code, which would then be called inside the user's profile.

The second one would be to treat the powershell plugin as an external dependency of the project and remove its code from navi, the installation process would be similar to the xonsh module.

The pros of the first would be the linked versioning of both navi and its powershell plugin.

The cons would be to have an update of navi even when navi's code might be unchanged.

The pros of the second would be the logical separation of the powershell plugin with navi which enables us to have more flexibility on their versions i.e. we're not obligated to create an update of navi to fix the powershell plugin.

The cons would be the break of the linked versioning, two users with Navi 2.24.0 can have a different behaviour with the powershell plugin.

What do you think?

@kit494way
Copy link
Contributor

@alexis-opolka
Considering the case of responding to issues from users, I think it would be easier to identify the problem if the versions are linked.
Once the shell widget is stable, I think there will be more questions from users than updating it, and we will instruct users how to customize the shell widget.

I expect shell widgets to serve two roles.

  • Make it easy for new users to set up shell integration and have a better user experience.
  • Provides a good starting point for users to create a shell widget that fits their needs.

On the second point, I prefer to output the function definitions, because I can easily see how the widget is implemented.

Although I am not familiar with powershell's philosophy, I know of two precedents that output function or module definitions, starship and mise.
The first case is particularly instructive.
Since New-Module is provided, I think the way to dynamically generate modules like starship is not against the powershell’s philosophy.

If the module is to be separated into external module, it would be better to output the version of the module, and it would be nice to be able to easily see where the source of the module is found.

@alexis-opolka
Copy link
Contributor Author

I agree with you @kit494way , I'll make the necessary modifications during the weekend if I'm able to, otherwise it will need to wait until next week.

@alexis-opolka
Copy link
Contributor Author

I'm going to update the command to output the module definition and I do think that your suggestions are a better way for the project, thanks! 😄

It should remove the need to put a "-" in the middle of the name. It's a bit impractical.
Updated the link to the module.
Moves the file to the base folder, removed cmdlets handlers and related options. Still needs modifications.
Signed-off-by: alexis-opolka <[email protected]>
@alexis-opolka alexis-opolka marked this pull request as ready for review March 14, 2025 23:03
@alexis-opolka
Copy link
Contributor Author

It should be good on my side @kit494way , this version should print out the module's definition and not issue any problems while trying to invoke the powershell plugin.

@kit494way
Copy link
Contributor

@alexis-opolka
It almost works perfectly.
But fzf's --height option does not work.
This problem occurs when invoked from a keybinding.
It appears that fzf's --height option does not work well with PSConsoleReadline of PowerShell.
This is a known issue reported here.
This problem seems to be solved by making the following changes based on the comment in the issue.

diff --git a/shell/navi.plugin.ps1 b/shell/navi.plugin.ps1
index c931357..a99592c 100644
--- a/shell/navi.plugin.ps1
+++ b/shell/navi.plugin.ps1
@@ -1,6 +1,21 @@
 
 
 $null = New-Module {
+    function Invoke-Navi {
+        $p = [System.Diagnostics.Process]@{StartInfo = @{
+          FileName = "navi";
+          Arguments = $args;
+          RedirectStandardOutput = $true;
+          WorkingDirectory = $PWD;
+        }}
+
+        [void]$p.Start()
+        $result = $p.StandardOutput.ReadToEnd()
+        $p.WaitForExit()
+
+        $result
+    }
+
     ### Initial code from @lurebat (https://github.com/lurebat/)
     ### See #570 (https://github.com/denisidoro/navi/issues/570) for its original contribution
     function Invoke-NaviWidget {
@@ -11,12 +26,13 @@ $null = New-Module {
         $output = $null
 
         if ([String]::IsNullOrEmpty($line)) {
-            $output = navi --print
+            $output = (Invoke-Navi "--print" | Out-String).Trim()
         }
         else {
-            $best_match = (navi --print --best-match --query $line | Out-String).Trim()
+            $best_match = (Invoke-Navi "--print" "--best-match" "--query `"$line`"" | Out-String).Trim()
+
             if ([String]::IsNullOrEmpty($best_match)) {
-                $output = (navi --print --query "$line" | Out-String).Trim()
+                $output = (Invoke-Navi "--print" "--query `"$line`"" | Out-String).Trim()
             }
             else {
                 $output = $best_match
@@ -24,6 +40,7 @@ $null = New-Module {
         }
 
         [Microsoft.PowerShell.PSConsoleReadLine]::RevertLine()
+        [Microsoft.PowerShell.PSConsoleReadLine]::InvokePrompt()
 
         ### Handling the case when the user escapes without selecting any entry
         if (-Not([String]::IsNullOrEmpty($output))) {

alexis-opolka and others added 3 commits March 16, 2025 07:44
Co-authored-by: KITAGAWA Yasutaka <[email protected]>
Signed-off-by: alexis-opolka <[email protected]>
Signed-off-by: alexis-opolka <[email protected]>
Signed-off-by: alexis-opolka <[email protected]>
@alexis-opolka
Copy link
Contributor Author

alexis-opolka commented Mar 16, 2025

@kit494way It should work correctly now.
@denisidoro If it's ok with you and @kit494way , you can squash and merge this PR.

Just a quick update though, the subcommand for the powershell plugin is not widget power-shell anymore but widget powershell which is easier to type than the first one and shouldn't impact the user base that much.

EDIT: In the case of a squash, please make sure that kit494way is mentioned as co-author of the commit.

@denisidoro denisidoro merged commit 6f1bbcf into denisidoro:master Mar 17, 2025
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

powershell widget does not work
3 participants