widelands-dev team mailing list archive

[Notabilis <notabilis27@xxxxxx>]

Review: Approve diff, testing

Code is looking good and it seems to work as intended. A few possible improvements are in the diff.

I assume it is intentional that there is no message on game start?
In the commit/merge message of this branch you could add that wood gnome is also affected.

Diff comments:

> === modified file 'data/scripting/win_conditions/collectors.lua'
> --- data/scripting/win_conditions/collectors.lua	2018-10-13 08:51:51 +0000
> +++ data/scripting/win_conditions/collectors.lua	2019-01-02 13:47:40 +0000
> @@ -28,7 +28,11 @@
>  
>     -- set the objective with the game type for all players
>     broadcast_objective("win_condition", wc_descname, wc_desc)
> -
> +   
> +   -- set the maximum game time of 4 hours
> +   local max_time = 4 * 60
> +   
> +   local game = wl.Game()
>     local plrs = wl.Game().players

Suggestion: Use new "game" variable

>     local teams = {}
>     for idx,plr in ipairs(plrs) do
> @@ -221,10 +199,14 @@
>              lost_or_won = 1
>              player:send_message(won_game_over.title, won_game_over.body)
>           end
> -         if (player.team == 0) then
> -            wl.game.report_result(player, lost_or_won, make_extra_data(player, wc_descname, wc_version, {score=info[2]}))
> +         if (count_factions(plrs) > 1) then

Suggestion: Call count_factions() only once before the loop?

> +            if (player.team == 0) then
> +               wl.game.report_result(player, lost_or_won, make_extra_data(player, wc_descname, wc_version, {score=info[2]}))
> +            else
> +               wl.game.report_result(player, lost_or_won, make_extra_data(player, wc_descname, wc_version, {score=info[3], team_score=info[2]}))
> +            end
>           else
> -            wl.game.report_result(player, lost_or_won, make_extra_data(player, wc_descname, wc_version, {score=info[3], team_score=info[2]}))
> +            wl.game.report_result(player, lost_or_won)
>           end
>        end
>     end
> 
> === modified file 'data/scripting/win_conditions/win_condition_functions.lua'
> --- data/scripting/win_conditions/win_condition_functions.lua	2018-09-28 17:20:32 +0000
> +++ data/scripting/win_conditions/win_condition_functions.lua	2019-01-02 13:47:40 +0000
> @@ -237,3 +237,59 @@
>     table.sort(ranked_players_and_teams, function(a,b) return a["points"] > b["points"] end)
>     return ranked_players_and_teams
>  end
> +
> +-- RST
> +-- .. function:: format_remaining_time(remaining_time)
> +--
> +--    return a message that contains the remaining game time
> +--    to be used when sending status meassages
> +--
> +--    :arg remaining_time:    The remaining game time in minutes
> +function format_remaining_time(remaining_time)
> +   local h = 0
> +   local m = 60
> +   local time = ""
> +   set_textdomain("win_conditions")
> +   
> +   if (remaining_time ~= 60) then
> +      h = math.floor(remaining_time / 60)
> +      m = remaining_time % 60
> +   end
> +
> +   if ((h > 0) and (m > 0)) then
> +      -- TRANSLATORS: Context: 'The game will end in 2 hours and 30 minutes.'
> +      time = (ngettext("%1% hour and %2% minutes", "%1% hours and %2% minutes", h, m)):bformat(h, m)
> +   elseif m > 0 then
> +      -- TRANSLATORS: Context: 'The game will end in 30 minutes.'
> +      time = (ngettext("%i minute", "%i minutes", m)):format(m)
> +   else
> +      -- TRANSLATORS: Context: 'The game will end in 2 hours.'
> +      time = (ngettext("%1% hour", "%1% hours", h)):bformat(h)
> +   end
> +   -- TRANSLATORS: Context: 'The game will end in (2 hours and) 30 minutes.'
> +   return p(_"The game will end in %s."):format(time)
> +end
> +
> +-- RST
> +-- .. function:: notification_remaining_time(max_time)
> +--
> +--    calculate the remaining game time for notifications 
> +--    returns the remaining time and whether the notification should popup
> +--    to be used when sending status meassages

Typo "messages"

> +--    status messages are to be send every 30 minutes and every 5 during the last 30 minutes
> +--    the message window pops up ever hour 30, 20 & 10 minutes berfore the game ends.

If I understand it right this method should only be called within coroutines and is blocking them. Add a commend about that to the documentation? Currently it reads as if this method only calculates something.

> +--
> +--    :arg max_time:    The time maximum game time in minutes
> +function notification_remaining_time(max_time, remaining_time)
> +   local show_popup = false
> +   if (wl.Game().time < ((max_time - 30) * 60 * 1000)) then --
> +      wake_me(wl.Game().time + (30 * 60 * 1000)) -- 30 minutes
> +      remaining_time = remaining_time - 30
> +      if (remaining_time % 60 == 0) or (remaining_time == 30) then show_popup = true end
> +   else
> +      wake_me(wl.Game().time + (300 * 1000)) --5 Minutes
> +      remaining_time = remaining_time - 5
> +      if ((remaining_time ~= 0) and (remaining_time % 10 == 0)) then show_popup = true end
> +   end
> +   return remaining_time, show_popup
> +end
> \ No newline at end of file


-- 
https://code.launchpad.net/~widelands-dev/widelands/collectors_notification/+merge/361334
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/collectors_notification.