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

fix Android tablet detection in VERSION_TRUNCATION_MAJOR mode #7414

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

blinkseb
Copy link

Description:

Hello,

First, thanks a lot for this awesome library!

Because of privacy concerns, I'm using this library in VERSION_TRUNCATION_MAJOR, and I noticed some issues when parsing some old Android 3 tablet user-agents. The device ends up empty instead of tablet.

Looking at the code, it's because of the builtin version_compare, for which, I'm not sure why, 3 is lower than 3.0 instead of being equals. I fixed the issue by using only major versions in version_compare, instead of major + minor.

For the tests, I've just duplicated the existing ones, running with VERSION_TRUNCATION_MAJOR instead of VERSION_TRUNCATION_NONE. There's probably a better / faster solution, please tell me what you think.

Thanks a lot in advance!

Review

@blinkseb blinkseb force-pushed the fix-tablet-detection branch from 0eb5389 to f557e08 Compare May 29, 2023 19:10
@blinkseb blinkseb force-pushed the fix-tablet-detection branch from f557e08 to 49ca643 Compare May 29, 2023 19:31
@blinkseb
Copy link
Author

It was a bit harder than expected, because the browser version is used in regexes to guess the browser engine, and, when truncated, it leads to different results.

I extracted the version truncation from the buildVersion to a dedicated truncateVersion and updated the code accordingly. This allows to use the full version to guess the engine, while propagating the truncated version.

Let me know what you think.

Thanks!

@sgiehl sgiehl requested a review from sanchezzzhak May 31, 2023 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant