Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/empire into lp:widelands
No problem. Thanks for the review - things can't be proofread too often :) -- https://code.launchpad.net/~widelands-dev/widelands/empire/+merge/257141 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/empire. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/find_attack_soldiers into lp:widelands
Can you create a bug report for this, so we can take care of it in a separate branch? I have worked with signals before on the UI and might be able to figure it out, so I would put it on my todo-list. -- https://code.launchpad.net/~widelands-dev/widelands/find_attack_soldiers/+merge/245276 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/find_attack_soldiers. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1390793 into lp:widelands
Why don't you try having a look at the code? I don't understand all the GL stuff SirVer is doing either, but I review it anyway, because there is no-one else to do it. Granted, this branch might be a bit too big to start doing your first code review. 1. The seafaring checkbox is for the filter checkbox when players want to select a map to start a new game. Analyzing all the maps there would be to time consuming. So, this is part of the "preload" packet that lets players see information about the map without loading the whole map. 2. I am using the same element for starting new games - I think editors should be able to figure this out? The tooltip is only there to give the player the English or translated name of the map, and only makes a difference if you don't choose English as your interface language. 3. I don't think I have the space on small screens - e.g. set your Widelands to 800x600 resolution, then go to Single Player -> New Game and pick Trident of Fire. The map details used here are the same as in the editor, to cut down on code repetition. 4. I agree, I am open to suggestions :) 5 - 6. Yes, out of scope. Can you open a new bug please? I am chasing deadlines this week. 7. Without the list of maps, you won't be able to pick a directory. I am against displaying map details for anything that you pick on the list, because as you say yourself, the map options for the map to be saved it the information needed here. What would be the use of displaying the file name twice? I'm not sure I understand what you're getting at here :( The filename edit field is pre-filled with the last filename that the map was saved under, so it's the current filename anyway. If it's a new map, it picks "No Name" as a filename. 8. Good idea. I was thinking "Type a new name, or select a map from the list to overwrite an existing map file." This isn't entirely correct though - if this map has been saved before, it will be overwritten (unless the user changes the contents of the edit box) without the need to select something from the list. My brain is mush at the moment, so I'm open to suggestions :) -- https://code.launchpad.net/~widelands-dev/widelands/bug-1390793/+merge/254558 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1390793 into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/find_attack_soldiers into lp:widelands
I will create a bug report as soon as this branch is merged. -- https://code.launchpad.net/~widelands-dev/widelands/find_attack_soldiers/+merge/245276 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/find_attack_soldiers. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~hjd/widelands/codecheck-and-deduplication into lp:widelands
Hans Joachim Desserud has proposed merging
lp:~hjd/widelands/codecheck-and-deduplication into lp:widelands.
Requested reviews:
Widelands Developers (widelands-dev)
For more details, see:
https://code.launchpad.net/~hjd/widelands/codecheck-and-deduplication/+merge/257464
I started looking at a whitespace codecheck issue, but noticed some duplicated
code nearby and ended up fixing that as well.
The first section in each case of this if was identical, so I restructured it
to place the conditional check inside the loop and always run the common code
before checking sparam1 more thoroughly.
Could arguably be refactored further into its own method if that makes the code
more readable.
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~hjd/widelands/codecheck-and-deduplication into lp:widelands.
=== modified file 'src/logic/worker.cc'
--- src/logic/worker.cc 2015-04-22 19:13:03 +
+++ src/logic/worker.cc 2015-04-25 17:39:47 +
@@ -396,22 +396,21 @@
Area area (map.get_fcoords(get_position()), 0);
bool found_reserved = false;
- if (action.sparam1 == "immovable") {
-
- for (;; ++area.radius) {
- if (action.iparam1 < area.radius) {
-send_signal(game, "fail"); // no object found, cannot run program
-pop_task(game);
-if (upcast(ProductionSite, productionsite, get_location(game))) {
- if (!found_reserved) {
- productionsite->notify_player(game, 30);
- }
- else {
- productionsite->unnotify_player();
- }
-}
-return true;
+ for (;; ++area.radius) {
+ if (action.iparam1 < area.radius) {
+ send_signal(game, "fail"); // no object found, cannot run program
+ pop_task(game);
+ if (upcast(ProductionSite, productionsite, get_location(game))) {
+if (!found_reserved) {
+ productionsite->notify_player(game, 30);
+}
+else {
+ productionsite->unnotify_player();
+}
}
+ return true;
+ }
+ if (action.sparam1 == "immovable") {
std::vector list;
if (action.iparam2 < 0)
map.find_reachable_immovables
@@ -443,25 +442,9 @@
(game, state, list[game.logic_rand() % list.size()].object);
break;
}
- }
- } else {
- for (;; ++area.radius) {
- if (action.iparam1 < area.radius) {
-send_signal(game, "fail"); // no object found, cannot run program
-pop_task(game);
-if (upcast(ProductionSite, productionsite, get_location(game))){
- if (!found_reserved) {
- productionsite->notify_player(game, 30);
- }
- else {
- productionsite->unnotify_player();
- }
-}
-return true;
- }
- else {
- if ( upcast(ProductionSite, productionsite, get_location(game)))
-productionsite->unnotify_player();
+ } else {
+ if (upcast(ProductionSite, productionsite, get_location(game))) {
+productionsite->unnotify_player();
}
std::vector list;
if (action.iparam2 < 0)
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
