@saleemkce opened this Pull Request on April 11th 2015 Contributor

This bug fix is for issue https://github.com/piwik/piwik/issues/7566 So, with this fix, now it's possible to give idSite=1 (single site image graph generation), idSite=1,2,3,4 (comma separated idSites) or idSite=all(list all sites the user has view permission).

During this fix, the only tradeoff is, I needed to create a new function strokeAsImageData() to send the generated image's content back to 'plugins/ImageGraph/StaticGraph.php' because current implementation causes the browser to output single PNG image. So, we refactor the code to just pass the image data back to the caller so that it could be utilized in HTML page with N no. of images for each image graph generation.

It's still not better approach because this new function is added in pChar library file. We need to inherit the code so that when pChart library is updated, we don't lose our new strokeAsImageData() function. Please go through the code and give suggests, we can improve further.

For time being, use my fix and play with this new multi image graph page.

@saleemkce commented on April 11th 2015 Contributor

Current newly implemented page looks like this:


@saleemkce commented on April 21st 2015 Contributor

hi all, what is the status for this commit? If you have feedback, please comment. I will do everything to get it done. Do you find issues with it or could it be moved to master?

@saleemkce commented on April 21st 2015 Contributor

Here is a bar graph representation for multiple websites.


Oh, how nice it looks in verticalBar, horizontalBar and as Pie. I cannot wait to see how long it will take before this commit is approved and moved to Piwik codebase. But everyone is eager to see it done sooner. And me too.

@mattab commented on April 21st 2015 Owner

Hi @saleemkce

Thanks for the pull request! The diff looks quite big so it may take a while before we get a chance to review and consider it. Also we'd need to add some tests for this (see also: Enable static PNG image graphs during Continuous Integration #6864 )

@saleemkce commented on April 22nd 2015 Contributor

@mattab Okay. Very glad t to know. Moreover, if you take a look at this http://forum.piwik.org/read.php?9,126057, it would be helpful.

@tsteur commented on May 6th 2015 Owner

The diff is quite big indeed. I wonder if it was possible to make the code simpler by leaving the behaviour of ImageGraph.get more or less as it is. Then introduce a new method like ImageGraph.getForSites which might use ImageGraph.get($idSite) for each given site and puts the graph together. Or maybe we could extract some code from ImageGraph.get into methods and reuse them in getForSites (feel free to change the name if you do it).

@mattab commented on May 6th 2015 Owner

The diff is quite big

Btw I just noticed if we look at the diff without whitespace (&w=1) it looks smaller: https://github.com/piwik/piwik/pull/7660/files?w=1

@saleemkce commented on May 7th 2015 Contributor

@mattab @tsteur Thanks a lot, guys. All your suggestions successfully implemented. I have tested all new changes with 2 sites in my local machine. Everything is okay and looks nice as before. Latest changes here: https://github.com/piwik/piwik/pull/7660/files?w=1 Please notify if anything is wrong.

@mattab commented on September 11th 2015 Owner

Dear @saleemkce

Thanks for the Pull request. At this stage, we will not merge it as we think we don't need this feature in Piwik. For example, it's quite simple for someone to include graphs from multiple websites by creating a little PHP script that loops through sites and then generates the image. Therefore we prefer not to add complexity in this part of the code. Thanks anyway for your PR and we hope you will keep contributing in the future :-)

This Pull Request was closed on September 11th 2015
Powered by GitHub Issue Mirror