Skip to content

Conversation

@gajop
Copy link
Collaborator

@gajop gajop commented Jul 14, 2019

Leaving this up until it's ready to be merged.

silentwings and others added 30 commits February 13, 2019 12:59
email verification in REGISTER/CONFIRMAGREEMENT
(they don't do anything anymore!)
& remove NOCHANNELTOPIC (no longer used by Spring and afaict not used by ZK)
updates for compat flags / stop sending dead compat flags
* Added gameConfig for TAPrime
various lobby protocol fixes
load game config in one place and check for some necessary tables (should probably add more...)
(try to fail early whenever possible)
added custom game lobbymusic option
Ruwetuin and others added 8 commits July 18, 2019 23:09
@gajop
Copy link
Collaborator Author

gajop commented Jul 19, 2019

@GoogleFrog Please check if it can be merged now.

"sounds/lobbyMusic/Tomorrow Landscape.ogg",
}
local randomTrackList
local OPEN_TRACK_NAME

Choose a reason for hiding this comment

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

I am going to change the name of OPEN_TRACK_NAME because it is not what I would call a constant. You can do this before it is pulled if you like.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, I'll fix

link_maps = link_maps,
link_particularMapPage = link_particularMapPage,
ignoreServerVersion = false,
openTrack = 'sounds/lobbyMusic/The Secret of Ayers Rock.ogg',
Copy link

@GoogleFrog GoogleFrog Jul 20, 2019

Choose a reason for hiding this comment

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

Bad whitespace. My rule is that tabs should only ever occur after newlines or tabs.

@Ruwetuin
Copy link

Ruwetuin commented Jul 20, 2019 via email


self.loadLocalWidgets = false
self.displayBots = false
self.displayBadEngines2 = true

Choose a reason for hiding this comment

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

I want this to be true because wrapper launch can launch into other engine versions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure I want to touch this because it was changed to fix an issue: Spring-Chobby@3627fe8
Was this change wrong?


Spring.SetConfigInt("WindowBorderless", (mode == 4 and 1) or 0, false)

Spring.SetConfigInt("Fullscreen", 0, false)

Choose a reason for hiding this comment

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

Need a tab here.

@GoogleFrog
Copy link

I'm a the point where I am just noting what I can fix after merging. @gajop said he will fix something so I will wait for his input.

@gajop
Copy link
Collaborator Author

gajop commented Jul 20, 2019

@GoogleFrog feel free to go ahead with the merge. I'd rather deal with smaller PRs after this gets in, would prefer to avoid any new conflicts.

if OPEN_TRACK_NAME == nil or OPEN_TRACK_NAME == '' then
OPEN_TRACK_NAME = randomTrackList[math.random(#randomTrackList)]
openTrack = WG.Chobby.Configuration.gameConfig.openTrack
if openTrack ~= nil then

Choose a reason for hiding this comment

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

I don't know what this nested if is meant to do, but there has to be a mistake here.

Choose a reason for hiding this comment

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

Why is open track being checked against ''? It looks like the meaning of configuration values is being overridden in a confusing way.

  • openTrack should be a string. If it is set then that track is played when the menu opens.
  • If openTrack is not set then a random track should be played.
  • If someone wants the ability to start the menu with no music they can add a config value to do that.
  • If the configuration has openTrack = '' then the music widget should print a warning about that track not existing.

end
end
if OPEN_TRACK_NAME == nil or OPEN_TRACK_NAME == '' then
if openTrack == nil or openTrack == '' then

Choose a reason for hiding this comment

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

Why can't this widget run if there is no open track? It would just mean there is silence until the first game is over (at which point a track will play).

Choose a reason for hiding this comment

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

Shouldn't this be

`if not openTrack then'

disablePlanetwars = true, -- removes settings related to planetwars
disableMatchMaking = true, -- removes match making
disableCommunityWindow = true, -- removes Community Window
openTrack = '',

Choose a reason for hiding this comment

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

This is bad.

@GoogleFrog
Copy link

It turns out I can commit changes.

@gajop
Copy link
Collaborator Author

gajop commented Jul 20, 2019

That's not an improvement though.
The empty string check might not be a good idea, but using not when you really mean to do a nil check is wrong and gives poor info to the reader.
not X should only be used with bools

@GoogleFrog
Copy link

Ok.

@GoogleFrog GoogleFrog merged commit 7847630 into ZeroK-RTS:master Jul 20, 2019
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.

6 participants