fix: execute login before starting login shell #2405
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- change the error message when `nvim` is not found in `command.rs` - add a new command in `create_platform_shell_command` in `command.rs` - print command output in `launch` async function in `mod.rs` - uncomment and implement the `fmt::debug` trait for `neovimsession` struct in `session.rs`
- add `error_handling::resultpanicexplanation` to the imports in `command.rs` - add the `setup_env` function definition in `command.rs` - add setup environment variables for `term` and `colorterm` in `command.rs` - modify the `create_platform_shell_command` to use the `user` environment variable in `command.rs` - update `nvim_cmd_impl` function to use the `user` environment variable in `command.rs` - move the `command` module to be public in `mod.rs` - update imports in `main.rs` to include `setup_env` from `command` in `main.rs`
- update error message for neovim not found - improve error message clarity in build_nvim_cmd function
- remove unnecessary line in create_platform_shell_command function
- modify neovim command for version check in src/bridge/mod.rs
- remove unnecessary code checking neovim version in `src/bridge/mod.rs`
- add checking for nvim existence and return an error if not found - update comment to remove mention of macos as the command is used on other platforms as well
- remove commented out field in `neovimsession` debug implementation
- simplify `create_platform_shell_command` by removing duplicate code - refactor argument handling for `result.args` in `create_platform_shell_command`
- change default shell to `/bin/zsh` - add command execution with a login shell for macos - update command structure for nvim_cmd_impl function
- remove the `setup_env` function from `command.rs` - remove the call to `setup_env` in `main.rs`
- update the build_nvim_cmd function to use settings.get::<<cmdlinesettings>>().neovim_bin directly
- move `command` module declaration to be non-public
- remove unnecessary import of `path::pathbuf` in `command.rs` in the bridge directory
- update neovide installation instructions in `installation.md`
- update `neovim_ok` function to handle errors properly - improve error message generation in `create_error_message` function - refactor `neovim_ok` to use a unified error handling approach - add logic to handle unexpected output in `neovim_ok` function
- add comment explaining lossy conversion for non-utf8 output - edit comment regarding windows specific output issues
- fix typo in a comment about using the `login` command on macos - update comment about `login` command to clarify tty session appearance
- add a debug trait implementation for `neovimsession` module
Kethku
reviewed
Mar 8, 2024
- fix a typo in the neovide text to user - fix the typo by changing `users` to `user's`
- add a check if a terminal is available on macos before executing a command - setup a startup directory on macos before executing commands - set the `neovide_tty` variable in neovim based on terminal availability
- remove unused import for `is_tty` in `src/bridge/setup.rs` - reorganize imports in `src/bridge/setup.rs` to group related items together
- remove `use std::io::isterminal` from the `src/bridge/command.rs` file - add a conditional import of `use std::io::isterminal` for `macos` in `src/bridge/command.rs`
- remove setting of `neovide_tty` variable outside of macos target - add setting of `neovide_tty` variable inside macos target - refactor code to properly handle platform-specific settings
fredizzimo
requested changes
Mar 8, 2024
falcucci
added
the
macos
- add a new file `lua/cmd.lua` - initialize `args` as a table - set `vim.g.cmd` to `args.cmd` - create an autocmd for `vimenter` - import `cmd_lua` from `cmd.lua` in `mod.rs` - add functions `handle_command_arg` and `handle_arg_as_path_or_default` - comment on the setup conditions in `setup_tty_startup_directory` - use the `command` module in `setup_tty_startup_directory` - remove redundant code in `setup_tty_startup_directory` - execute `cmd.lua` in `neovim.exec_lua`
- add macos-specific condition for `handle_command_arg` function - add macos-specific condition for `handle_arg_as_path_or_default` function
- change the function name from `handle_arg_as_path_or_default` to `handle_command_arg_as_path_or_default`
- modify `build_login_cmd_args` function to properly handle and join command arguments - update `exec` variable to include joined command and args without additional join statement
- remove the `cmd_lua` constant from `mod.rs` - replace `nvim.exec_lua` with `nvim.command` in `mod.rs`
Kethku
reviewed
Mar 22, 2024
Kethku
reviewed
Mar 22, 2024
Kethku
reviewed
Mar 22, 2024
|
The last thing that stands out to me is a bit of a nitpick, but theres enough macos specific code here that I wonder if those functions should get pulled out into their own module that is conditionally compiled as a group. Up to you |
- fix the condition for `is_tty` in `platform_which` function - change the variable name `neovide_tty` to `neovide_ntty` in `handle_command_arg_as_path_or_default` function - update the condition in the comment for `setup_tty_startup_directory` function - in `setup_neovide_specific_state` function, change the variable name from `neovide_tty` to `neovide_ntty`
fredizzimo
reviewed
Mar 23, 2024
- modify the `neovim_ok` function to handle command output differently
- remove the `tty_startup_directory` constant from `mod.rs` - update the `setup_tty_startup_directory` function to use the `home` directory instead of `tty_startup_directory` - update the `get_startup_directory` function to use the `home` directory instead of `tty_startup_directory`
|
Thanks you! |
|
NOTE: the custom startup dir feature has been reverted due to possible macOS incompatibilities or bugs to determine the non-tty instance of itself. This feature is not also desirable as neovide should always be launched and recognized as non-tty as default. Some specific nvim commands regressions were also found during the usability. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This must solve most problems when neovide interact as a NON tty from the macOS application perspective persisting user sessions.
The following changes has been made:
execute a non interactive login shell properly;
After some research, using
/usr/bin/loginis the proper way for tty creation (hello alacrity, kitty), this is also important for script creation which depends from different conditions to be applied in case of tty command execution. I could avoid for example some mistakes at my scripts after identify this improvement.This also fixes feat: launch neovide as
--cwd $HOMEif not given directory path on macOS #2407 which is a desirable behavior for macOS users to apply when they launch Neovide from the Finder or the Dock without any neovim setup changes to work out of the box.It forks a login shell with the user environment correctly such as its PATH values and uses the default macOS shell
/bin/zshin case we miss the shell common identification instead of/bin/shto be spawned.update neovide installation instructions for macOS users;
mainly homebrew users are getting confused about it afterwards, regarding to their shell setup, this instruct them better and avoid possible open issues like Failed launching neovide on macOS from brew #1373;
update
neovim_okfunction to handle errors properly;in the previous implementation the function wasn't working as expect after some debugging scenarios, such as returning OK when stderr is empty and stdout is succeed but also empty.
The dynamic of the function was also simplified for a better maintainability.
What kind of change does this PR introduce?
Did this PR introduce a breaking change?
fixes #2407 #1373 #1604 #2112 #1859 #1679