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

Change filter hint text to "Games shown: X / Y" and fix user-game segfault #3973

Merged
merged 6 commits into from
May 8, 2020

Conversation

knitknit
Copy link
Contributor

@knitknit knitknit commented Apr 26, 2020

Related Ticket(s)

Short roundup of the initial problem

Cockatrice remembers filters applied between sessions, but users were not alerted if any filters were altered from their default values.

What will change with this Pull Request?

  • Revert Fix #3068: Add hint text about game list filters #3946
  • Instead, change the hint text to read "Games shown: 13 / 42"
  • Update hint text on interesting events, like games being added or newly meeting/not meeting filter criteria
  • Fix segfault that occurred when viewing user's games

Screenshots

Screenshot of "Games shown" text:
Screen Shot 2020-04-30 at 8 16 55 PM

Screenshot of viewing user's games, client did not crash before/during/after:
Screen Shot 2020-04-30 at 8 17 12 PM

@knitknit
Copy link
Contributor Author

@tooomm @ZeldaZach this appears to work the way I would expect, though I'm wondering if there's a more efficient way to do it?

Once we agree on general approach, I'll wire in the changes to also update the hint text as the game list is updated, if we want to. Thanks!

@knitknit knitknit marked this pull request as ready for review May 1, 2020 00:32
@knitknit knitknit changed the title WIP Change filter hint text to "Games shown: X / Y" Change filter hint text to "Games shown: X / Y" and fix user-game segfault May 1, 2020
@ebbit1q
Copy link
Member

ebbit1q commented May 3, 2020

Still crashes when trying to view a user's games, did you test this locally? You can follow the steps in #3979 to reproduce.

@knitknit
Copy link
Contributor Author

knitknit commented May 4, 2020

Still crashes when trying to view a user's games, did you test this locally? You can follow the steps in #3979 to reproduce.

So strange! I just brought up a fresh build with the change again and I don't see it crashing. Maybe this is a difference between Mac and Windows?

Just to make sure we're talking about the same thing - what do you mean by crash? I assume the client totally freezes and/or exits out? Or does it appear fine to a user but show errors in a console somewhere? (Sorry if these are silly questions, still a new contributor.)

@ebbit1q
Copy link
Member

ebbit1q commented May 5, 2020

There are no silly questions, by crash I mean it segfaults the same as in the linked ticket I made, anyway I must have messed up building (probably was still on master) because I built it again and it now works fine.

Copy link
Member

@ebbit1q ebbit1q left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great!

@tooomm
Copy link
Member

tooomm commented May 5, 2020

Will finally get around to check this later today, sorry. 🙈

@ebbit1q
Copy link
Member

ebbit1q commented May 5, 2020

Travis finished but doesn't update on github https://travis-ci.org/github/Cockatrice/Cockatrice/builds/683312387

This is really annoying cus it blocks merge

@knitknit
Copy link
Contributor Author

knitknit commented May 5, 2020

Travis finished but doesn't update on github https://travis-ci.org/github/Cockatrice/Cockatrice/builds/683312387

This is really annoying cus it blocks merge

Anything I can help with from my end to re-run or update it?

@ebbit1q
Copy link
Member

ebbit1q commented May 6, 2020

No, this is an issue with our ci system that automatically validates pull requests, if someone else tests this pr to work well I'll instead merge it manually circumventing the web interface.

@tooomm
Copy link
Member

tooomm commented May 6, 2020

Hmm, this took me a while as I'm still not sure what (or how) to write (it) exactly...

First things first:
The feature seems to work and I prefer this over the initial approach in #3946. 👍
I also can no longer crash the client. 😄


Note: This PR adds the counter of currently visible/shown games requested in #583 and extends it for the number of total games in this specific room on the server. With a different location to display it. :)


But...
I created the requesting issue (#3068) with the problem in mind, that a user should quickly & easily know once default filters are altered. I realized that I worded that not ideal as I also wrote "clarify to the user that the game list doesn't show all available games" next to its title and the quote.


To wrap it up - what this PR basically does

  • Show number of displayed games
  • Show number of total games in this room (not on all server, which is good!)
  • Clear filter button is disabled after click (👍)

What this PR doesn't

From this PR description:

Short roundup of the initial problem

Cockatrice remembers filters applied between sessions, but users were not alerted if any filters were altered from their default values.

I'm still pretty sure - even with this PR - users don't know if default filters do apply or if they are altered. Maybe I'm completely overlooking something here?
Showing all existing games /= default filter settings!
Most of the created games are not relevant to the user normally as they are already running/filled up (=unavailable games). Hence they are not shown on default.
Only hitting the clear button indicates the user that default values are used, because it disables the button. On the other hand, setting default values manually still shows the button active...


What to do with this PR now? It still adds a nice value to the client.

I propose to also disable the Clear filter button if filters set by hand do match default values.
Additionally, IMO the ideal solution would be to move the game count to the headline above the game list (fixes #583), and just show a simple text hint next to the filter buttons once default is altered.
Maybe such a hint can completely be skipped as long as the clear button is consistently disabled on default filter setting! Also, the current location to show the game count is not necessarily worse - I just like the headline more for consistency reasons.

Thoughts?

@ebbit1q
Copy link
Member

ebbit1q commented May 6, 2020

The main issue this addresses is the people complaining about not seeing any games going on. I'd like the amount of games shown in the ui somewhere merged as quickly as possible. The reset filter button behavior can be split off if need be.

Copy link
Member

@ZeldaZach ZeldaZach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this makes me happy, and i'd like to ship it as-is. If there are changes to be made in the future, that can happen.

@ZeldaZach ZeldaZach merged commit 1988e45 into Cockatrice:master May 8, 2020
@knitknit
Copy link
Contributor Author

knitknit commented May 8, 2020

Sounds good, thanks all! @tooomm feel free to reopen and assign #3068 to me and we can discuss the idea of default filters there, if we'd like to tweak it?

@knitknit knitknit deleted the filters branch May 8, 2020 22:55
@tooomm tooomm mentioned this pull request May 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants