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

WEB: Add a "Recommended download" button to the downloads page #8

Merged
merged 14 commits into from
Nov 17, 2013

Conversation

bluegr
Copy link
Member

@bluegr bluegr commented Nov 5, 2013

The idea has been taken and adapted from the VideoLAN web site
(http://www.videolan.org). A big 'download' button is shown,
which links to the most appropriate version for the user, depending
on the OS reported by the user's browser. Only OSes with a browser
have been added. Known browser user-agent strings have been taken
from http://user-agent-string.info/list-of-ua/os

The idea has been taken and adapted from the VideoLAN web site
(http://www.videolan.org). A big 'download' button is shown,
which links to the most appropriate version for the user, depending
on the OS reported by the user's browser. Only OSes with a browser
have been added. Known browser user-agent strings have been taken
from http://user-agent-string.info/list-of-ua/os
@bluegr
Copy link
Member Author

bluegr commented Nov 5, 2013

Here's how it currently looks like:

recommended_dl

@lordhoto
Copy link
Contributor

lordhoto commented Nov 5, 2013

I think this is a good idea (and I also think we had some plans on this in the past, it feels much too familiar at least ;-)). However, I'm not sure about this particular implementation. It looks like we will have to maintain another instance of "what's the current version" in javascripts/recommended_dl.js now (i.e. in addition to the list in downloads.xml). IMHO it would be better if we could merge this to reduce the duplication and lower maintaince burden.

Also, Windows is default? That sounds just wrong, I think it makes more sense to not show any download button in that case or to link to the sources.

I will try to actually test this later and see how it behaves in practice.

@bluegr
Copy link
Member Author

bluegr commented Nov 5, 2013

Thanks for the feedback :)

The Windows version has been set as default because of the significantly larger number of downloads it has compared to the others - check the download stats for version 1.6.0 on SF:
https://sourceforge.net/projects/scummvm/files/scummvm/1.6.0/

We could remove the default, and not show a button at all if Javascript is disabled, or if the user's system isn't recognized from the browser's user-agent string.

Theoretically, it is possible to just generate the Javascript download array for the script, but we'll need to add another tag to each download (the user-agent string to look for) - the other data is already there. I'll check if it's feasible.

Add a new XML field, "user_agent", used to find the user agent string
where the download item in question will be suggested to the user. Only
items with the "user_agent" field can be provided as recommended
downloads
@bluegr
Copy link
Member Author

bluegr commented Nov 8, 2013

Recommended downloads are created automatically from the XML data now.

Only the downloads with the new "user_agent" field are picked to be presented as recommended downloads.

I'll leave the "Windows" entry as default, based on the number of downloads we have for the Windows version compared to the others. It's better to have a button that shows a recommended download, rather than having a blank spot there.

@bluegr
Copy link
Member Author

bluegr commented Nov 8, 2013

Here's how it looks now:

untitled

@clone2727
Copy link
Contributor

Defaulting to Windows really makes no sense. If you're on Windows, then you'll get the Windows button. Otherwise, why would you be given Windows as default? If I use say, my Wii browser, why should I get Windows as the default? It's irrelevant how many people download the Windows builds; it's far better to not show the button (or the section) if the platform is not known.

We also need to take padding into account, which was omitted in the
previously updated images
Also, the creation of the button itself has been pushed to Javascript.
The recommended d/l section is now only shown if JS is enabled and an
appropriate user-agent has been matched
@bluegr
Copy link
Member Author

bluegr commented Nov 8, 2013

@clone2727 : The idea was that some users disable Javascript in their browsers (e.g. via the noScript addon). Also, some antivirus software may change the user-agent string. This was why the Windows (i.e. the most popular) package was shown.

I've changed this behavior, so if a user has Javascript disabled, or if no user-agent match is found, then the recommended download button (and the whole section) isn't shown at all

@bluegr
Copy link
Member Author

bluegr commented Nov 12, 2013

I've uploaded this for testing to:
http://thebluegr.users.sourceforge.net/scummvm-test/downloads/

@somaen
Copy link
Member

somaen commented Nov 12, 2013

This doesn't work at all for the Mac-downloads right now, changing the User Agent in Safari triggers the Windows-dialogue just fine, both for faking IE and faking Firefox (Windows), but the instant I try to use any Mac-user agent, I won't get anything at all here.

For the record, the default user agent I have is: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9) AppleWebKit/537.71 (KHTML, like Gecko) Version/7.0 Safari/537.71

@bluegr
Copy link
Member Author

bluegr commented Nov 12, 2013

@somaen: Ah, I see. In downloads.xml, the user-agent (inside the user_agent field) for the Mac downloads is defined as "MacOS". I just changed it to "Mac OS X" on the testing website. Can you try again?

@somaen
Copy link
Member

somaen commented Nov 12, 2013

It works now.

If I might suggest putting the OS-logo alongside the os-identifier in that box, to make it a tad clearer what gets downloaded though?

It isn't as important now that the default to Windows has been removed (in which case it would be really bad not to do so).

Otherwise, well done.

  1. nov. 2013 kl. 14:10 skrev Filippos Karapetis notifications@github.com:

@somaen: Ah, I see. In downloads.xml, the user-agent (inside the user_agent field) for the Mac downloads is defined as "MacOS". I just changed it to "Mac OS X" on the testing website. Can you try again?


Reply to this email directly or view it on GitHub.

@criezy
Copy link
Member

criezy commented Nov 12, 2013

It looks good to me. But is that supposed to work on Linux? I see you have added user-agents for Linux downloads in the download.xml file (e.g. Debian/Ubuntu/SlackWare/Fedora) but I have never seen web browser using those in their user agent. I tested your page on my Debian machine and on my Ubuntu VM and in both cases I don't get the button. For example on Debian I have:
Iceweasel: Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20100101 Firefox/11.0 Iceweasel/11.0
Chromium: Mozilla/5.0 (X11; U; Linux x86_64; en-US) AppleWebKit/534.3 (KHTML, like Gecko) Chrome/6.0.472.63 Safari/534.3

