Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/savegame-menu into lp:widelands

[GunChleoc]

@bunnybot merge force
-- 
https://code.launchpad.net/~widelands-dev/widelands/savegame-menu/+merge/322924
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/savegame-menu.

___
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/savegame-menu into lp:widelands

[GunChleoc]

@bunnybot merge force
-- 
https://code.launchpad.net/~widelands-dev/widelands/savegame-menu/+merge/322924
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/savegame-menu.

___
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/savegame-menu into lp:widelands

[GunChleoc]

@bunnybot merge
-- 
https://code.launchpad.net/~widelands-dev/widelands/savegame-menu/+merge/322924
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/savegame-menu.

___
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/savegame-menu into lp:widelands

[GunChleoc]

Thanks for pointing that out - fixed by adding a dummy file.
-- 
https://code.launchpad.net/~widelands-dev/widelands/savegame-menu/+merge/322924
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/savegame-menu.

___
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/savegame-menu into lp:widelands

[Notabilis]

If I interpret the Travis output right, it is complaining about the cmake 
library liblogic_constants. Wasn't there something that header-only libraries 
do not work? Might be the problem here, it only contains widelands.h.
-- 
https://code.launchpad.net/~widelands-dev/widelands/savegame-menu/+merge/322924
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/savegame-menu.

___
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/savegame-menu into lp:widelands

[Notabilis]

Review: Approve commit diff and short test

Code looks good and compiling & starting games works. As far as I am concerned, 
this branch can be merged.

I guess filesystem_constants is a good enough name for the file. After all, 
that is what it contains.
-- 
https://code.launchpad.net/~widelands-dev/widelands/savegame-menu/+merge/322924
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/savegame-menu.

___
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/savegame-menu into lp:widelands

[GunChleoc]

OK, everything should be addressed now.

I renamed logic/constants.h to logic/filesystem_constants.h, because I couldn't 
think of a better place to put them. I moved the statistics sample time 
constant out into game.h.
-- 
https://code.launchpad.net/~widelands-dev/widelands/savegame-menu/+merge/322924
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/savegame-menu.

___
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/savegame-menu into lp:widelands

[GunChleoc]

We're trying to keep the base directory as free as possible. How about putting 
them in src/io, since it's all filesystem stuff? Not entirely happy with that 
though, since it's all widelands-specific, and io is widelands-agnostic.

kStatisticsSampleTime could be moved to logic/game.h, since it's used both by 
logic and wui and has nothing to do with the I/O.
-- 
https://code.launchpad.net/~widelands-dev/widelands/savegame-menu/+merge/322924
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/savegame-menu.

___
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/savegame-menu into lp:widelands

[Notabilis]

Review: Needs Fixing

I only looked at the commit-diffs, but the changes are mostly fine with me, 
thanks.

However, there are two problems:
- Codecheck complains about wrong include order in logic/game.cc
- For me, it does not compile. Strangely, Travis seems to be happy with the 
code. The problem is the undefined variable mso in ai/defaultai_warfare.cc. It 
has been removed in commit 8154, is there a reason for it? After re-adding the 
line it compiles fine.

A quick test with my locally fixed version did not crash any longer in the 
menus and the issues from the review are done or have become TODOs. What is a 
bit strange: The preview-map of savegames and the additional game information 
are gone. Is this intentional?

Oh, and I am not able to start or load a game. It loads fine but the game 
crashes as soon as the game is displayed. I found the source in 
AiDnaHandler::fetch_dna() in logic/ai_dna_handler.cc. The path to the file is 
created by concatenating char pointers since a std::string() cast is missing 
due to the new char* constants. I haven't checked the other uses of the 
constants, maybe its happening there, too. Is it possible to make the constants 
std::strings to avoid this (possible/future) bug?

Regarding constants.h: Currently the file is in src/logic/ but also contains 
constants from, e.g. the ai. Maybe place it directly in src/ ? Related: in 
src/network/ there is also a constants.h containing the used network port 
numbers. Shall I move them to our global(?) constants file (I have to do this 
myself)?
-- 
https://code.launchpad.net/~widelands-dev/widelands/savegame-menu/+merge/322924
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/savegame-menu.

___
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/savegame-menu into lp:widelands

[GunChleoc]

Review: Resubmit

This is now ready again, the crash is gone.

I added the things I couldn't solve right now as TODO comments to the code - 
those are wider issues with the UI toolbox that are already in trunk.

I also cleaned up the file system constants, because it was really annoying me.

Thanks for the detailed testing and feedback!
-- 
https://code.launchpad.net/~widelands-dev/widelands/savegame-menu/+merge/322924
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/savegame-menu.

___
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/savegame-menu into lp:widelands

[GunChleoc]

Answered the diff comments, I'm still waiting for the compiler for the fixes. 
Will have a look at the bugs after that.

Diff comments:

> === modified file 'src/game_io/game_preload_packet.cc'
> --- src/game_io/game_preload_packet.cc2017-08-17 15:34:45 +
> +++ src/game_io/game_preload_packet.cc2017-10-02 17:28:52 +
> @@ -46,9 +46,11 @@
>  constexpr uint16_t kCurrentPacketVersion = 6;
>  constexpr const char* kMinimapFilename = "minimap.png";
>  
> +// Win condition localization can come from the 'widelands' or 
> 'win_conditions' textdomain.
>  std::string GamePreloadPacket::get_localized_win_condition() const {

No, moving this would mean that we need to add dead code to the win conditions 
lua files, which we might easily lose in the future if somebody assumes it is 
useless.

> + std::string result = _(win_condition_);
>   i18n::Textdomain td("win_conditions");
> - return _(win_condition_);
> + return _(result);
>  }
>  
>  void GamePreloadPacket::read(FileSystem& fs, Game&, MapObjectLoader* const) {
> 
> === modified file 'src/ui_basic/icon.cc'
> --- src/ui_basic/icon.cc  2017-04-22 12:19:21 +
> +++ src/ui_basic/icon.cc  2017-10-02 17:28:52 +
> @@ -54,16 +54,16 @@
>   if (pic_) {
>   const float scale = std::min(1.f, 
> std::min(static_cast(get_w()) / pic_->width(),
>  
> static_cast(get_h()) / pic_->height()));
> -
> - const float width = scale * get_w();
> - const float height = scale * get_h();
> - const float x = (get_w() - width) / 2.f;
> - const float y = (get_h() - height) / 2.f;
> + // We need to be pixel perfect, so we use ints.
> + const int width = scale * get_w();
> + const int height = scale * get_h();
> + const int x = (get_w() - width) / 2;
> + const int y = (get_h() - height) / 2;
>   dst.blitrect_scale(Rectf(x, y, width, height), pic_,
>  Recti(0, 0, pic_->width(), pic_->height()), 
> 1., BlendMode::UseAlpha);
> - }
> - if (draw_frame_) {
> - dst.draw_rect(Recti(0, 0, get_w(), get_h()), framecolor_);
> + if (draw_frame_) {
> + dst.draw_rect(Recti(x, y, width, height), framecolor_);

Will have a look

> + }
>   }
>  }
>  }
> 
> === modified file 'src/ui_basic/messagebox.cc'
> --- src/ui_basic/messagebox.cc2017-05-25 12:30:40 +
> +++ src/ui_basic/messagebox.cc2017-10-02 17:28:52 +
> @@ -146,12 +146,16 @@
>  }
>  
>  void WLMessageBox::clicked_ok() {
> + ok_button_->set_enabled(false);
> + cancel_button_->set_enabled(false);
>   ok();

Exactly. It also provides some visual feedback to the user in case it is slow.

>   if (is_modal())
>   end_modal(UI::Panel::Returncodes::kOk);
>  }
>  
>  void WLMessageBox::clicked_back() {
> + ok_button_->set_enabled(false);
> + cancel_button_->set_enabled(false);
>   cancel();
>   if (is_modal())
>   
> end_modal(UI::Panel::Returncodes::kBack);
> 
> === modified file 'src/ui_basic/multilinetextarea.cc'
> --- src/ui_basic/multilinetextarea.cc 2017-05-21 18:15:17 +
> +++ src/ui_basic/multilinetextarea.cc 2017-10-02 17:28:52 +
> @@ -51,15 +51,13 @@
>   scrollmode_(scroll_mode),
>   pic_background_(nullptr) {
>   assert(scrollmode_ == MultilineTextarea::ScrollMode::kNoScrolling || 
> Scrollbar::kSize <= w);
> + set_scrollmode(scroll_mode);

Good point, will change this.

>   set_thinks(false);
>  
>   
> scrollbar_.moved.connect(boost::bind(::scrollpos_changed, 
> this, _1));
>  
>   scrollbar_.set_singlestepsize(text_height());
>   scrollbar_.set_steps(1);
> - scrollbar_.set_force_draw(scrollmode_ == 
> ScrollMode::kScrollNormalForced ||
> -   scrollmode_ == ScrollMode::kScrollLogForced);
> -
>   layout();
>  }
>  
> 
> === modified file 'src/ui_fsmenu/loadgame.cc'
> --- src/ui_fsmenu/loadgame.cc 2017-06-24 10:38:19 +
> +++ src/ui_fsmenu/loadgame.cc 2017-10-02 17:28:52 +
> @@ -19,659 +19,121 @@
>  
>  #include "ui_fsmenu/loadgame.h"
>  
> -#include 
> -#include 
> -#include 
> -#include 
> -
> -#include 
> -#include 
> -
>  #include "base/i18n.h"
> -#include "base/log.h"
> -#include "base/time_string.h"
> -#include "game_io/game_loader.h"
> -#include "game_io/game_preload_packet.h"
> -#include "graphic/graphic.h"
> -#include "graphic/image_io.h"
> -#include "graphic/text_constants.h"
> -#include "graphic/texture.h"
> -#include "helper.h"
> -#include "io/filesystem/layered_filesystem.h"
> -#include "logic/game.h"
> -#include "logic/game_controller.h"
> -#include "logic/game_settings.h"
> -#include "logic/replay.h"
> -#include "ui_basic/icon.h"
> -#include "ui_basic/messagebox.h"
> -
> -// TODO(GunChleoc): Arabic: line height broken for descriptions for 

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/savegame-menu into lp:widelands

[Jukka Pakarinen]

I have been testing the branch and today got it crash.

This is how it crashes:

Single Player
New Game
Click Crater and then OK
Start game
Main Menu
Save Game
Write some Filename and then OK
Main Menu
Exit Game 
OK
Watch Replay
Choose the saved game and then OK
Wait until the end of replay and "End of repaly" message pops up.
Click OK
Game crashes, "Segmentation fault"




Here is few lines of what gdb says about it.



REPLAY: End of replay (gametime: 10434)

Thread 1 "widelands" received signal SIGSEGV, Segmentation fault.
0x5610c735 in UI::Button::set_enabled (this=0x0, on=false) at 
/usr/src/bzr/widelands-repo/savegame-menu/src/ui_basic/button.cc:128
128 if (enabled_ == on)
#0  0x5610c735 in UI::Button::set_enabled (this=0x0, on=false) at 
/usr/src/bzr/widelands-repo/savegame-menu/src/ui_basic/button.cc:128
#1  0x5611cdec in UI::WLMessageBox::clicked_ok (this=0x7fffa520) at 
/usr/src/bzr/widelands-repo/savegame-menu/src/ui_basic/messagebox.cc:150
#2  0x5611dc6f in boost::_mfi::mf0::operator() 
(this=0x5d6f5230, t=...) at /usr/include/boost/bind/mem_fn_template.hpp:70




I tested the same with trunk and it didn't crash.

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

___
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/savegame-menu into lp:widelands

[GunChleoc]

Thanks for the detailed review :)

This will need some TLC from a real machine, so I'll take care of this one when 
I'm back home.
-- 
https://code.launchpad.net/~widelands-dev/widelands/savegame-menu/+merge/322924
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/savegame-menu.

___
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/savegame-menu into lp:widelands

[Notabilis]

Review: Needs Fixing

I did a round of code diff review and a bit of testing. The code looks good to 
me, just some small remarks. I haven't retraced the whole refactoring process, 
but it seems to be okay.

However, there are some issues with the removal of save games:
- Just a suggestion: After removing a game, nothing is selected. Maybe select 
the next entry in the list?

- Bug: After removing a file, the right column still displays the information 
about the no longer existing save. Also the OK button is enabled but does 
nothing.

- Bug: When the removal dialog was open, navigation with arrow keys no longer 
works.

- Bug: When selecting many entries (around 30 for me in non-fullscreen mode) 
the "really remove" dialog box can become cut off. First, the entries are all 
listed normally which is fine. At some point the dialog box uses a scroll box 
which is fine, too. Between this cases there is some number of entries where 
the message window is too big to be drawn completely and parts are cut off at 
the top and bottom.

- A small optical comment: When having slightly less than the "use scroll box" 
number of entries selected, the window with the list is bigger than with the 
scrollbox. No need to fix this, though.

