search

m0r13 / mclogalyzer

Minecraft Server Log Analyzer
GNU General Public License v3.0
44 stars 20 forks source link

Track rage quits, update HTML, change URLs #12

Closed johnwoltman closed 9 years ago

johnwoltman commented 9 years ago

There's three main things in this PR.

Rage Quit Tracking - compares the last death time to your log out time, if it's less than a certain time period, add one to the rage quit counter. I used 45 seconds, because that gives someone enough time to swear about their death then log out.

Updated URLs - Made the CDN-hosted CSS and scripts use relative protocols, and changed the tablesorter to the jsdelivr CDN.

Updated Bootstrap 3 support - updated the CSS where it made sense, and added support for collapsible navbars.

m0r13 commented 9 years ago

Thanks for your changes again! I also like your idea with the rage quits.

A small problem is for me that I can't really view the generated file locally (opened with file://) because jQuery, Bootstrap and tablesorter aren't loaded via file://. Is there a way to workaround this?

Edit: Can't we load the files via https:// per default?

johnwoltman commented 9 years ago

I had the same problem too. I used pip to install simplehttpsserver. Then I ran python -m SimpleHTTPServer in the same directory as my output html file.

thorerik commented 9 years ago

I believe should rather just use https:// by default, I believe most CDNs today use SPDY anyways, so the transferspeeds, despite overhead added by tcp and ssl handshake is minor compared to the improvements in https(which we will see in http 2.0 spec as well).

Another benefit would be not to rely on dependencies that are silly and not strictly needed, such as simplerhttpsserver just to be able to view the generated files locally.

On a side note which is more just commiting ettiquette in this case is that you should squash commits to 1 commit and 1 change, 1 PR, thus, if eg. rage quit tracking wasn't desirable(I can't see why not in this case, but roll with me :)), the other 2 could be accepted and the rage quit tracking denied, but as it stands you'd need to cherrypick the commits, which is tedious.

m0r13 commented 9 years ago

I agree with @thorerik. I'll merge your changes and change the url schemes.

johnwoltman commented 9 years ago

Thorerik, thanks for the advice. I'm relatively new to GitHub and working collaboratively. My initial PR for this was even worse: I had somehow rebased my code on an old version of m0r13's, and then I had to fix all my new stuff, and then git complained about conflicts, and I made like 7 commits before I got it right. At that point it was such a mess that I deleted my fork and recreated it to keep the commit branch cleaner.