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

fix(main/zsh): Fix zsh-newuser-install being run twice #20787

Merged
merged 1 commit into from
Jul 6, 2024

Conversation

kenjipuro
Copy link
Contributor

@kenjipuro kenjipuro commented Jul 6, 2024

Zsh already automatically runs zsh-newuser-install as part of the newuser module. Running it manually in the system zshrc caused it to run twice.

Note that zsh-newuser-install automatically exits if the terminal's size is less than 72x15, so it might not run on the default Termux terminal size.

Zsh already automatically runs zsh-newuser-install as part of the
`newuser` module. Running it manually in the system zshrc caused it to
run twice.
Copy link
Member

@TomJo2000 TomJo2000 left a comment

Choose a reason for hiding this comment

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

Good catch.

@TomJo2000
Copy link
Member

Note that zsh-newuser-install automatically exists if the terminal's size is less than 72x15, so it might not run on the default Termux terminal size.

Do you happen to have a source for that, because that might explain how I missed this initially.

@kenjipuro
Copy link
Contributor Author

Note that zsh-newuser-install automatically exists if the terminal's size is less than 72x15, so it might not run on the default Termux terminal size.

Do you happen to have a source for that, because that might explain how I missed this initially.

Yes, here is a link to the check on the unofficial GitHub Zsh mirror: https://github.com/zsh-users/zsh/blob/master/Functions/Newuser/zsh-newuser-install#L114

I found this while I was debugging why it was running in an SSH environment but not in a normal Termux session.

@TomJo2000
Copy link
Member

Okay so if $LINES is less than 15 or $COLUMNS is less than 72 it returns 1 (does not run).
That way around makes more sense to me.

@kenjipuro
Copy link
Contributor Author

It is probably the reason why @gphg had trouble bringing the wizard up in #20604 (comment).

@TomJo2000
Copy link
Member

Hmm maybe a notice would be good if that condition applies?

Something along the lines of:

# if zsh-newuser-install doesn't consider the terminal size "sane"
if (( ${LINES:-0} < 15 || ${COLUMNS:-0} < 72 )); then
# and no ~/.zshrc exists
[[ -r "${ZDOTDIR:-$HOME}"/.zshrc ]] || printf '%s\n' \
	'No ~/.zshrc exists' \
	'and zsh-newuser-install was not able to run.' \
	'Please consider zooming out by pinching' \
	'to increase the terminal size' \
	'and running zsh-newuser-install manually.' \
	'For first-run setup.' \
fi

@kenjipuro
Copy link
Contributor Author

I think handling that situation should be solved on upstream because it is not a Termux-specific issue.

As for the implementation itself, we do not need to check if zshrc exists because the function is either being ran automatically by zsh/newuser when no start-up files are found (see zshmodules(1)) or being ran manually by a user that wants to set them up.

Maybe something like this (considering how the other checks are handled):

# Don't run unless terminal is sane.
if (( ${LINES:-0} < 15 || ${COLUMNS:-0} < 72 )); then
  if [[ $1 = -f ]]; then
    print -r "$myname: terminal size too small (less than 72x15)." >&2
  fi
  return 1
fi

@TomJo2000
Copy link
Member

Well if a .zshrc exists, we don't run zsh-newuser-install,
so its existence does matter. No?

But yeah you're right, that's probably for upstream to work out.

@TomJo2000 TomJo2000 merged commit c145666 into termux:master Jul 6, 2024
5 checks passed
@TomJo2000
Copy link
Member

Oh congrats, you won the commit hash lottery.
four in a row, that's 1 in 65536.

@kenjipuro
Copy link
Contributor Author

Well if a .zshrc exists, we don't run zsh-newuser-install, so its existence does matter. No?

Yes, but if zsh-newuser-install is being run we are in that situation already (the zsh/newuser module runs it when no start-up files, including .zshrc exist). To clarify, I was suggesting that change in zsh-newuser-install, not in the system zshrc.

But it might not hurt to add a post-install script that notifies users of the terminal size issue.

@kenjipuro kenjipuro deleted the zsh-fix-extra-newuser branch July 6, 2024 22:55
@gphg
Copy link

gphg commented Jul 7, 2024

LGTM. I was work remotely at that time, I didn't notice it.

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.

3 participants