- The removal dialog displays the filename when a single file is selected but a 
list of "mapnames + saved-on-date" when multiple files are selected. The 
filename is not that useful I think, maybe change it to the name of the save? 
When I had multiple campaign saves selected, the list contained five times the 
same entry. Looked a bit strange but is no bug. For campaign save games the 
name of the save and the filename can both be quite long. So using the full 
name in the "multiple files selected" list is probably no good idea.


A feel-free-to-ignore comment regarding the minimap preview: From other games I 
am used to having the preview box always in the same size and scaling the image 
accordingly. Here the size of the box changes to match the image. That has the 
advantage of having a good image quality but is a bit "jumpy" when scrolling 
through the list.

Diff comments:

> === modified file 'src/game_io/game_preload_packet.cc'
> --- src/game_io/game_preload_packet.cc2017-08-17 15:34:45 +
> +++ src/game_io/game_preload_packet.cc2017-10-02 17:28:52 +
> @@ -46,9 +46,11 @@
>  constexpr uint16_t kCurrentPacketVersion = 6;
>  constexpr const char* kMinimapFilename = "minimap.png";
>  
> +// Win condition localization can come from the 'widelands' or 
> 'win_conditions' textdomain.
>  std::string GamePreloadPacket::get_localized_win_condition() const {

When I understand this method correctly, one of the two translation attempts 
will fail, right? Should work fine, I guess, but it does look a bit messy. It 
is possible to use only one of the textdomains (i.e. move the translations)?

> + std::string result = _(win_condition_);
>   i18n::Textdomain td("win_conditions");
> - return _(win_condition_);
> + return _(result);
>  }
>  
>  void GamePreloadPacket::read(FileSystem& fs, Game&, MapObjectLoader* const) {
> 
> === modified file 'src/ui_basic/icon.cc'
> --- src/ui_basic/icon.cc  2017-04-22 12:19:21 +
> +++ src/ui_basic/icon.cc  2017-10-02 17:28:52 +
> @@ -54,16 +54,16 @@
>   if (pic_) {
>   const float scale = std::min(1.f, 
> std::min(static_cast(get_w()) / pic_->width(),
>  
> static_cast(get_h()) / pic_->height()));
> -
> - const float width = scale * get_w();
> - const float height = scale * get_h();
> - const float x = (get_w() - width) / 2.f;
> - const float y = (get_h() - height) / 2.f;
> + // We need to be pixel perfect, so we use ints.
> + const int width = scale * get_w();
> + const int height = scale * get_h();
> + const int x = (get_w() - width) / 2;
> + const int y = (get_h() - height) / 2;
>   dst.blitrect_scale(Rectf(x, y, width, height), pic_,
>  Recti(0, 0, pic_->width(), pic_->height()), 
> 1., BlendMode::UseAlpha);
> - }
> - if (draw_frame_) {
> - dst.draw_rect(Recti(0, 0, get_w(), get_h()), framecolor_);
> + if (draw_frame_) {
> + dst.draw_rect(Recti(x, y, width, height), framecolor_);

I think this overdraws one line/column of pixels of the picture on each side.

> + }
>   }
>  }
>  }
> 
> === modified file 'src/ui_basic/messagebox.cc'
> --- src/ui_basic/messagebox.cc2017-05-25 12:30:40 +
> +++ src/ui_basic/messagebox.cc2017-10-02 17:28:52 +
> @@ -146,12 +146,16 @@
>  }
>  
>  void WLMessageBox::clicked_ok() {
> + ok_button_->set_enabled(false);
> + cancel_button_->set_enabled(false);
>   ok();

Fine by me, but what is the purpose of this? Avoiding potential bugs through 
double 

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/savegame-menu into lp:widelands

[kaputtnik]

Review: Approve testing

Opened all menus and all looks good to me :-)
-- 
https://code.launchpad.net/~widelands-dev/widelands/savegame-menu/+merge/322924
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/savegame-menu.

