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

Flattening Tables via API and UI #3073

Closed
timo-bes opened this issue Mar 28, 2012 · 35 comments
Closed

Flattening Tables via API and UI #3073

timo-bes opened this issue Mar 28, 2012 · 35 comments
Assignees
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical.
Milestone

Comments

@timo-bes
Copy link
Member

Provide a mechanism to make nested tables into a single flat table. This will allow users to compare (i.e. sort) rows from different data tables that make up a nested report.

  • Introduce a new API parameter flat=1
  • Add a UI for flattening tables
  • The CSV export does something very similar if expanded=1 is set. Can we reuse the flattener for the CSV export?
@timo-bes
Copy link
Member Author

(In [6124]) refs #3073 data table flattener and test cases

@timo-bes
Copy link
Member Author

The previous commit introduces a base class for manipulating data tables. The idea is that the label filter uses it as a base as well.

Please review.

@timo-bes
Copy link
Member Author

(In [6125]) refs #3073 svn properties for previous commit

@timo-bes
Copy link
Member Author

(In [6126]) refs #3073 proper class name for tests

@timo-bes
Copy link
Member Author

There's a number of things that the UI mechanism should provide.

  • The obvious one is to switch flattening on/off.
  • Since there are multiple version of flattening planned, we need a way to select that as well.
  • Also there's the option to include inner nodes or not. We could provide good defaults for including inner node via meta data but people might want different things.

How about a dialog to configure the way the table should be displayed?

We could have some explanation of the different options there to make it easier than more icons at the bottom of the table. Also, the selected behavior from the dialog could influence the way the export is rendered. So most of the stuff we discussed about an export dialog would be valid here as well.

Here' a very rough sketch of what I have in mind:

[  ]  Flatten the report

---

(  )  Concatene labels

(  )  Keep labels and indent

---

(  )  Include inner nodes

(  )  Only show the leafs

What do you think?

I know this is not the way we usually do things but adding more icons will become confusing at some point. With this dialog, we can easily add more options in the future. Or is there a more simple way for adding the functionality we need?

@mattab
Copy link
Member

mattab commented Apr 4, 2012

Code review:

  • Good work & useful visualization!
  • I think the flatten modification could be implemented as a standard datatable filter.
    • This would avoid adding new directory datatableManipulator
    • It would be consistent to have it as filter
  • in the tests, please generate URLs with a few sub directories
  • It would be very very nice if we could refactor the css code that is similar to this one! Is it possible?

@mattab
Copy link
Member

mattab commented Apr 4, 2012

Also there's the option to include inner nodes or not. We could provide good defaults for including inner node via meta data but people might want different things.

What would the difference be exactly when this is enabled or disabled?

Since there are multiple version of flattening planned, we need a way to select that as well.

For this, we could for example name each combination, and put a screenshot of what the data will look like when this is ticked. I saw a very good example of such UI in Songbird software. It shows you screenshot of the directory/file organization based on the automatic filename pattern you choose.

  • will there be 4 combinations available?

@timo-bes
Copy link
Member Author

timo-bes commented Apr 4, 2012

For what including inner nodes means, see #2714

In your review, what css code are you talking about?

So should we add a dialog? I would suggest to make it appear between the table and the icons when you click the (leftmost) table icon.

Filters and what I called Manipulators are different (btw: the label filter is a manipulator as well, I want to refactor it into the new directory):

  • Filters mainly change rows and keep the overall structure intact - Manipulators change the entire thing.
  • Filters work on data in memory; the base class loads from Piwik_DataTable_Manager - Manipulators work on archived data; the base class Piwik_API_DataTableManipulator loads them from the API
  • Filters are used in the APIs of plugins - Manipulators are used in ResponseBuilder

Are you sure we should combine those two things? IMO, only loading subtables from different places makes them different enough to keep them separate.

The alternative would be to force expanded=1 and use them in API_DataTableGenericFilter. Then they would work like the other filters. But that's very wasteful for the label filter becuase at the moment, it only loads the subtables from the archive it really needs. That makes a huge difference on reports like the page urls report.

@mattab
Copy link
Member

mattab commented Apr 4, 2012

Replying to EZdesign:

For what including inner nodes means, see #2714

Thanks. we have to change the name... How about "keepParents" or "keepExpanded" ?

In your review, what css code are you talking about?

I meant, it would be nice to use this new code in the CSV exporter which does some of the same logic.

So should we add a dialog?

I have one idea among others:

  • on hover of the table, display an icon, outside the datatable, on the bottom right of the table

It's just an idea, not sure how to integrate this easily yet discreetly as it will not be used a lot and cannot add a new icon visible all the time?

I would suggest to make it appear between the table and the icons when you click the (leftmost) table icon.

agreed, rather than popover..

Filters and what I called Manipulators are different (btw: the label filter is a manipulator as well, I want to refactor it into the new directory):

  • Filters mainly change rows and keep the overall structure intact - Manipulators change the entire thing.
  • Filters work on data in memory; the base class loads from Piwik_DataTable_Manager - Manipulators work on archived data; the base class Piwik_API_DataTableManipulator loads them from the API
  • Filters are used in the APIs of plugins - Manipulators are used in ResponseBuilder

Thanks. Can you please write this as a comment in the manipulator?

Are you sure we should combine those two things? IMO, only loading subtables from different places makes them different enough to keep them separate.
OK as long as code is not duplicated a lot between these 2 similar concepts (but they're different so OK)

The alternative would be to force expanded=1 and use them in API_DataTableGenericFilter. Then they would work like the other filters. But that's very wasteful for the label filter becuase at the moment, it only loads the subtables from the archive it really needs. That makes a huge difference on reports like the page urls report.

Can you please elaborate on this problem?

Cheers

@timo-bes
Copy link
Member Author

timo-bes commented Apr 5, 2012

(In [6159]) refs #3073 flatten table UI, option to include aggregate rows (this option will replace includeInnerNodes in the CSV renderer)

@timo-bes
Copy link
Member Author

timo-bes commented Apr 5, 2012

I thought long and hard about how we could add these new options to the footer that already is pretty packed. So I added a dialog similar to the selection of how many rows you want to display. I put exclude low population and options for flattening in there. It can easily be extended in the future.

I renamed includeInnerNodes to includeAggregateRows (since you suggested to pick a different name). Is that more clear? When the option is selected, the labels of the aggregate rows are suffixed with [aggregate] to make it clear. The option is only there in the UI if the report is flat.

Matt, you mentioned two kinds of flattening in an email to me. If I understood what you meant correctly, both are implemented now. Please check.

@mattab
Copy link
Member

mattab commented Apr 6, 2012

UX Review:

  • well done. You found a good solution to a hard problem. I like this implementation and direction.
  • I replaced icon with a more discreet one, but we can definitely find better
  • Please open the submenu on hover, without requiring a click, as this should be easy to access yet not taking place
  • Aggregate rows: Here I think we could find a way to clearly highlight aggregate rows without displaying text
    • Aggregate rows in bold
    • Prepend the plus + logo
    • It should then be clear that the row is aggregate.
    • Maybe we could also left pad the sub rows: this would show the hierarchy.
  • text 'exclude aggregate rows' -> Hide aggregate rows
  • What other options do we plan to include in this window later on?

@mattab
Copy link
Member

mattab commented Apr 6, 2012

I renamed includeInnerNodes to includeAggregateRows (since you suggested to pick a different name). Is that more clear?

Yes thanks. However there are some "InnerNodes" left in code ini many places can you please change everywhere for consitency?

Matt, you mentioned two kinds of flattening in an email to me. If I understood what you meant correctly, both are implemented now. Please check.

  • That is very cool. How ever the graph don't work with flattened, but it would be very good to be able to plot these tables. It would allow for first time to plot in particular custom variable values, which I know many users are interested in.
  • it should also be fowrarded in all clicks in footer (API, tableGoals, tableAllColumns etc)

@mattab
Copy link
Member

mattab commented Apr 6, 2012

(In [6162]) Refs #3073 New icon more discreet

@mattab
Copy link
Member

mattab commented Apr 9, 2012

One more UX suggestion:

  • when we start recording the full status of each view (exclude low pop, flatten, etc.) to restore the table as they were, I think it will be very important to highlight the fact that the table is currently flattened / or that low populations are excluded (or both).
    • when any option was clicked or initialized in the new menu (btw we need a name for it...), Could you please put the icon in yellow? I think this would at least help users understand a bit why the view is the way it is.
    • it would be also very nice if we could somehow display somewhere "Report is flattened - un-flatten report" or "Rows with few visits are hidden - Show all rows" so it is clear, just by reading the menu, what is the status of the report.

Thoughts?

@timo-bes
Copy link
Member Author

Replying to matt:

  • Please open the submenu on hover, without requiring a click, as this should be easy to access yet not taking place

I did it on click because to number of rows selection opens on click too. They look similar and are located next to eachother. Thus, they should work in a similar fashion. So do we do both on hover or both on click? The problem with hover is that the number of rows selection is pretty narrow so it would be easy to mouseout on accident.

  • What other options do we plan to include in this window later on?

I'm sure we will come up with new things ;-)

Replying to matt:

Matt, you mentioned two kinds of flattening in an email to me. If I understood what you meant correctly, both are implemented now. Please check.

  • That is very cool. How ever the graph don't work with flattened, but it would be very good to be able to plot these tables. It would allow for first time to plot in particular custom variable values, which I know many users are interested in.

I don't understand. Can you clarify? Do you mean row evolution? What is special about the custom variables in this context?

Replying to matt:

  • when we start recording the full status of each view (exclude low pop, flatten, etc.) to restore the table as they were, I think it will be very important to highlight the fact that the table is currently flattened / or that low populations are excluded (or both).
    • when any option was clicked or initialized in the new menu (btw we need a name for it...), Could you please put the icon in yellow? I think this would at least help users understand a bit why the view is the way it is.
    • it would be also very nice if we could somehow display somewhere "Report is flattened - un-flatten report" or "Rows with few visits are hidden - Show all rows" so it is clear, just by reading the menu, what is the status of the report.

Maybe we could show the modified state of the table on hover over the icon. So you'd have a highlighted icon on the bottom of the table. You'd hover over it and the tooltip would show "Report is flat. Aggregate rows are excluded". On click, the menu would open and give you the options. This way, you could quickly see what is going on with the table on hover.

@timo-bes
Copy link
Member Author

Replying to matt:

(In [6162]) Refs #3073 New icon more discreet

Are you sure this is an improvement? I don't think the faded style fits well with the other icons.

@timo-bes
Copy link
Member Author

(In [6181]) refs #2714, refs #3073

  • flat settings are passed to graph views and exports
  • csv renderer does not do flattening anymore. the new api parameters can be used instead - js and tests updated accordingly
  • aggregate rows are italic (imo, the plus logo would be misleading because it would suggest clickability)
  • disable row evolution on flat tables for performance reasons
  • minor language adjustments

@timo-bes
Copy link
Member Author

When you flatten the websites report and then display it as a bar chart, you get weird labels like www.example.org/http://example.org/thePage.html. This is because the flattener gets a data table with the full URLs as labels for the second level. It does not happen when the report is displayed as a regular data table. Do you know why?

@mattab
Copy link
Member

mattab commented Apr 10, 2012

Feedback on new commit:

  • so good to see the csv going away :) I'm so glad you wrote these tests... !

  • graph should always request data with include_aggregate_rows = 0 otherwise graphs are not correct, as they include more data than the sum (this is especially true for pie charts but also incorrect to do so in bar charts)

  • in the code I see includeAggregateRows and include_aggregate_rows -- should they not be both includeAggregateRows for consistency?

    disable row evolution on flat tables for performance reasons
    Is "flat" working OK with row evolution? if it's working but the only problem is performance, I would be very keen to leave the feature in, and we can improve it later (i'm hoping to look into row evolution performance at some point).

