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

Serverlist: Add ping indicators #5164

Merged
merged 1 commit into from Feb 3, 2017

Conversation

@kilbith
Copy link
Contributor

commented Feb 2, 2017

2017-02-02-232107_1366x768_scrot

@SmallJoker

This comment has been minimized.

Copy link
Member

commented Feb 2, 2017

Very nice. I would prefer having it with two textures: Shadow and overlay.
Like it was done in the furnace, MTG.
👍

@lisacvuk

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2017

Maybe also allow user to set it to display ping in ms, rather than an icon?

@@ -577,6 +577,13 @@ int ModApiMainMenu::l_get_favorites(lua_State *L)
lua_settable(L, top_lvl2);
}

if (servers[i]["ping"].asString().size()) {

This comment has been minimized.

Copy link
@nerzhul

nerzhul Feb 2, 2017

Member

please store the string before condition and use !str.empty() and re-use the stored string into function instead of converting json value twice

@sfan5

This comment has been minimized.

Copy link
Member

commented Feb 2, 2017

hm
I think it would look nicer if the ping indicators were left of the player count, can you try that?

std::string str_ping = servers[i]["ping"].asString();
if (!str_ping.empty()) {
lua_pushstring(L, "ping");
lua_pushstring(L, str_ping.c_str());

This comment has been minimized.

Copy link
@sfan5

sfan5 Feb 2, 2017

Member

why not convert it to a number?

elseif ping <= 250 then
details = details .. "4,"
elseif ping <= 999 then
details = details .. "5,"

This comment has been minimized.

Copy link
@sfan5

sfan5 Feb 2, 2017

Member

so nothing is displayed if the ping is > 999?

@kilbith

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2017

Addressed @sfan5 comments. Preview updated.

details = details .. "3,"
elseif ping <= 250 then
details = details .. "4,"
elseif ping <= 500 or ping > 500 then

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Feb 2, 2017

Member

can just use else here

@nerzhul
nerzhul approved these changes Feb 3, 2017
@nerzhul

This comment has been minimized.

Copy link
Member

commented Feb 3, 2017

okay for me

@nerzhul nerzhul added the One approval label Feb 3, 2017
@@ -577,6 +577,13 @@ int ModApiMainMenu::l_get_favorites(lua_State *L)
lua_settable(L, top_lvl2);
}

float ping = servers[i]["ping"].asFloat();
if (ping) {

This comment has been minimized.

Copy link
@sfan5

sfan5 Feb 3, 2017

Member

this if seems incorrect to me
what does asFloat() return if the element doesn't exist or isn't a float?

This comment has been minimized.

Copy link
@nerzhul

nerzhul Feb 3, 2017

Member

the if should be servers[i].hasMember("ping")

This comment has been minimized.

Copy link
@kilbith

kilbith Feb 3, 2017

Author Contributor

I wasn't sure about it. According to my readings, the variable will be uninitialized since it's a local variable. Correct me if I'm wrong.

This comment has been minimized.

Copy link
@Zeno-

Zeno- Feb 3, 2017

Contributor

asFloat() needs fixing, yes. That function (asFloat()) should not drop off the end of the function without a return IMO

This comment has been minimized.

Copy link
@kilbith

kilbith Feb 3, 2017

Author Contributor

So it's isMember(), not hasMember(). Fixed.

@sfan5
sfan5 approved these changes Feb 3, 2017
@sfan5 sfan5 added >= Two approvals and removed One approval labels Feb 3, 2017
@Zeno-

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2017

👍

@Zeno- Zeno- merged commit 03b34cb into minetest:master Feb 3, 2017
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2017

Nice!

Ekdohibs added a commit to Ekdohibs/minetest that referenced this pull request Feb 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.