Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Static Graph: better graphs when date range period parameter #2788

Closed
mattab opened this Issue · 5 comments

2 participants

Matthieu Aubry Julien Moumné
Matthieu Aubry
Owner
  • when period=range, should be rewritten to period=day in evolution graphs
    • Do we add a new parameter to decide which subperiod to plot, or default to "day" (configuration via config file?)
  • when period=month, should not be changed to period=range! but instead plot last N months

Cf: http://demo.piwik.org/index.php?module=API&method=API.getReportMetadata&period=range&date=2011-01-01,2012-01-01&format=xml&token_auth=x&idSites=1

<imageGraphUrl>
index.php?module=API&method=ImageGraph.get&idSite=1&apiModule=VisitsSummary&apiAction=get&token_auth=x&graphType=evolution&period=range&date=2011-01-01,2012-01-01
</imageGraphUrl>

at line: https://github.com/piwik/piwik/blob/master/plugins/ImageGraph/ImageGraph.php#L68

Julien Moumné
Collaborator

With the addition of a new graph type in ticket:2704#comment:11 and the refactoring of ImageGraph plugin, I would like to precisely define the following points:

Matthieu Aubry
Owner

What are the default graph types for each report and each period type (single period and multiple periods)

pretty much all that is currently, except the 2 bugs in this ticket: when period=range

However on trunk, the following debug URL will show all graphs as per imageGraphUrl:

http://localhost/trunk/index.php?module=ImageGraph&action=index&idSite=1&period=month&date=2011-10-10

You can see that it loads the Visits summary graph rewritten as: &period=month&date=2009-05-01,2011-10-31

Which seems to look good?

So is there really a bug with period=month?

What are the applicable graph types for each report and each period type. As I've understood, evolution is not applicable to reports with dimension. In the ImageGraph Plugin, this logic is buried way deep in the code at line https://github.com/piwik/piwik/blob/master/plugins/ImageGraph/API.php#L301. I would like to recode and move-up that logic to make it clearer and more maintainable.

The logic seems OK in this function, however this function is bloated and getting complicated. Re-factor sounds good

The default graph type is currently part of the ImageGraphUrl string within reports metadata. Wouldn't it be more suitable to remove it from there and add the 'default graph type' logic in https://github.com/piwik/piwik/blob/master/plugins/ImageGraph/API.php#L74 when the type is not specified as a parameter ? Currently, the logic is broken see : https://github.com/piwik/piwik/blob/master/plugins/ImageGraph/API.php#L105

If it's easier to have the logic in the get method then sure, +1 to remove the parameter from imageGraphUrl and put it to default to false in the API function parameter list

refactoring and simplicity are good ;)

Julien Moumné
Collaborator

Concerning

when period=range, should be rewritten to period=day in evolution graphs

    * Do we add a new parameter to decide which subperiod to plot, or default to "day" (configuration via config file?)

I've currently set period='day' which seems to me a reasonable default.

Shall I add a new entry in the configuration file ?

Matthieu Aubry
Owner

Shall I add a new entry in the configuration file ?
Yes please it sounds the best approach (graphs_default_period_to_plot_when_period_range ?)

Julien Moumné
Collaborator

(In [5582]) * fixes #2706, #2828, #2704, refs #1721, #2637, #2711, #2318, #71 : horizontal static graph implemented

  • fixes #2788, refs #2670, #1721, #2637, #2711 : default graph type logic moved to ImageGraph API, improved date/period logic, new parameter graphs_default_period_to_plot_when_period_range
  • fixes #2704, #2804, refs #1721 : pChart updated to 2.1.3, pChart code removed from Piwik code, OOP refactoring, support for unifont.ttf if present in ImageGraph/fonts, testAllSizes now uses report metadata ImageGraphUrl
  • refs #5491 : space between report title and report table reduced to avoid page overflow
  • refs #2829 : TODO display percentages
  • r5544, r5547, r5549 merged
Matthieu Aubry mattab added this to the 1.7 Piwik 1.7 milestone
Julien Moumné JulienMoumne was assigned by mattab
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.