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

Make Chronometer usable on smaller screens #1518

Merged
merged 5 commits into from Jul 4, 2017

Conversation

Projects
None yet
6 participants
@WaLLy3K
Copy link
Collaborator

WaLLy3K commented Jun 4, 2017

By submitting this pull request, I confirm the following:

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.

How familiar are you with the codebase?:

10


  • FTL when offline will output "0"
  • Rewrite printFunc() to suit 17x40 screens
  • Silence calcFunc() errors
  • Print parsed throttle state from vcgencmd get_throttled
  • Expand or contract text depending on screen size
  • Provide "Press Ctrl-C to exit" msg when on auto-refresh mode

Preview

Make Chronometer usable on smaller screens
* FTL when offline will output "0"
* Rewrite printFunc() to suit 17x40 screens
* Silence calcFunc() errors
* Print parsed throttle state from `vcgencmd get_throttled`
* Expand or contract text depending on screen size
* Provide "Press Ctrl-C to exit" msg when on auto-refresh mode
@WaLLy3K

This comment has been minimized.

Copy link
Collaborator Author

WaLLy3K commented Jun 4, 2017

@cybrnook, you can test this by using pihole checkout core tweak/chrono-small (revert with pihole checkout core master) and we'll how the auto detection works out! :)

@cybrnook

This comment has been minimized.

Copy link
Contributor

cybrnook commented Jun 4, 2017

Looking pretty good :-) great work. Auto Detected like a charm, and has filled up the small screen perfectly, with usable information.

However it is still showing as PH: Offline on the top right but then shows active in the below list. My pihole is for sure online!

From WebIf:
Pi-hole Version v3.0.1 Web Interface Version v3.0.1 FTL Version v2.8
My pihole is a pi2 if that makes a difference.

fi

# Determine max length of divider
scr_line_len=$(( ${scr_size[1]} - 2 ))

This comment has been minimized.

@DL6ER

DL6ER Jun 4, 2017

Member

Is the ${} syntax necessary here or can it be $(( scr_size[1] - 2 ))?

This comment has been minimized.

@WaLLy3K

WaLLy3K Jun 4, 2017

Author Collaborator

https://github.com/koalaman/shellcheck/wiki/SC2004

The $ is unavoidable for special variables like $1 vs 1, $# vs #. It's also required when adding modifiers to parameters expansions, like ${#var} or ${var%-}. ShellCheck does not warn about these cases.

Since Codacy is complaining, I can just assign ${scr_size[1]} to its own variable, and perform arithmetic against that.

@DL6ER

This comment has been minimized.

Copy link
Member

DL6ER commented Jun 4, 2017

However it is still showing as PH: Offline on the top right but then shows active in the below list. My pihole is for sure online!

Can you run the following and tell us what it prints for you?

pihole -v -c 2> /dev/null | sed -n 's/^.* v/v/p'
@cybrnook

This comment has been minimized.

Copy link
Contributor

cybrnook commented Jun 4, 2017

Nothing directly, but return code is "0" if that helps:

pi@pihole:~ $ pihole -v -c 2> /dev/null | sed -n 's/^.* v/v/p'
pi@pihole:~ $
pi@pihole:~ $ echo $?
0
@DL6ER

This comment has been minimized.

Copy link
Member

DL6ER commented Jun 4, 2017

Okay, it should return something like

v3.0.1-89-gf1146a3
v3.0-49-gf2b4544
v2.8

Is there any output if you do

pihole -v -c

directly?

@cybrnook

This comment has been minimized.

Copy link
Contributor

cybrnook commented Jun 4, 2017

No, however independently:

pi@pihole:~ $ pihole -v
::: Pi-hole version is v3.0.1 (Latest version is v3.0.1)
::: Web-Admin version is v3.0.1 (Latest version is v3.0.1)

And -c just restarts chronometer

@DL6ER

This comment has been minimized.

Copy link
Member

DL6ER commented Jun 4, 2017

Strange ... when you ran

pihole checkout core tweak/chrono-small

was it running the installer for you?

Could you do a pihole -r + Repair?

@cybrnook

This comment has been minimized.

Copy link
Contributor

cybrnook commented Jun 4, 2017

I just downloaded the chronometer.sh script directly and uploaded it. I did not checkout and install the branch itself via git.

@DL6ER

This comment has been minimized.

Copy link
Member

DL6ER commented Jun 4, 2017

Well, this explains then why it did not work, as the new version of Chronometer depends also on a newer version of version.sh.

@@ -177,7 +224,7 @@ get_sys_stats() {
ph_lte_ver="${ph_ver_raw[1]}"
ph_ftl_ver="${ph_ver_raw[2]}"
else
ph_core_ver="${COL_LIGHT_RED}API unavailable${COL_NC}"
ph_core_ver="${COL_LIGHT_RED}Offline${COL_NC}"

This comment has been minimized.

@DL6ER

DL6ER Jun 4, 2017

Member

I don't understand the logic why you show a red "Offline" when the version cannot be obtained.

This comment has been minimized.

@WaLLy3K

WaLLy3K Jun 4, 2017

Author Collaborator

If the version can't be obtained, then I make the assumption that the pihole command is broken, or not installed. "Offline" is just wording that fits on a smaller screen, unlike "API unavailable" which is technically more accurate.

This comment has been minimized.

@dschaper

dschaper Jun 4, 2017

Member

Would API Down be a good compromise?

This comment has been minimized.

@WaLLy3K

WaLLy3K Jun 4, 2017

Author Collaborator

API Down fits perfectly! :D

# Expand or contract strings depending on screen size
if [[ "$chrono_type" == "large" ]]; then
phc_str=" ${COL_DARK_GRAY}Pi-hole"
lte_str=" ${COL_DARK_GRAY}AdminLTE"

This comment has been minimized.

@DL6ER

DL6ER Jun 4, 2017

Member

Instead of AdminLTE use something like dashboard or web? Are are planing to rename the web frontend repository in the not so distant future.

This comment has been minimized.

@WaLLy3K

WaLLy3K Jun 4, 2017

Author Collaborator

I was using current repository names to keep naming consistent, but I can definitely take this suggestion onboard!

This comment has been minimized.

@Mcat12

Mcat12 Jun 4, 2017

Member

Best bet would be Web or Web Interface, if there's the space.

This comment has been minimized.

@WaLLy3K

WaLLy3K Jun 4, 2017

Author Collaborator

Web can be used on smaller screens, but Web Interface is too long and looks out of place on larger screens. I'm thinking Web Admin perhaps?

This comment has been minimized.

@dschaper

dschaper Jun 4, 2017

Member

'Web' and 'Admin Web' may work...

total_str="Total: "
else
phc_str=" ${COL_DARK_GRAY}PH"
lte_str=" ${COL_DARK_GRAY}LTE"

This comment has been minimized.

@DL6ER

DL6ER Jun 4, 2017

Member

See comment to line 395.

@cybrnook

This comment has been minimized.

Copy link
Contributor

cybrnook commented Jun 4, 2017

Just want to add, thanks and yes, it's working fine now.

I stashed my manual changes, and played by the rules and switched to the dev branch "tweak/chrono-small".

It is working as expected.

@DL6ER and @WaLLy3K , I will back out now as the small screen issue has been addressed, and you can carry on your discussion around the logic.

@WaLLy3K WaLLy3K referenced this pull request Jun 5, 2017

Closed

Count non-static DHCP leases from available range #1500

5 of 5 tasks complete
Wording & Housekeeping
* Tidy up formatting and variable names
* Altered some comments
* Make temperature readout consistent between C/F/K
* Make Pi-hole "inactive" status match Admin Console "offline"
* Loopless DHCP lease counting logic
* Print "API Offline" / "API Down" depending on screen width, if `pihole -v` is not available
* Change "AdminLTE"/"LTE" to "Admin"/"Web"
* Removed unnecessary $cpu_freq_str line
@WaLLy3K

This comment has been minimized.

Copy link
Collaborator Author

WaLLy3K commented Jun 5, 2017

Reading the Travis CI log, I'm not sure what, if anything, I've done wrong.

I'd like to see DHCP re-tested, as I've changed the lease counting logic. I've also opted to replace "AdminLTE" with "Admin"/"Web" depending on screen size.

If this is acceptable (and there's nothing else that I've overlooked), then this should be good to go!

@dschaper

This comment has been minimized.

Copy link
Member

dschaper commented Jun 5, 2017

@DL6ER It's failing in the telnet tests for CentOS, /dev/localhost/ problem again.

@PromoFaux

This comment has been minimized.

Copy link
Member

PromoFaux commented Jun 5, 2017

Reran build after previous tests were reverted, and it now passes.

@WaLLy3K WaLLy3K changed the title [WIP] Make Chronometer usable on smaller screens Make Chronometer usable on smaller screens Jun 6, 2017

@dschaper dschaper added this to the v3.1 milestone Jun 17, 2017

@pi-hole pi-hole deleted a comment from codacy-bot Jun 20, 2017

@WaLLy3K WaLLy3K modified the milestones: v3.2, v3.1 Jun 20, 2017

@WaLLy3K

This comment has been minimized.

Copy link
Collaborator Author

WaLLy3K commented Jun 21, 2017

Hold off on merging this until #1541 is resolved.

Also, the DHCP output is a bit confusing if you're not aware of what it's doing. Happy to take on suggestions on how we can make the output concise and useful.

@WaLLy3K WaLLy3K referenced this pull request Jun 29, 2017

Closed

Simplify interaction between Chronometer and FTL #1541

5 of 5 tasks complete
@DL6ER

This comment has been minimized.

Copy link
Member

DL6ER commented Jun 29, 2017

#1541 is resolved

@WaLLy3K

This comment has been minimized.

Copy link
Collaborator Author

WaLLy3K commented Jun 30, 2017

I still can't think of a better solution to the DHCP readability issue, so feel free to review/merge this and I'll look into the issue later 😄

ads_blocked_today=$(printf "%.0f\n" "${ads_blocked_today_raw}")
ads_percentage_today=$(printf "%'.0f\n" "${ads_percentage_today_raw}")
queries_cached_percentage=$(printf "%.0f\n" "$(calcFunc "$queries_cached_raw * 100 / ( $queries_forwarded_raw + $queries_cached_raw )")")
recent_blocked=$(pihole-FTL recentBlocked)
top_ad_raw=($(pihole-FTL "top-ads (1)"))

This comment has been minimized.

@DL6ER

DL6ER Jul 2, 2017

Member

I wonder if you should quote here to avoid splitting (see also the next two lines).
array=( "$(mycommand)" ) as the output should be a single element (and there is no fail check here).

@DL6ER

This comment has been minimized.

Copy link
Member

DL6ER commented Jul 2, 2017

60x20 Okay:
60-20

40x17: Not ideal (but that wouldn't be seen on master):
40-17

Fix dev version output on small screens
* Also use read/mapfile for arrays
@DL6ER

DL6ER approved these changes Jul 2, 2017

Copy link
Member

DL6ER left a comment

screenshot at 2017-07-02 16-24-37

@WaLLy3K WaLLy3K merged commit 1bebcef into development Jul 4, 2017

5 checks passed

codacy/pr Good work! A positive pull request.
Details
code-review/pullapprove Approved by DL6ER, WaLLy3K
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@WaLLy3K WaLLy3K deleted the tweak/chrono-small branch Jul 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.