Note: sf.net gives me the source package as recommanded download when it detects Linux. Should we do the same? The current solution of not having the button is fine with me as well.

@bluegr
Copy link
Member Author

bluegr commented Nov 12, 2013

@criezy: Thanks for testing. I got some of these user-agent strings from the VideoLAN site code. As we do have compiled packages for *nix systems, perhaps it would be best if we just don't offer a recommended download button for any *nix flavor?

@criezy
Copy link
Member

criezy commented Nov 12, 2013

Now that I am home I tested it on my mac with all the browsers installed. It worked with Safari, Opera and Chrome but not with Firefox.

Firefox user agent is: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20100101 Firefox/25.0

@somaen
Copy link
Member

somaen commented Nov 12, 2013

Interestingly, it DOES work in Safar, when I tell Safari to fake Firefox Mac's User Agent, which then returns:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9) AppleWebKit/537.71

@Templier
Copy link
Member

I tested on a couple platforms and browsers. It seems iPhone/iPad are not detected correctly and I can confirm MacOSX /Firefox is not working at all [1]:

  • Windows / Chrome, Opera, IE: OK
  • ⛔ MacOSX / Firefox: not detected (navigator.appVersion is 5.0 (Macintosh))
  • MacOSX / Chrome, Safari: OK
  • Android Nexus 7 / Chrome: OK
  • Android Galaxy Nexus / Chrome, Stock browser: OK
  • ⛔ iPhone / Safari: wrongly detected as Mac OS X
  • ⛔ iPad / Safari, Chrome: wrongly detected as Mac OS X

[1] You also happen to be using undefined values starting at recommend_dl.js:32 as versions[OS] is undefined when OS == ""

@bluegr
Copy link
Member Author

bluegr commented Nov 13, 2013

Many thanks for the feedback @somaen, @criezy and @Littleboy! Much appreciated :)

I've hopefully fixed the Mac + iPhone/iPad detection, plus fixed the undefined variables issue. Could you please test again? Both PPC and Intel Macs, as well as iPhone/iPad should be correctly identified now.

Also, concerning *nix: should we provide a download to the source code, or no download at all? I'm in favor of the latter, as it's impossible to distinguish the *nix flavor used from the user-agent.

@bluegr
Copy link
Member Author

bluegr commented Nov 13, 2013

Apparently, there are some versions of Firefox and Chrome that include the "Ubuntu" string in their user-agent:
http://www.useragentstring.com/pages/Firefox/
http://www.useragentstring.com/pages/Chrome/

Not sure if these are release or development versions, or custom user agent strings, modified by users themselves.

Edit: Seems like this isn't reliable at all:
http://stackoverflow.com/questions/7644912/how-can-i-distinguish-between-fedora-and-ubuntu

So IMHO it's not worth trying to distinguish *nix versions at all. I'll remove them completely for now

It's not possible/reliable to distinguish *nix flavors from the web
user-agent.
@criezy
Copy link
Member

criezy commented Nov 13, 2013

If we cannot distinguish the *nix flavor I agree with having no recommended download.
Note however that it looks like some Linux distributions (e.g. Ubuntu) include the distro name for the firefox version included in the distribution. So maybe we could detect it when that is the case.
See also https://bugzilla.mozilla.org/show_bug.cgi?id=567679

@criezy
Copy link
Member

criezy commented Nov 13, 2013

I have rested on Mac. Now all browsers (including Firefox) propose the Mac download as preferred download.

I also checked on iPad and I get the iPhone download as preferred download. I have no idea if it can be used on a jailbroken iPad though - mine isn't and doesn't allow any download. And I am not sure if it is a good idea to have a big red button shouting CLICK ME if it cannot be used.

@bluegr
Copy link
Member Author

bluegr commented Nov 14, 2013

@criezy: Granted, there are some versions of Firefox that had the "Ubuntu" string in their user-agent, but according to this page, linked from that bug:
https://developer.mozilla.org/en/User_Agent_Strings_Reference
they are trying to reduce the information given away from the user-agent string. Thus, IMHO, although there are some versions of Firefox with that string, it's wrong to assume that it's a standard. I'll add handling for Ubuntu again, as this seems to be a special case.

As for iPhone/iPad: indeed, only jailbroken phones with iOS can download ScummVM, but I assume that this is already known, regardless of the existence of a recommended download button. If a user is browsing the downloads page with an iPhone/iPad, it is assumed that he wants to download ScummVM.

It seems that the "Ubuntu" string is part of the user-agent for some
browser versions in Ubuntu, thus we could use it for our recommended
download button, if the string exists
@bluegr
Copy link
Member Author

bluegr commented Nov 14, 2013

If all the issues have been resolved, I'll go ahead and merge this on Sunday evening, as all the major issues have been identified and addressed. Any further tweaks and changes can be done on master.

@sev-
Copy link
Member

sev- commented Nov 14, 2013

OK to merge from me

@lordhoto
Copy link
Contributor

I gave it a really quick check with JavaScript disabled on my Linux system and everything seems to work fine (i.e. it looks just like what we have currently).

@bluegr
Copy link
Member Author

bluegr commented Nov 17, 2013

All blocking issues have been addressed, this has been thoroughly tessted by many people, @sev- and @lordhoto are OK with it, so I'm merging this. Any further improvements can be done on master

bluegr added a commit that referenced this pull request Nov 17, 2013
WEB: Add a "Recommended download" button to the downloads page
@bluegr bluegr merged commit ef8f241 into scummvm:master Nov 17, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants