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

[WICKET-6544] mobile browser detection is improved #269

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@solomax
Copy link
Contributor

commented Mar 21, 2018

No description provided.

is(equalTo(6)));
assertThat(userAgent, webClientInfo.getProperties().isBrowserMozillaFirefox(),
is(equalTo(true)));
assertThat(userAgent, webClientInfo.getProperties().isBrowserMozilla(),

This comment has been minimized.

Copy link
@solomax

solomax Mar 21, 2018

Author Contributor

This part is something I don;t really like in this PR :(

@martin-g

This comment has been minimized.

Copy link
Member

commented Mar 22, 2018

With the addition of these new mobile browsers user agent strings the code becomes more complex.
Maybe we should think in direction how to make use of external libraries to do this checks, e.g. https://github.com/pieroxy/java-user-agent-detection or https://github.com/HaraldWalker/user-agent-utils, https://github.com/browscap/browscap/wiki/Using-Browscap#java

@solomax

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2018

And/or maybe browser list and supported patterns can be cleaned up to support more recent versions only?

@kbachl

This comment has been minimized.

Copy link

commented Mar 28, 2018

drop this "feature" completely - it just wont work reliable anyway

@klopfdreh

This comment has been minimized.

Copy link
Member

commented Apr 4, 2018

I also looked a bit around and stumbled over Apache DeviceMap which has been moved into the Attic and from the Attic (http://attic.apache.org/projects/devicemap.html) I followed the link to https://github.com/nielsbasjes/yauaa which seems to be maintained.

@kbachl - I understand your suggestion to remove it because of reliability, but I would keep the functionality with the hint that there might be some not supported browsers which is better than no detection.

Edit: So my vote goes to https://github.com/nielsbasjes/yauaa

@klopfdreh

This comment has been minimized.

Copy link
Member

commented Apr 5, 2018

I just tested a bit around and I would suggest the following changes:

  • Drop the support for minor version in exchange for a full version String (11.0, 3.5.8, 4.0b4a)
  • Change the implementation to yauaa - I also played around a bit and found some not correctly resolved version numbers (the browsers are all detected correctly) nielsbasjes/yauaa#75
  • Change the UserAgent enum to not handle excludes, but only browser names

If the issue is fixed I can provide a PR.

@svenmeier

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2018

Guys I get it, there are many good solutions to choose from, but why should we make Wicket dependent on anyone of them?

https://github.com/nielsbasjes/yauaa looks good, but do users really need assistance for this?

UserAgent userAgent = analyzer.parse(new WebClientInfo(requestCycle).getUserAgent());

@klopfdreh

This comment has been minimized.

Copy link
Member

commented Apr 5, 2018

In my opinion and because of such a big pool of browsers I would say yes.

@klopfdreh

This comment has been minimized.

Copy link
Member

commented Apr 12, 2018

See my suggestion over here: #275

@klopfdreh

This comment has been minimized.

Copy link
Member

commented Apr 26, 2018

Close this one in favour of the mentioned one? (#275)

@solomax

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2018

OK, let's close this one :)

@solomax solomax closed this Apr 26, 2018

@asfgit asfgit deleted the WICKET-6544-mobile-browsers branch May 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.