___
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/savegame-menu into lp:widelands

[kaputtnik]

Had a quick look and it LGTM :-)

Don't know if this related to your changes: 
- Fullscreen toggling does not work for the save game menu, as well for Editor 
Load/Save map menu. But for me this is ok.
- I have some incompatible maps showing an Error message. Hovering with the 
mouse over the errormessage shows the tooltip "The Map that this map is based 
on" of the map.
- The tooltip is only shown when hovering exactly over the first line. 
- In trunk there is also a tooltip for the Win Condition, which is missing here

General in regard to the tooltips when hovering "Map name" and "Win condition": 
I don't know if they are use full in the Load game menu, because they are 
telling the same as the texts already says. So they do not help in any way, but 
causes additional work for the translators, imho.
-- 
https://code.launchpad.net/~widelands-dev/widelands/savegame-menu/+merge/322924
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/savegame-menu.

___
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/savegame-menu into lp:widelands

[GunChleoc]

Review: Resubmit

I have implemented all your suggestions except for the colors, and it looks 
much better now :)

The color scheme is as follows:

Fullscreen menu text: Yellow, bold headers/variable names. Grey, bold 
paragraph-level texts. Due to the background images, all texts have to be bold 
in order to be legible (yuck).

In-game/in-editor text: Grey, bold header/variable names. Yellow 
paragraph-level texts.

Once the compiler finishes, I'll double-check that I haven't made any mistakes 
here.

If we change the color scheme in the savegame menu, then we also need to change 
it for all campaign, help and messages texts for consistency, and that's a 
clear -1 on my part.
-- 
https://code.launchpad.net/~widelands-dev/widelands/savegame-menu/+merge/322924
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/savegame-menu.

___
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/savegame-menu into lp:widelands

[kaputtnik]

> Colors are reversed between the Fullscreen menus and the in.game menus on
> purpose. This is necessary because the background makes non-bold text
> unreadable in the Fullscreen menus 

Can't follow you here. The Save game menu shows currently (in this branch) bold 
white constants ('Map name:') with non bold yellow variables (e.g. "Crater"). 
What is against having bold yellow constants with bold white variables like it 
is all other gameplay menus?

> - the coloring is consistent with the
> mapselect screen though.

You mean mapselect screen in Editor? I think this branch do not touch the menus 
in editor. The Choose map screen for single player game shows also bold yellow 
constants with bold white variables.

Maybe i am too fussy :)

> I used the arrow in the replay in an attempt to signal that the gametime is
> the start of the replay, not the end or length. 
Ok, never associate it with the beginning of the replay... maybe has to be 
clarified in the tooltip? Or, since the same information is also on the right 
side, describe it it on the right side. So instead: 'Game Time: 20:44' -> 
'Start of replay: 20:44'. One disadvantage when the game time is in the column: 
Clicking on the column header sort the entrys by gametime then. Don't know if 
gametime is a necessary information to sort for.

> Regarding the load/save
> screens, we have map descriptions in the load menu and actual filenames in the
> save menu. Do you thing that we need a button to toggle those like in the
> editor load/save map?
No, i don't think so. I had trouble to write what i meant. Another try:

The column description in Load save game menu contains for example:

1. The Strands of Malac'mor (Build blockhouse) -> Campaign(Objective)
2. test (Crater) -> filename (Mapname)
3. Autosave 00: Crater -> somename: Mapname

Especially the 2. and 3. example shows different formats of the same kind of 
information. Having an extra string for autosave files is ok for me, but the 
format should be equal in both entries, imho. To distinguish between campaigns 
both entries may be in format 'name: mapname'. The resulting entries would then 
be:

1. The Strands of Malac'mor (Build blockhouse)
2. test: Crater -> removed curved brackets
3. Autosave 00: Crater


> H means Host, we are distinguishing host from client here. MP is reserved for
> multiplayer client games. How about MPH/MPC?

I don't mind. For me all those abbreviations are not self explaining *lol*

> And yes, the savegame menu is consistent with the editor menus, not the
> Fullscreen menus. I can look into changing the button positions.

I think distinguishing between editor and playing menus, if feasible, would be 
good, since editor and playing are different kind of programs.

> I don't see a natural cutoff point to make the editbox shorter that would be
> visually pleasing - ideas?

In the old save game menu the editbox was visual a part of the table. I liked 
that...


-- 
https://code.launchpad.net/~widelands-dev/widelands/savegame-menu/+merge/322924
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/savegame-menu 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/savegame-menu into lp:widelands

[GunChleoc]

Colors are reversed between the Fullscreen menus and the in.game menus on 
purpose. This is necessary because the background makes non-bold text 
unreadable in the Fullscreen menus - the coloring is consistent with the 
mapselect screen though.

I used the arrow in the replay in an attempt to signal that the gametime is the 
start of the replay, not the end or length. Regarding the load/save screens, we 
have map descriptions in the load menu and actual filenames in the save menu. 
Do you thing that we need a button to toggle those like in the editor load/save 
map?

H means Host, we are distinguishing host from client here. MP is reserved for 
multiplayer client games. How about MPH/MPC?

And yes, the savegame menu is consistent with the editor menus, not the 
Fullscreen menus. I can look into changing the button positions.

I don't see a natural cutoff point to make the editbox shorter that would be 
visually pleasing - ideas?

-- 
https://code.launchpad.net/~widelands-dev/widelands/savegame-menu/+merge/322924
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/savegame-menu 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/savegame-menu into lp:widelands

[kaputtnik]

Sorry for confusing, i was much tired yesterday. I meant: '... white for 
variables':

In Load screen there is yellow for constants (like 'Map Name:') and white for 
variables.
In save screen this is vice versa.

Another inconsistency:

In 'Watch Replay' the column 'Description' contains an arrow, while in all 
other menus the Format is different:

Watch Replay: GameTime arrow NameOfMap
Load Save Game has already two different styles: Autosave 00 colon Name of Map, 
File name exclamation_mark_open Name of Map exclamation_mark_close
Save Game: wl_autosave_00 exclamation_mark_open Name of Map 
exclamation_mark_close

I think there should be one format for all of this menus. This would make it 
easier for a user to read the tables.

'Watch Replay' menu:
- The tooltip for column "Description" has to be adjusted
- In Column 'Mode' there is 'SP' for single player but 'H' for Multiplayer. 
While the 'H' is described in the Tooltip, i think if the 'H' could be replaced 
with 'MP' it would be much easier to guess the meaning.

'Load save Game' menu:
- The tooltip for column description says "The Filename the game was saved 
under ..." This is not true for autosaved games, because then the String 
"Autosave 00" is shown, but the Filename is wl_autosave_00.

'Save Game' menu:
- The editbox for the filename is very long
- This is now the only menu which breaks with the positions of buttons (except 
the menus in editor). Since all other Playing menus have the buttons on the 
right, this menu shows two of them 'suddenly' at the bottom.

I am not an UI expert, but i think a consistent coloring and positioning of 
elements would be good.
-- 
https://code.launchpad.net/~widelands-dev/widelands/savegame-menu/+merge/322924
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/savegame-menu 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/savegame-menu into lp:widelands

[kaputtnik]

I am struggling with the different coloring of texts:

In Load screen there is yellow for constants (like 'Map Name:') and yellow for 
variables. 
In save screen this is vice versa.

In general an improvement though :-)
-- 
https://code.launchpad.net/~widelands-dev/widelands/savegame-menu/+merge/322924
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/savegame-menu 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