Opened 23 months ago

Closed 21 months ago

Last modified 21 months ago

#3192 closed Bug (fixed)

mobile vs. desktop pie chart does not show all values

Reported by: matt Owned by:
Priority: normal Milestone: 1.8.3 - Piwik 1.8.3
Component: Core Keywords:
Cc: Sensitive: no

Description

Reported in forum

Also it seems the pie graphs use filter_limit instead of filter_truncate?

Attachments (1)

3192.diff.tar.gz (1.4 KB) - added by capedfuzz 21 months ago.
Patch for always showing mobile/desktop rows.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 21 months ago by capedfuzz (diosmosis)

  • Resolution set to fixed
  • Status changed from new to closed

(In [6516]) Fixes #3192, fixed bug in UserSettings where GroupBy filter was queued instead of applied directly & fixed regression where generic filters were run when displaying pie graphs.

NOTES:

  • Refactored ViewDataTale.php a bit to clear up code.

comment:2 follow-up: Changed 21 months ago by matt (mattab)

The disableGenericFilters() seems dangerous here - because the graph must sometimes apply generic filter data (eg. I haven't tested but filtering with filter_pattern or using filter_truncate should still work) ?

Were the generic filters causing a problem in the case of pie graphs ?

comment:3 Changed 21 months ago by matt (mattab)

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:4 in reply to: ↑ 2 ; follow-up: Changed 21 months ago by capedfuzz (diosmosis)

Replying to matt:

The disableGenericFilters() seems dangerous here - because the graph must sometimes apply generic filter data (eg. I haven't tested but filtering with filter_pattern or using filter_truncate should still work) ?

Were the generic filters causing a problem in the case of pie graphs ?

The limit filter was reducing the number of values before. Also, this bug was introduced (I think) w/ #3004 (or maybe the 'report was generated on' issue) when I put the postDataTableLoaded call in there, so the generic filters were never called before. (Which might be another bug, I guess :)).

We could enable generic filters and move the call to after the AddSummaryRow filter is used.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 21 months ago by matt (mattab)

Replying to capedfuzz:

Replying to matt:

The disableGenericFilters() seems dangerous here - because the graph must sometimes apply generic filter data (eg. I haven't tested but filtering with filter_pattern or using filter_truncate should still work) ?

Were the generic filters causing a problem in the case of pie graphs ?

The limit filter was reducing the number of values before. Also, this bug was introduced (I think) w/ #3004 (or maybe the 'report was generated on' issue) when I put the postDataTableLoaded call in there, so the generic filters were never called before. (Which might be another bug, I guess :)).

Umh OK but can you take this call out then? ;-)


We could enable generic filters and move the call to after the AddSummaryRow filter is used.

Sounds good, as long as they run before the queued filters

comment:6 in reply to: ↑ 5 Changed 21 months ago by capedfuzz (diosmosis)

Replying to matt:

Replying to capedfuzz:

Replying to matt:

The disableGenericFilters() seems dangerous here - because the graph must sometimes apply generic filter data (eg. I haven't tested but filtering with filter_pattern or using filter_truncate should still work) ?

Were the generic filters causing a problem in the case of pie graphs ?

The limit filter was reducing the number of values before. Also, this bug was introduced (I think) w/ #3004 (or maybe the 'report was generated on' issue) when I put the postDataTableLoaded call in there, so the generic filters were never called before. (Which might be another bug, I guess :)).

Umh OK but can you take this call out then? ;-)

I think the call was needed for #3004, so priority filters will be run on a view.


We could enable generic filters and move the call to after the AddSummaryRow filter is used.

Sounds good, as long as they run before the queued filters

Just tried this and it won't work. datatable.js will send the filter_limit parameter when switching to the pie graph view. I'll investigate alternative solutions.

comment:7 Changed 21 months ago by capedfuzz (diosmosis)

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [6558]) Fixes #3192, allow limit filter to be skipped by ViewDataTable descendents & skip in GenerateGraphData.

comment:8 follow-up: Changed 21 months ago by matt (mattab)

  • Resolution fixed deleted
  • Status changed from closed to reopened

Maybe this problem could be fixed in the Javascript rather than changing the Api of postDataTableLoadedFromAPI() - changing Core API is always risky and more hacky.

For example maybe the commit [6558] could be reverted the following

Index: plugins/CoreHome/templates/datatable.js
===================================================================
--- plugins/CoreHome/templates/datatable.js	(revision 6564)
+++ plugins/CoreHome/templates/datatable.js	(working copy)
@@ -542,6 +542,10 @@
 				var filters = self.resetAllFilters();
 				self.param.flat = filters.flat;
 				self.param.columns = filters.columns;
+
+				// when switching to display graphs, reset limit
+				delete self.param.filter_offset;
+				delete self.param.filter_limit;
 				
 				self.param.viewDataTable = viewDataTable;
 				self.reloadAjaxDataTable();

comment:9 in reply to: ↑ 8 Changed 21 months ago by capedfuzz (diosmosis)

Replying to matt:

Maybe this problem could be fixed in the Javascript rather than changing the Api of postDataTableLoadedFromAPI() - changing Core API is always risky and more hacky.

For example maybe the commit [6558] could be reverted the following

Index: plugins/CoreHome/templates/datatable.js
===================================================================
--- plugins/CoreHome/templates/datatable.js	(revision 6564)
+++ plugins/CoreHome/templates/datatable.js	(working copy)
@@ -542,6 +542,10 @@
 				var filters = self.resetAllFilters();
 				self.param.flat = filters.flat;
 				self.param.columns = filters.columns;
+
+				// when switching to display graphs, reset limit
+				delete self.param.filter_offset;
+				delete self.param.filter_limit;
 				
 				self.param.viewDataTable = viewDataTable;
 				self.reloadAjaxDataTable();

I thought of that, but it won't work. If you remove the params when loading a graph, the table won't have the limit/offset when you switch back. Which is also why I didn't add a disableLimit method (like the disableSort method).

comment:10 follow-up: Changed 21 months ago by matt (mattab)

I think it's OK if the tables don't have the limit information after looking at the graph... Limit can be lost when switching contest, we want to keep: Searched string, Flattened, etc. but not limit.

comment:11 in reply to: ↑ 10 Changed 21 months ago by capedfuzz (diosmosis)

I guess if you're ok w/ that info being lost, the change you proposed will work, but I don't see the problem w/ disabling the limit filter for graphs. Graphs will always provide an overview of all the information not a subsection of it, so limit/offset will never have any meaning for them. Deleting the parameters in the JS to avoid a bug in the php seems more hacky than allowing descendants of ViewDataTable to customize behavior.

comment:12 Changed 21 months ago by capedfuzz (diosmosis)

(In [6569]) Refs #3192, revert [6558], modify datatable JS so limit/offset parameters are not sent to graphs.

comment:13 Changed 21 months ago by matt (mattab)

Looks good - can I please make one more request,

  • I think it would be nice to always show "Mobile" in the datatable row even if 0 visits, to make the report consistent with the title "Desktop VS Mobile" ?

Changed 21 months ago by capedfuzz (diosmosis)

Patch for always showing mobile/desktop rows.

comment:14 Changed 21 months ago by capedfuzz (diosmosis)

I had to add a new filter for the last request (since API methods can get DataTable_Arrays). Take a look and let me know if it's good to commit.

comment:15 Changed 21 months ago by matt (mattab)

Look good - however you can just inline the filter in this case, since it's as simple as creating the filter, and minimizing number of small filters is better.

comment:16 Changed 21 months ago by capedfuzz (diosmosis)

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [6617]) Fixes #3192, modified getMobileVsDesktop report to always show Mobile/Desktop rows, even if there're visits of one type.

comment:17 Changed 21 months ago by matt (mattab)

(In [6623]) Refs #3192 refactoring

comment:18 Changed 21 months ago by matt (mattab)

(In [6625]) Refs #3192 forgot this file!

comment:19 Changed 21 months ago by capedfuzz (diosmosis)

(In [6679]) Refs #3275 & refs #3192, added report metadata for visits by day of week report and added said report as widget. Also moved code that removed filter_limit/offset params for graphs so related reports wouldn't be affected by #3192 bug.

Note: See TracTickets for help on using tickets.