Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant