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

Addon client: close connection if the player cancels the download #2351

Merged
merged 3 commits into from
Jan 13, 2018

Conversation

jyrkive
Copy link
Member

@jyrkive jyrkive commented Jan 2, 2018

This pull request is a fix for bug #2203.

The approach is to close the connection when the player cancels the download. This is the same thing that Wesnoth was doing before the GUI2 port of the addon manager, and it has zero overhead if the player doesn't cancel the download (unlike some other options which were suggested).

This ensures that the game won't interpret the rest of the addon as a
response for the client's next request.

Fixes #2203.
@loonycyborg
Copy link
Member

loonycyborg commented Jan 4, 2018

Tested it, work for me in general. Though there is one issue: after you cancel a download it shows "connecting to server" window, which contains a cancel button. Pressing it doesn't seem to have a particular effect at this point other than freezing ui until connection is re-established. Will it hang indefinitely if there is a dc? User is likely to cancel download if connection between him and addon server is lost for whatever reason. Would be cool if in this case we didn't hang the dialog if user cancels reconnection, that is need to ensure that user can escape to main menu if there is a dc.

@GregoryLundberg
Copy link
Contributor

I would test this but the client crashes before I can upload my first addon.

It seems I must have addons uploaded in order to upload more, and I don't see how to hand-create the required pre-existing addon to get the ball rolling. sigh

@jyrkive
Copy link
Member Author

jyrkive commented Jan 5, 2018

@loonycyborg The latest commit restores ability to cancel connecting to the server.

Only happened if the player used the buttons in the addon list: the buttons
in the right panel worked fine. The problem was that the addon list caught
the addons_client::user_exit exception separately and didn't rethrow it
when necessary, unlike the addon manager itself.

I decided to simply throw a different exception instead.
@jyrkive
Copy link
Member Author

jyrkive commented Jan 13, 2018

I have fixed the addon manager not closing on cancel if the player initiated the download from the addon list. The game freezing on cancel on GNU/Linux isn't a regression relative to 1.12 and therefore doesn't block this PR from being merged. No one raised any other concerns. Hence, merging.

@jyrkive jyrkive merged commit 8731930 into master Jan 13, 2018
@jyrkive jyrkive deleted the close-connection-on-cancel branch January 13, 2018 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants