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

Replays - Install missing map #13233

Merged
merged 3 commits into from May 14, 2017

Conversation

Projects
None yet
6 participants
@rob-v
Contributor

rob-v commented May 3, 2017

Replay's Map preview unification with Lobby map preview - Install map, map author, map title tooltip, ...

obrazok

Closes #11149.

@pchote

The overall approach here looks good, and naturally extends towards our wishlist goal of showing players and spawns in the server list map previews. I haven't looked at the replay part of this in any detail yet, but here's some issues with the map preview logic to start with.

Please also split this into separate commits for refactoring (changing LobbyMapPreviewLogic to work from additional places) vs new features (making the ReplayBrowserLogic use it). This makes it much easier to understand changes when looking back through the git history.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v May 6, 2017

Contributor

Updated.

Contributor

rob-v commented May 6, 2017

Updated.

@chrisforbes

Minor nits, but I like it.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v May 13, 2017

Contributor

Updated - property Map replaced by field.

Contributor

rob-v commented May 13, 2017

Updated - property Map replaced by field.

@pchote

pchote approved these changes May 14, 2017

Looks good, thanks for taking this feature on. 👍 after a few small tweaks:

Show outdated Hide outdated OpenRA.Mods.Common/Widgets/Logic/Lobby/MapPreviewLogic.cs
Show outdated Hide outdated OpenRA.Mods.Common/Widgets/Logic/Lobby/MapPreviewLogic.cs
Show outdated Hide outdated OpenRA.Mods.Common/Widgets/Logic/Lobby/MapPreviewLogic.cs
statusSearching.IsVisible = () => getMap().Status == MapStatus.Searching;
var statusUnavailable = progress.GetOrNull("MAP_STATUS_UNAVAILABLE");
if (statusUnavailable != null)

This comment has been minimized.

@pchote

pchote May 14, 2017

Member

Style nit: put braces around hanging blocks.

@pchote

pchote May 14, 2017

Member

Style nit: put braces around hanging blocks.

Show outdated Hide outdated OpenRA.Mods.Common/Widgets/Logic/ReplayBrowserLogic.cs
Show outdated Hide outdated OpenRA.Mods.Common/Widgets/Logic/ReplayBrowserLogic.cs
@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v May 14, 2017

Contributor

Updated.

Contributor

rob-v commented May 14, 2017

Updated.

@pchote pchote merged commit 127ef8b into OpenRA:bleed May 14, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@LipkeGu

This comment has been minimized.

Show comment
Hide comment
@LipkeGu

LipkeGu May 17, 2017

Member

Can anyone agree that the PR is getting compiled .... i get errors like "local variable map is already define and gets overwritten"

Member

LipkeGu commented May 17, 2017

Can anyone agree that the PR is getting compiled .... i get errors like "local variable map is already define and gets overwritten"

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 17, 2017

Contributor

i get errors like "local variable map is already define and gets overwritten"

Same here, the last commit is to blame. Needs to be reverted or fixed.

Contributor

reaperrr commented May 17, 2017

i get errors like "local variable map is already define and gets overwritten"

Same here, the last commit is to blame. Needs to be reverted or fixed.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v May 17, 2017

Contributor

The PR before merge was built succesfully (also locally). There is simple fix to avoid this 'warning as error'. Could you please review quickly #13071? It by chance removes the local 'map' argument so solves this issue.

Contributor

rob-v commented May 17, 2017

The PR before merge was built succesfully (also locally). There is simple fix to avoid this 'warning as error'. Could you please review quickly #13071? It by chance removes the local 'map' argument so solves this issue.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 17, 2017

Contributor

#13071 reduces the errors, but lines 387 and 401 in LobbyLogic still have that issue there.

Contributor

reaperrr commented May 17, 2017

#13071 reduces the errors, but lines 387 and 401 in LobbyLogic still have that issue there.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote May 17, 2017

Member

This should be fixed as its own PR, separate to adding any new features.

Member

pchote commented May 17, 2017

This should be fixed as its own PR, separate to adding any new features.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v May 17, 2017

Contributor

#13071 Updated - map parameter renamed, compiled now with the same MSBuild, so should be fine now.

Contributor

rob-v commented May 17, 2017

#13071 Updated - map parameter renamed, compiled now with the same MSBuild, so should be fine now.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v May 17, 2017

Contributor

Fixed by #13313.

Contributor

rob-v commented May 17, 2017

Fixed by #13313.

@rob-v rob-v deleted the rob-v:ReplayInstallMap branch May 18, 2017

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