- User Since
- Jul 2 2017, 4:22 PM (198 w, 3 d)
Dec 1 2019
I think this is okay. While a build-time copy isn't perfect, having a binaries' help text rely on an external file seems equivalently undesirable. A help text should always work in my opinion.
Thanks you so much for taking a look at this! Strong GL ES support has been on my wishlist for a long time. If the minimap still doesn't support GL ES (it didn't last I looked), I have a patch from years that could be salvaged rather than starting from scratch.
Sep 6 2019
With a much more thorough review, I believe my earlier concerns were completely misplaced. While I think we need to sit down and think about a cleaner way to handle all the possible lobby states, this does move things in the right direction. I only have one minor comment about side-effects.
Woah, give this one a minute for review. I think this is really the wrong approach and needs major rework before landing. I haven't had a chance to write up all my comments yet, but I wanted to quickly jump in and say that I think this should be rolled back until the review process can run its course.
Jul 20 2019
I'm not entirely sure that all these changes are needed, but they seem okay. Coming from mostly writing C, it strikes me as particularly strange to modify something passed as a C++ reference. However, it doesn't seem that there's anything technically wrong with that. I assume you've tested that hosting a game and STUN still work?
Jul 19 2019
Thank you for bringing the problems with rP19703 to our attention Krinkle! It's much appreciated.
Correct the handling of sessionInitiate based off discussions with elexis on IRC yesterday.
Jul 17 2019
Remove a superfluous newline and tweak the wording
Add copyright year updates
Change the diff to be from the repository root rather than the source folder.
D2090 brought to my attention that a few outstanding comments on this were never addressed. I've annotated the remaining comments with where they were fixed and opened D2093 to handle the remaining problems.
Jul 15 2019
Why put this behind a hotkey? This seems to me like desirable default placement behavior.
Jul 16 2018
Okay, seems reasonable; we want the user to verify their identity before accepting the terms. We could ask for the user to re-accept the terms (if they've changed) on another page after the login one to improve the experience. I can look into implementing that (and maybe storing the terms acceptance state per-account on the server) if there are no objections.
There seems to be an issue where we do not always correctly persist the state of the user's acceptance on first-run.
- Reset to default config
- Open lobby, enter incorrect password, accept terms, click login
- Observe incorrect password message
- Enter correct password
- Observe requirement to re-read and accept the terms, even though you already did that during your earlier login attempt
This seems to almost always work great, and the code looks pretty good. One bug though when I tested locally:
Jan 24 2018
Changes look great at a glance! I will try to take a longer look at it and try running it locally later today.
Fantastic direction! That panel should always have automatically hidden when empty, so no need to make it configurable. Thanks for finally looking into fixing this! I did a quick run over the patch and tried to point out anything that I imagine possibly being a problem.