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

Screen resolution should be tracked correctly #9914

Closed
wants to merge 1 commit into from
Closed

Screen resolution should be tracked correctly #9914

wants to merge 1 commit into from

Conversation

mattab
Copy link
Member

@mattab mattab commented Mar 10, 2016

fixes #9911

@mattab mattab added the Needs Review PRs that need a code review label Mar 10, 2016
@mattab mattab added this to the 2.16.1 milestone Mar 10, 2016
@tsteur
Copy link
Member

tsteur commented Mar 10, 2016

I'm not sure if we should do too much validation here in Tracker API. In this case the user was using an old version of Piwik and it would have been fixed by updating. I'd rather have the raw data in DB whatever was sent and format if needed later in the report etc. Ideally the code was written well enough that we need to format output only only in one place wherever resolution is used. This gives developers to track whatever they want as the Tracking API is kind of generic and might not only relate to screen resolutions but possibly also resolutions. Now, if we think of pixels, they will be likely always an integer. But if we think of other resolutions such as tracking it in mm (millimeter), on Android you could technically use 0.5dp or so etc.

Also re check for width > 9999. There are already screens with 7680×4320 and likely there are at some point soon, if not already, screens > 9999. Something like this should definitely not be done in Tracking API. The tracking API has to be "stupid" and not do such things like checking whether the width is greater than something.

@tsteur
Copy link
Member

tsteur commented Mar 10, 2016

Similar problem where API tried to be smart is here #9842 (comment) .

@tsteur
Copy link
Member

tsteur commented Mar 10, 2016

noticed #9913 re 9999px. I would still remove it here meanwhile. And ideally also not use round as a generic tracking API should not make such assumptions about what the resolution might be

@mattab
Copy link
Member Author

mattab commented Mar 14, 2016

I agree generally Tracking API shouldn't validate data. in this case I thought it makes some sense, because the config_resolution DB table column is by design meant to store a screen resolution. Since we store resolution in one string column it is important to validate that the screen resolution is correctly formed... Resolution must include a width and a height. we validate the width and height to prevent storing an invalid resolution.

In general if it's not good to validate data in tracking API, an alternative solution would be that the Archiver validates the resolution labels and removes those that are invalid... is this maybe this you would prefer?

@tsteur
Copy link
Member

tsteur commented Mar 14, 2016

In general if it's not good to validate data in tracking API, an alternative solution would be that the Archiver validates the resolution labels and removes those that are invalid... is this maybe this you would prefer?

Yes that's something I would prefer. This let's us later reuse the same field for tracking some "resolution" in "mm". In some cases it might even make sense to track screen resolution in mm instead of px. We'd need to create a custom report / archiver for this but that's fine.

@tsteur
Copy link
Member

tsteur commented Mar 14, 2016

because the config_resolution DB table column is by design meant to store a screen resolution

In general we need to rethink some of the meanings while we're going generic. Don't have to change the meaning everywhere but could make sense for some columns.

As for Piwik 2.16.0 the bug should be fixed already. For Piwik 3+ we could issue an alter update to increase the size of this field and track all kinda values. If they track some values wrong, they still can make at least something out of the data instead of having them all tracked as unknown. Eg when only width was tracked they know at least the width. It seems not so important for resolution but I'm more thinking globally how the Tracking API should behave. We can always remove invalid data afterwards but we will never be able to restore not tracked values (or values set to unknown)

@mattab mattab modified the milestones: 2.16.x (LTS), 2.16.1 Mar 15, 2016
@mattab
Copy link
Member Author

mattab commented Mar 15, 2016

OK I understand your point of view. We don't really need this change for now.

As for Piwik 2.16.0 the bug should be fixed already. For Piwik 3+ we could issue an alter update to increase the size of this field and track all kinda values.

Covered in #9913

@mattab mattab closed this Mar 15, 2016
@mattab mattab deleted the 9911 branch April 7, 2016 01:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants