online-go / online-go.com

Source code for the Online-Go.com web interface
https://online-go.com/
GNU Affero General Public License v3.0
1.26k stars 346 forks source link

Date sort puts games with opponent on vacation at the very top #424

Closed apetresc closed 11 months ago

apetresc commented 7 years ago

Summary

When you have the game list sorted by date, games where the opponent is on vacation appear to be stuck to the very top no matter how much time is actually remaining. For example, this screenshot:

At least, I'm assuming it is my opponent's vacation flag that is causing this unexpected sort, obviously the game with 6 hours left should be at the top. If anything, games where the opponent is on vacation should be at the very bottom of the list, since they require the least immediate attention.

I've lost games on time because in thumbnail view, a quick glance at the top row or two didn't show any games where I was in danger of timing out, because of lots of opponents on vacation, but there was one lurking right beneath the fold :(

Environment

Steps To Reproduce:

  1. Sort by Clock
  2. Have a game where the opponent is on vacation
  3. See it at the very top regardless of how many other more urgent games there are.
anoek commented 7 years ago

Hmm, I'm having a hard time reproducing this, everything seems to be sorting good for me whether or not opponents are on vacation or otherwise paused =/. There might have been something else going on as well that caused this.

apetresc commented 7 years ago

Hm, yeah, there's definitely something weird going on - I can reproduce it quite consistently, although there are some weird wrinkles for me as well. For example, here is my current game list, very recently refreshed:

image

The Turing tournament respects weekend time, so the first three entries are all from there - even though game 4 against mimano should be sorted higher than my game against Peter K, based on time remaining. This would be completely consistent with my assumption that paused games are always sorted first... except for the last game on my move which is also on weekend time, but is sorted correctly at the end. :man_shrugging:

So, I'm not sure exactly what's going on, but I do know that my game list is very consistently mis-sorted whenever vacations are happening :(

BHydden commented 2 years ago

This can be closed. Same issue as #500

BHydden commented 1 year ago

I was wrong, the issues were the same, but the problem persists.

BHydden commented 1 year ago

3d06445d-1882-4982-8ac7-0524bdacb206 Example of vacation game sorting above games where the clock is active.

dexonsmith commented 11 months ago

The sorting logic for clocks in GameList.applyCurrentSort is straightforward and doesn't directly consider paused games. The logic is:

  1. is it the player's turn?
  2. when does the clock expire? (clock.expiration)
  3. what is the game ID?

I tried to track down how clock.expiration is set for paused games and couldn't find it (I suspect it's server-side, and I don't have access to that), but I did discover that the data type appears to be number in src/models/games.d.ts.

I suspect that accessing/comparing the clock.expiration for a paused game fires an exception, which falls back to an == result.

If so, this would throw off the entire sort, since the comparison fails to follow strict-weak-ordering.

Consider the list of expiration times:

The pair-wise comparisons are:

With such a comparison function, the following are all "valid" results of the sort:

dexonsmith commented 11 months ago

I suspect that accessing/comparing the clock.expiration for a paused game fires an exception, which falls back to an == result.

Or, perhaps that what used to happen?

Playing with game lists on beta.online-go.com, it looks to me like paused games are consistently at the top of the list (or the bottom when in reverse sort). This suggests that clock.expiration is 0 or negative, not something that throws. I'll play around a bit more.

dexonsmith commented 11 months ago

Aha. I added some extra output to Clock.tsx:

        <div>since={goban.last_clock.paused_since}</div>
        <div>expiration={goban.last_clock.expiration}</div>

It looks like when a game is paused, last_clock.expiration is set to the time that the game was paused (equal to paused_since). This is why paused games are at the top of the list: their expiration time is in the past.

dexonsmith commented 11 months ago

I see a few ways to fix this.

  1. Keep relying entirely on the AdHocClock. Sniff out whether the game is paused by looking at expiration and paused_since. If paused_since is set, that means the game was ever paused, since it's never unset. So, check if they're set and equal, and if so, consider the game "paused" and sort it after "unpaused" games.
  2. Check the JGOFClock to figure out if the game is paused.
  3. Same as (2), plus switch sorting logic over to JGOFClock (dropping this use of AdHocClock).

The JGOFClock option(s) seems cleaner. But I'm not sure if/when this JSON fallback is needed:

                        const a_clock =
                            a.goban && a.goban.last_clock ? a.goban.last_clock : a.json.clock;

and the JSON fallback relies on AdHocClock.

@anoek, any thoughts?

anoek commented 11 months ago

Yeah the pause system is kind of ugly. I reckon we need a method added to easily check and see if a game is paused or not. Here's how we do it in GobanCore:

https://github.com/online-go/goban/blob/main/src/GobanCore.ts#L3260-L3264

along with the definitions of those two functions later in that file:

https://github.com/online-go/goban/blob/main/src/GobanCore.ts#L4005-L4049 https://github.com/online-go/goban/blob/main/src/GobanCore.ts#L4125-L4130

dexonsmith commented 11 months ago

@anoek, I had a look a deeper look, and I don't think we have pause state available in the initial sort. Have a look at https://github.com/online-go/online-go.com/pull/2440, which documents my findings in code comments.

Bottom line: the pause state isn't available at the time of the initial sort, before rendering. I tried referencing it only when available (in subsequent sorts after rendering) but it made the game list glitchy.

dexonsmith commented 11 months ago

I think these are the options for fixing (but maybe I'm missing something):