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

Add missing feature rememberance to client #2275

Merged
merged 1 commit into from
Nov 29, 2016

Conversation

woogerboy21
Copy link
Contributor

Fix #2249

Add the ability for the client to remember the missing features that it received from the server it last connected to and not display the "missing/optional feature" message at every login. At each startup the client checks the known missing feature list and compares it to its feature set clearing any known features that have been added after a client upgrade. The client will now only display the missing / optional feature window when a new missing feature is found upon connecting to a server only once. Afterwords it will not display the message until an even newer feature the client is missing is found.

Fix Cockatrice#2249
Add the ability for the client to remember the missing features that it
received from the server it last connected to and not display the
"missing/optional feature" message at every login.
@woogerboy21 woogerboy21 added the App - Cockatrice Tickets relating to the cockatrice application label Nov 11, 2016
@Daenyth
Copy link
Member

Daenyth commented Nov 11, 2016

Awesome! I'll review as soon as I get a chance

On Fri, Nov 11, 2016, 12:28 AM woogerboy21 notifications@github.com wrote:

Fix #2249 #2249

Add the ability for the client to remember the missing features that it
received from the server it last connected to and not display the
"missing/optional feature" message at every login. At each startup the
client checks the known missing feature list and compares it to its feature
set clearing any known features that have been added after a client
upgrade. The client will now only display the missing / optional feature
window when a new missing feature is found upon connecting to a server only
once. Afterwords it will not display the message until an even newer

feature the client is missing is found.

You can view, comment on, or merge this pull request online at:

#2275
Commit Summary

  • Add missing feature rememberance to client

File Changes

Patch Links:


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#2275, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAA5NAokULSDLrPyy1ox_vpmNl0-hLmWks5q8_zggaJpZM4KvcFP
.

@Psithief
Copy link
Contributor

So the user isn't allowed to find out what the missing feature is again from some esoteric window deep inside a help menu? :D

@woogerboy21
Copy link
Contributor Author

woogerboy21 commented Nov 13, 2016

So the user isn't allowed to find out what the missing feature is again from some esoteric window deep inside a help menu? :D

Im confused by this. The user is never presented the missing features that there client has unless its a required feature on the server preventing them from logging in. Otherwise there is a popup on login that indicates they are missing features that the server they are connecting to understands. And before this change it pops up every single time the user logs in and there client is missing the feature.

With this change the only difference in user experience is that the client will only present the popup message about missing features once as the client now remembers what those features are. If say for example they log in with a down level client that is missing feature a, there client remembers feature a. Then if the server upgrades again and now the client is missing feature a & b, the user will be presented again with the message when that condition happens (and so on) but wont be presented with the popup every single login. The experience for what features the users see's if denied access to the server from a missing feature doesnt change.

Or are you wanting the ability to see what those missing features might be? If so I wouldnt suggest it be in some buried popup window but possibly in the debug log. Maybe on startup?

@woogerboy21
Copy link
Contributor Author

Was this ever looked at by anyone? I know personally with work and the holiday stuff pretty much here I havent had a bunch of time myself to do much more project wise.

Copy link
Member

@Daenyth Daenyth left a comment

Choose a reason for hiding this comment

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

Overall the approach seems reasonable. I think it might be a little easier to use a QSet instead of trying to manipulate a comma-separated string.

@@ -135,6 +136,13 @@ void RemoteClient::processConnectionClosedEvent(const Event_ConnectionClosed & /
void RemoteClient::loginResponse(const Response &response)
{
const Response_Login &resp = response.GetExtension(Response_Login::ext);

QString possibleMissingFeatures;
Copy link
Member

Choose a reason for hiding this comment

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

Why not a QSet<QString>? That would provide much easier checking, I'd think

Copy link
Contributor Author

@woogerboy21 woogerboy21 Nov 29, 2016

Choose a reason for hiding this comment

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

I actually started out by using a QSet<QString> but ran into an issue with doing so. At this point though I don't recall what the issue was and it very well could have been something to do with me taking a different approach and then changing it after the failures occurred.

@woogerboy21 woogerboy21 merged commit d039c9b into Cockatrice:master Nov 29, 2016
@woogerboy21 woogerboy21 deleted the client_misfeat_remember branch December 4, 2016 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App - Cockatrice Tickets relating to the cockatrice application
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants