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

Change over-time graphs from line to stacked bar representation #1098

Merged
merged 3 commits into from Dec 28, 2019

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Dec 18, 2019

By submitting this pull request, I confirm the following:

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes, and have included unit tests where possible.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)

What does this PR aim to accomplish?:

Change over-time graphs from line to stacked bar representation. This is much more natural for this kind of data (it is discrete, not continuous!).

   Before
Screenshot at 2019-12-18 14-13-53

   Now
Screenshot at 2019-12-18 14-13-40


Also, improve DB graphs to always generate a meaningful display (always generate about 200 bars). This graph was basically unusable when specifying a larger range than, say, one week.

   Before (this week)
Screenshot at 2019-12-18 14-15-45

   Now (this week)
Screenshot at 2019-12-18 14-16-04

   Before (this month)
Screenshot at 2019-12-18 14-17-03

   Now (this month)
Screenshot at 2019-12-18 14-16-42

The slight difference seen in the strongest peeks is due to the monotone interpolation of the lines graphs (and wrong!).


How does this PR accomplish the above?:

Change over-time graphs from line to stacked bar representation. Compute interval based on specified time window.

What documentation changes (if any) are needed to support this PR?:

@DL6ER DL6ER added this to the v5.0 milestone Dec 18, 2019
@DL6ER DL6ER requested a review from a team December 18, 2019 13:18
@@ -382,13 +382,6 @@ function parseDBData($results, $interval, $from, $until) {
// $data[timestamp] = value_in_this_interval
$data[$row[0]] = intval($row[1]);
}

// Fill the missing intervals with zero
Copy link
Member Author

Choose a reason for hiding this comment

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

With bar graphs, we do not need to fill gaps (there will simply be gaps for empty days). This change enhances processing speed notably.

$.getJSON("api_db.php?getGraphData&from="+from+"&until="+until, function(data) {

// Compute interval to obtain about 200 values
var num = 200;
Copy link
Member Author

Choose a reason for hiding this comment

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

This variable controls how many lines will be computed. I don't think we need to make it user-defined, however, I put it in a common place so users can still modify it when they deem it appropriate. We could also make it a global variable, users could then modify this value (live) through the developer tools.

… is much more natural for this kind of data. Also, improve DB graphs to always generate a meaningful display (always generate about 200 bars). This graph was basically unusable when specifying a larger range than, say, one week.

Signed-off-by: DL6ER <dl6er@dl6er.de>
@XhmikosR
Copy link
Contributor

I definitely like this change, but I really think we need #1080 and #1079 sorted before any further JS changes

@DL6ER
Copy link
Member Author

DL6ER commented Dec 18, 2019

The changes in here are of purely functional. We can get them in and cherry-pick them into the PRs that format the code. This seems a logical order because this new code would be formatted in them as well. I don't know if we merge #1079 and #1080 is still a [WIP].

@XhmikosR
Copy link
Contributor

They might be functional but they don't follow the best practices, hence why I suggested the above...

scripts\pi-hole\js\db_graph.js:199:1
  ×  199:1   Avoid using extended native objects                                       no-use-extend-native/no-use-extend-native
  ×  199:1   Date prototype is read only, properties should not be added.              no-extend-native

  scripts\pi-hole\js\index.js:10:20
  ×   10:20  forwardDestinationChart is defined but never used.                        no-unused-vars

(the lines don't match this patch due to the style changes, but it is the date prototype addition and one unused var)

Anyway, as long as the above are sorted, this is fine by me. Generally extending the native objects is bad practice. Many things work but it doesn't mean they are OK to do :)

Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
@DL6ER
Copy link
Member Author

DL6ER commented Dec 19, 2019

@XhmikosR I have never learned any language by attending a course so I may not know what best practices are or may have learned them from the wrong people (you never know). Which checker did you use to get the reply you're seeing? I'd like to see the same being mentioned by the CI to teach me how to get better. But this might be what you are looking into with #1080

I addressed the mentioned points meanwhile.

@XhmikosR
Copy link
Contributor

@DL6ER yeah that's with #1080 which only misses the CI part and someone to check out my comments

@DL6ER
Copy link
Member Author

DL6ER commented Dec 20, 2019

Once this is merged, we should update the screenshots shown at https://github.com/pi-hole/AdminLTE/blob/master/README.md

(see also here: https://github.com/pi-hole/graphics/tree/master/Screenshots)

@DL6ER DL6ER merged commit 306b0b9 into devel Dec 28, 2019
@DL6ER DL6ER deleted the tweak/graphs branch December 28, 2019 15:46
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/query-activity-graphs-changed-to-individual-bars/31297/2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants