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.

Sign up for GitHub

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

Jump to bottom

fix #944 click user game announcement link if game has ended #946

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

fix #944 click user game announcement link if game has ended #946

wants to merge 1 commit into from

Conversation

GrotheFAF
Copy link
Contributor

@GrotheFAF GrotheFAF commented Dec 31, 2017 •
edited
Loading

In Chat a click on player live game announcement when game had ended, let to fa starting with error message.
Now it will give a message about game ended, with an option to search the users replays in Vault.
And click on host game announcements will now be checked for game started (message with option to watch the live game) and game ended (message).

Wesmania
Wesmania requested changes Dec 31, 2017
View reviewed changes
src/chat/channel.py Outdated
@@ -199,7 +199,12 @@ def pingWindow(self):
def openUrl(self, url):
logger.debug("Clicked on URL: " + url.toString())
if url.scheme() == "faflive":
replay(url)
if replay(url) == "Replays": # game ended -> Search Replaya
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't accept this kind of hack. If we want to pull the player id from the url, let's not hack it directly out of the url string, this is fragile as hell. I guess QUrl is the only thing we have here, but making a dedicated class for parsing and constructing these out of player / game data would be much more robust.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when I added something to the QUrl (it was the game-uid) in the past you ripped my head of, so I worked here with what I got.
I would have no problem with spicing up the QUrl with 1-2 more attr.
(Keep in mind, this is a fix of a pretty old bug (not a refactor), which wasn't that noticeable with the rare friend host announcement - but will be once the friend announcement will come in.)

Copy link
Contributor

Choose a reason for hiding this comment

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

If I recall correctly, my issue was with the exact same thing - raw parsing of the url string. I'd really like to see a separate class for both building and parsing these game URLs.

src/chat/channel.py Outdated
# hack to get player name from url
player_id = int(url.path().split("/")[2].split(".")[0])
if player_id in self._chatterset._playerset._players:
player = self._chatterset._playerset._players[player_id].login
Copy link
Contributor

Choose a reason for hiding this comment

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

Holy shit, these two lines. Class members starting with an underscore are private, this should be respected. If we need a playerset in the class, pass it in the constructor. Playerset itself has dict-like semantics, there are plenty examples for it in existing code.

If we make a dedicated class for handling these URLs, then using playerset directly won't even be necessary.

src/fa/replay.py Outdated
@@ -87,7 +87,15 @@ def replay(source, detach=False):
if url.scheme() == "faflive":
mod = QtCore.QUrlQuery(url).queryItemValue("mod")
mapname = QtCore.QUrlQuery(url).queryItemValue("map")
replay_id = url.path().split("/")[0]
replay_id = url.path().split("/")[1]
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment about the url as above.

src/fa/replay.py Outdated
"Would you like to look for it in Replays?",
QtWidgets.QMessageBox.Yes,
QtWidgets.QMessageBox.No) == QtWidgets.QMessageBox.Yes:
return "Replays"
Copy link
Contributor

Choose a reason for hiding this comment

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

Return type doesn't match up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

aren't we flexible there?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not an excuse. This return type means something to the caller, adding something out of the blue is unacceptable.

src/fa/replay.py Outdated
replay_id = url.path().split("/")[0]
replay_id = url.path().split("/")[1]
if int(replay_id) not in client.instance.gameset:
if QtWidgets.QMessageBox.question(client.instance, "Live Game ended",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is blocking, but there's another example of it in this function already, so I guess it's good.

@GrotheFAF GrotheFAF force-pushed the fix-user-announcement-game-ended branch 2 times, most recently from e7f7fca to 06265cc Compare January 1, 2018 14:38
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 27.505% when pulling 06265cc on GrotheFAF:fix-user-announcement-game-ended into f6eb7b2 on FAForever:develop.

@GrotheFAF GrotheFAF force-pushed the fix-user-announcement-game-ended branch from 06265cc to 07c337e Compare January 1, 2018 15:39
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 27.505% when pulling 07c337e on GrotheFAF:fix-user-announcement-game-ended into 1263c12 on FAForever:develop.

@GrotheFAF GrotheFAF force-pushed the fix-user-announcement-game-ended branch from 07c337e to 2b57343 Compare January 1, 2018 18:29
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 27.505% when pulling 2b57343 on GrotheFAF:fix-user-announcement-game-ended into e34dd0b on FAForever:develop.

@GrotheFAF GrotheFAF force-pushed the fix-user-announcement-game-ended branch from 2b57343 to f372ce7 Compare January 2, 2018 20:31
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 27.498% when pulling f372ce7 on GrotheFAF:fix-user-announcement-game-ended into b1b600a on FAForever:develop.

@GrotheFAF GrotheFAF force-pushed the fix-user-announcement-game-ended branch from f372ce7 to 5447f62 Compare January 9, 2018 00:17
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 27.474% when pulling 5447f62 on GrotheFAF:fix-user-announcement-game-ended into b0f9e5d on FAForever:develop.

@GrotheFAF GrotheFAF force-pushed the fix-user-announcement-game-ended branch from 5447f62 to b01d5f5 Compare January 9, 2018 21:59
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 27.474% when pulling b01d5f5 on GrotheFAF:fix-user-announcement-game-ended into 99d772d on FAForever:develop.

@GrotheFAF GrotheFAF force-pushed the fix-user-announcement-game-ended branch from b01d5f5 to 3994188 Compare January 17, 2018 19:04
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 27.467% when pulling 3994188 on GrotheFAF:fix-user-announcement-game-ended into ef57cf4 on FAForever:develop.

@Wesmania Wesmania added this to the 0.17.0 backlog milestone Jan 17, 2018
@GrotheFAF GrotheFAF force-pushed the fix-user-announcement-game-ended branch from 3994188 to 568a2fd Compare January 17, 2018 21:13
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 27.467% when pulling 568a2fd on GrotheFAF:fix-user-announcement-game-ended into 2eb640c on FAForever:develop.

@GrotheFAF GrotheFAF force-pushed the fix-user-announcement-game-ended branch from 568a2fd to c3f305e Compare January 18, 2018 18:23
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 27.465% when pulling c3f305e on GrotheFAF:fix-user-announcement-game-ended into cd1b011 on FAForever:develop.

@GrotheFAF GrotheFAF force-pushed the fix-user-announcement-game-ended branch 2 times, most recently from 95582af to a2f6f4a Compare January 19, 2018 21:31
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 24.411% when pulling a2f6f4a on GrotheFAF:fix-user-announcement-game-ended into 1afb1f0 on FAForever:develop.

@GrotheFAF
fix FAForever#944 click user game announcement link if game has ended
fd177f3 
click on player live game announcement when game had ended, let to fa starting with error message.
Now it will give a message about game ended, with an option to search users replays in Vault
click on host/join game announcent when the game has already started, now
will gives a message with an option to watch the live game
@GrotheFAF GrotheFAF force-pushed the fix-user-announcement-game-ended branch from a2f6f4a to fd177f3 Compare January 20, 2018 18:23
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 23.152% when pulling fd177f3 on GrotheFAF:fix-user-announcement-game-ended into b90bffa on FAForever:develop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wesmania Wesmania Wesmania requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.17.0 backlog
Development

Successfully merging this pull request may close these issues.
3 participants
@GrotheFAF @coveralls @Wesmania