I see some very useful use case where you could see the evolution over time of a particular custom variable name-value pair... very meaningful & powerful that would be!

I don't understand. Can you clarify? Do you mean row evolution? What is special about the custom variables in this context?
Sorry, I didn't identify the problem well, you fixed the issue by forwarding the "flat" parameter.

I did it on click because to number of rows selection opens on click too. They look similar and are located next to eachother. Thus, they should work in a similar fashion. So do we do both on hover or both on click? The problem with hover is that the number of rows selection is pretty narrow so it would be easy to mouseout on accident.

My thoughts are that:

  • it is more often used during analysis to "exclude low population" and "flatten table" and is part of the "flow" of analysing data

    • however, changing number of rows is more a one off thing - I think most users would change to what they like and leave it as it is.
  • the "settings" icon is not very visible -- therefore showing the menu on hover on these few pixels will help users discover these features.

    • As opposed to the row selector which is more visible with grey border -- which is good btw, because it displays the number of rows in the table which is useful

    You'd hover over it and the tooltip would show "Report is flat. Aggregate rows are excluded". On click, the menu would open and give you the options. This way, you could quickly see what is going on with the table on hover.

Im definitely more keen for a one fit all approach: show on hover the menu that shows both the current status and links to change status ie.

  • "Report is flattened - un-flatten report"
  • "Rows with few visits are hidden - Show all rows"
    • Maybe it could look nice, for example having the "Report is flattened" text in black not linked, then below it have a link with an arrow "→ Flatten report"

Maybe you can try it and if you don't like it we can all try it and discuss it. I think it will make the feature more accessible and generally better :)

Are you sure this is an improvement? I don't think the faded style fits well with the other icons.

I'm not entirely convinced indeed... but I prefer it than the previous one.

  • Because, the feature allows to "update" the current view (keeping the existing "view" tableGoals tableAllColumns etc.).
  • As opposed to other icons on the left, which reload the view and change the columns or load graphs.

However we should keep open mind as there might be better solution ? thoughts?

aggregate rows are italic (imo, the plus logo would be misleading because it would suggest clickability)

I thought initially about using this icon which would be consistent with the Pages report: http://demo.piwik.org/themes/default/images/minus.png ? I think it would look nice and easier to understand.
But then I realized that aggregated rows are not kept together but are sorted by metric. So, the minus icon would be very confusing in this case.

Displaying in italic is definitely an improvement!

When you flatten the websites report and then display it as a bar chart, you get weird labels like www.example.org/http://example.org/thePage.html. This is because the flattener gets a data table with the full URLs as labels for the second level. It does not happen when the report is displayed as a regular data table. Do you know why?

I took a quick 5minutes look and couldn't find the cause. I'll definitely check before the release if you haven't found the bug then.

I'm sure we will come up with new things ;-)
Definitely... I already thought of one: "Expand all rows" #1040 which would be very cool and useful for example for the Downloads/Outlinks reports (usually a few hosts only) or for the page URLs report (on small websites).

@timo-bes
Copy link
Member Author

Replying to matt:

Is "flat" working OK with row evolution? if it's working but the only problem is performance, I would be very keen to leave the feature in, and we can improve it later (i'm hoping to look into row evolution performance at some point).
I see some very useful use case where you could see the evolution over time of a particular custom variable name-value pair... very meaningful & powerful that would be!

Row evoution does work on custom variable name-value pairs. Just open the subtable and click the icon. Using it on flat tables does not add any functionality. It's just orders of magnitue slower. Also, it doesn't work without further code modifications.

@timo-bes
Copy link
Member Author

Regarding the icon, take a look at this: http://www.iconfinder.com/search/?q=iconset%3Asilk2+cog

We could show the first one (maybe faded out a little) by default. On hover, the menu opens and it's shown opaquely. When low population is excluded or the report is flat, we can show one of the other icons, maybe the one with the small yellow (!) sign?

@timo-bes
Copy link
Member Author

(In [6182]) refs #3073

  • graph views of data tables always request data without aggregate rows
  • csv export js fix: includeAggregateRows => include_aggregate_rows
  • the config menu is more verbose now (added new lang strings rather than reusing existing ones for low population - this way translators will notice the change)
  • refactoring to reduce duplication: label filter becomes a data table manipulator
  • integration test for include_aggregate_rows parameter

@timo-bes
Copy link
Member Author

(In [6183]) refs #3073

  • data table manipulator applies queued filters. this solves the issue with displaying the websites report as a bar/pie chart.
  • temporarily added removed lang strings again. will be removed once we agreed to do so.
  • fixed bug introduced by label filter refactoring.

@timo-bes
Copy link
Member Author

(In [6184]) refs #3073 svn properties for new file

@timo-bes
Copy link
Member Author

(In [6185]) refs #3073: extended test suite FlattenReports

@mattab
Copy link
Member

mattab commented Apr 11, 2012

KABOOM! it looks beautiful!!

Row evoution does work on custom variable name-value pairs. Just open the subtable and click the icon. Using it on flat tables does not add any functionality. It's just orders of magnitue slower. Also, it doesn't work without further code modifications.

That makes sense. Perfect as it is.

Regarding the icon, take a look at this: http://www.iconfinder.com/search/?q=iconset%3Asilk2+cog
We could show the first one (maybe faded out a little) by default. On hover, the menu opens and it's shown opaquely. When low population is excluded or the report is flat, we can show one of the other icons, maybe the one with the small yellow (!) sign?

First one looks good (agreed on fading out as it's too dark).

for the "enabled state", the (!) wouldn't work as it triggers a "warning" feeling. I would be more inclined to have a "highlight" effect (for example yellow hallow around the icon, maybe needs a new icon image) that shows that something is enabled, but should not distract the user, but I'm not sure what would work nice...
thoughts?

worst case scenario we could use: http://www.iconfinder.com/icondetails/9568/22/cog_face_female_gearhead_icon

Feedback:

  • Could you please append the string "(default)" after each blue link, when the feature currently enabled is not the default case?
    • For example "Make it hierarchical (default)"
    • this would reduce user frustration as to losing track of what the repotr should look like. because, we make sure the default report looks the best it should "by default" so it's nice to remind it.
  • OK to remove translation strings, the new UI makes the feature much easier to understand.

@timo-bes
Copy link
Member Author

(In [6191]) refs #3073 new icon, highlighted icon when the configuration differs from default, showing (default) in options

@timo-bes
Copy link
Member Author

(In [6192]) refs #3073 removing left over lang string

@timo-bes
Copy link
Member Author

(In [6194]) refs #3073: cross-browser ie789, safari, chrome, firefox

@timo-bes
Copy link
Member Author

My to do list for this feature is empty now.

Are there any more suggestions? If not, we can close the ticket.

@mattab
Copy link
Member

mattab commented Apr 12, 2012

Well done, Looks good to me :)

@sgiehl
Copy link
Member

sgiehl commented May 3, 2012

(In [6242]) refs #3073 save new options in dashboard, so that they are restored

@mattab
Copy link
Member

mattab commented May 31, 2012

Small bug noticed: the cog icon should appear on the graph, since the graph plotting flattened custom variables is very useful in some cases, but better have a way to disable the flattening.

@mattab
Copy link
Member

mattab commented Jun 1, 2012

Fixed in [6424]

@timo-bes timo-bes added this to the 1.12.x - Piwik 1.12.x milestone Jul 8, 2014
@timo-bes timo-bes self-assigned this Jul 8, 2014
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical.
Projects
None yet
Development

No branches or pull requests

3 participants