Adds Android icon/battery plugin support by guixxx · Pull Request #480 · Powerlevel9k/powerlevel9k

guixxx · 2017-04-12T19:45:10Z

Plus fixes #479, which was causing integer overflow on 32-bit CPUs.

Plus fixes #479, which was causing integer overflow on 32-bit CPUs

dritter · 2017-04-12T19:47:05Z

😆 Awesome! That was fast. Thank you very much for your contribution!

dritter · 2017-04-12T19:48:02Z

powerlevel9k.zsh-theme

+
  # Reset start time
-  _P9K_TIMER_START=99999999999
+  _P9K_TIMER_START=2147483647


Could you add a comment, that this is the maximum integer on 32bit?

dritter · 2017-04-12T19:48:23Z

powerlevel9k.zsh-theme

+
  # Disable false display of command execution time
-  _P9K_TIMER_START=99999999999
+  _P9K_TIMER_START=2147483647


Could you add a comment, that this is the maximum integer on 32bit? <- Here as well

dritter · 2017-04-12T20:12:52Z

Awesome! One thing: Our development branch is next. Could you rebase your PR against that branch? That would be nice. 👍

guixxx · 2017-04-12T20:18:22Z

How can I do that? 😐 I'm completely new to the git universe, I created my account a few days ago. I could search on how to do that, but I don't want to risk messing things up. Thanks.

dritter · 2017-04-12T20:22:24Z

@guixxx Don't worry, I guess @bhilburn can change the base of this PR. So you won't have a hassle with it. 😉

bhilburn

Thanks so much for the PR, @guixxx! Nice work. Especially impressive how quickly you went from filing a bug to filing a PR if you are still learning git!

I'm excited to get this merged and think we can do it quickly. I'd like to just address a small item, first :)

bhilburn · 2017-04-12T21:08:41Z

functions/utilities.zsh


+# Not all OSes support the '-o' parameter
+# That's why this second condition is needed 
+case $(uname -o 2>/dev/null) in


Hm, I see why you did this, but at the same time, breaking this out of the main case statement will result in every OS evaluating this conditional, even if the OS has already been discovered.

What does uname return on Android? Linux? If so, perhaps we can add this as a second conditional within the Linux case in the main statement.

Thanks for your kind words! Yes, you're right and I've fixed it. Let me know what you think now!

dritter · 2017-04-12T21:18:21Z

I just installed Termux on my phone. I don't face this issues, as I have there 64bit as well 🙄

And @bhilburn : Yes, uname returns Linux on Android. Good catch.

onaforeignshore · 2017-04-13T18:52:37Z

I was wondering if it would be better to do something like ...

functions/utilities.zsh

# Architecture detection
case $(uname -m 2>/dev/null) in
    x86_64)
      ARCH='x64'
      ;;
    *)
      ARCH='x86'
      ;;
esac

Main file:

[[ "$ARCH" == "x64" ]] && _P9K_TIMER_START=99999999999 || _P9K_TIMER_START=2147483647

That way we can add support for 32-bit without changing 64-bit, and also use $ARCH globally if future segments need them. We can also add other architectures if required.

onaforeignshore · 2017-04-13T19:00:56Z

Don't forget that the OS_ICON is set in prompt_os_icon(), not in utilities...

prompt_os_icon() {
  local OS_ICON
  case "${OS}" in
    OSX)
      OS_ICON=$(print_icon 'APPLE_ICON')
      ;;
    BSD)
      OS_ICON=$(print_icon 'FREEBSD_ICON')
      ;;
    Linux)
      OS_ICON=$(print_icon 'LINUX_ICON')
      ;;
    Android)
      OS_ICON=$(print_icon 'ANDROID_ICON')
      ;;
    Solaris)
      OS_ICON=$(print_icon 'SUNOS_ICON')
      ;;
  esac

bhilburn · 2017-04-18T22:02:13Z

@onaforeignshore -

Don't forget that the OS_ICON is set in prompt_os_icon(), not in utilities...

Er, actually, it is set in utilities, now. See:
https://github.com/bhilburn/powerlevel9k/blob/master/powerlevel9k.zsh-theme#L886

Are you working off of an older branch, by chance?

That way we can add support for 32-bit without changing 64-bit, and also use $ARCH globally if future segments need them. We can also add other architectures if required.

Excellent suggestion, @onaforeignshore! I actually tried to implement this, and discovered that while I'm on a 64-bit machine, my ZSH shell is only compiled to support 4-byte integers. I guess the Fedora packager for ZSH just decided that 4-bytes is fine, and rolled with it.

Since $ARCH isn't a good indicator, the only other way would be to directly test bit precision in the shell. This seemed like a bit overboard for a number that will rarely overflow 0xFFFFFFFF, though, so I left it as-is for now.

bhilburn · 2017-04-18T22:02:56Z

This PR has been merged into next!

guixxx · 2017-05-07T01:51:22Z

Hey guys! Sorry for the very late response. First of, thanks for adding this PR. :)

I've just updated my powerlevel9k to the master branch, and I got this error:

powerlevel9k_prepare_prompts:3: number truncated after 7 digits: FFFFFFFF

Which refers to this line:

_P9K_TIMER_START=0xFFFFFFFF

That hex notation represents the maximum unsigned integer value in 32-bit systems. Since zsh scripts don't have the luxury of unsigned integers, we need to change that value to the maximum signed integer in 32-bit, which is 0x7FFFFFFF or 2147483647. I'd also suggest changing this line to the hex notation as well (it looks better :D).

Thanks guys.

Adds Android icon/battery plugin support

0a7a766

Plus fixes #479, which was causing integer overflow on 32-bit CPUs

dritter reviewed Apr 12, 2017

View reviewed changes

Adds useful comment

9b81602

onaforeignshore mentioned this pull request Apr 12, 2017

Merge engines #476

Closed

23 tasks

onaforeignshore mentioned this pull request Apr 12, 2017

A couple of issues when using powerlevel9k on Android #479

Closed

onaforeignshore added a commit to onaforeignshore/powerlevel9k that referenced this pull request Apr 12, 2017

merged Powerlevel9k#466 Powerlevel9k#472 Powerlevel9k#480

dc37e50

bhilburn reviewed Apr 12, 2017

View reviewed changes

bhilburn self-assigned this Apr 12, 2017

Improve logic

7028859

bhilburn closed this Apr 18, 2017

guixxx mentioned this pull request May 7, 2017

Icons for Windows/Cygwin + Refixes 32-bit support #521

Merged

bhilburn mentioned this pull request Oct 7, 2017

icons.zsh:168: character not in range #639

Closed

Conversation

guixxx commented Apr 12, 2017

Uh oh!

dritter commented Apr 12, 2017

Uh oh!

dritter Apr 12, 2017

Choose a reason for hiding this comment

Uh oh!

dritter Apr 12, 2017

Choose a reason for hiding this comment

Uh oh!

dritter commented Apr 12, 2017

Uh oh!

guixxx commented Apr 12, 2017

Uh oh!

dritter commented Apr 12, 2017

Uh oh!

bhilburn left a comment

Choose a reason for hiding this comment

Uh oh!

bhilburn Apr 12, 2017

Choose a reason for hiding this comment

Uh oh!

guixxx Apr 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dritter commented Apr 12, 2017

Uh oh!

onaforeignshore commented Apr 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

onaforeignshore commented Apr 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bhilburn commented Apr 18, 2017

Uh oh!

bhilburn commented Apr 18, 2017

Uh oh!

guixxx commented May 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

guixxx Apr 12, 2017 •

edited

Loading

onaforeignshore commented Apr 13, 2017 •

edited

Loading

onaforeignshore commented Apr 13, 2017 •

edited

Loading