widelands-dev team mailing list archive

[Notabilis <notabilis27@xxxxxx>]

Review: Approve

Code is looking good and the checkbox is working as advertised.

I am a bit unsure about having it on by default. On 800x600 it makes the list pretty useless, since only date+time+sp/mp of the filename is shown but no information about the map, etc. On high(er) resolutions its fine, though. Just an idea: Maybe append the filename to the line instead of prepending it?
On high resolutions I would prefer an additional column for the filename but having different GUIs based on the resolution doesn't sound like a good idea.

Some related feature requests:
- Add the filename to the information about selected replay (right half of screen)
- Maybe a similar checkbox for the single player "Load game" screen
- Store state of checkbox in config file

Diff comments:

> 
> === modified file 'src/ui_fsmenu/loadgame.cc'
> --- src/ui_fsmenu/loadgame.cc	2018-05-13 07:15:39 +0000
> +++ src/ui_fsmenu/loadgame.cc	2018-05-16 05:04:34 +0000
> @@ -107,6 +117,23 @@
>  	   main_box_.get_w() - tablew_ - right_column_margin_, tableh_);
>  }
>  
> +void FullscreenMenuLoadGame::toggle_filenames() {
> +	showing_filenames_ = show_filenames_->get_state();
> +
> +	// Remember selection
> +	const std::set<uint32_t> selected = load_or_save_.table().selections();
> +	// Fill table again
> +	fill_table();
> +
> +	// Restore selection items
> +	// TODO(GunChleoc): It would be nice to have a function to just change the entry texts
> +	for (const uint32_t selectme : selected) {
> +		load_or_save_.table().multiselect(selectme, true);

Maybe add a method to set the selection to a set?

> +	}
> +	entry_selected();
> +}
> +
> +
>  void FullscreenMenuLoadGame::clicked_ok() {
>  	if (load_or_save_.table().selections().size() != 1) {
>  		return;


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1733280-replay_filenames/+merge/345566
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1733280-replay_filenames.