[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1733280-replay_filenames into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/bug-1733280-replay_filenames into lp:widelands has been updated. Status: Needs review => Merged For more details, see: 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. ___ 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-1733280-replay_filenames into lp:widelands
Thanks, looking good :) Transient failure on Travis @bunnybot merge force -- 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. ___ 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:~widelands-dev/widelands/bug-1733280-replay_filenames into lp:widelands
Continuous integration builds have changed state: Travis build 3596. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/389372550. Appveyor build 3399. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1733280_replay_filenames-3399. -- 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. ___ 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-1733280-replay_filenames into lp:widelands
No problem, is done. -- 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. ___ 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-1733280-replay_filenames into lp:widelands
Trunk is green again. Can you please merge so that I can get a new AppVeyor build for testing? Thanks! -- 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. ___ 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:~widelands-dev/widelands/bug-1733280-replay_filenames into lp:widelands
Continuous integration builds have changed state: Travis build 3580. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/387949166. Appveyor build 3383. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1733280_replay_filenames-3383. -- 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. ___ 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-1733280-replay_filenames into lp:widelands
Okay, is done. The first directory in the filename is removed now. Bye the way: The filename is also displayed in load and save game dialogs. -- 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. ___ 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-1733280-replay_filenames into lp:widelands
Almost - the filenames shown on the right still have the "replay/" directory prefix in them. Since all repays are in the same directory, we should remove that. -- 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. ___ 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:~widelands-dev/widelands/bug-1733280-replay_filenames into lp:widelands
Continuous integration builds have changed state: Travis build 3571. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/386730286. Appveyor build 3374. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1733280_replay_filenames-3374. -- 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. ___ 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-1733280-replay_filenames into lp:widelands
Okay, hope you are perfectly happy now. ;) Feel free to review, test and merge. -- 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. ___ 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-1733280-replay_filenames into lp:widelands
Both would be prefect, but I'd be happy with jus the checkbox state for now. -- 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. ___ 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-1733280-replay_filenames into lp:widelands
Just one checkbox state is what I would prefer as well. If you want, I can add this functionality in the next days to this branch. What is to be done? Only storing the state or also "Add the filename to the information about selected replay" ? -- 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. ___ 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-1733280-replay_filenames into lp:widelands
I think it might be even better to hide the functionality in fullscreen mode? UI elements can be set to invisible. There is a GraphicResolutionChanged notification that we're already reacting to for relayouting the screen, so that can be used here as well. I'm not sure if that isn't confusing to the user, to have content changed based on resolution switch. Better leave the handling of the checkbox to the user. So, store only 1 checkbox state and use it at all times. I'd like to do the tweaks in a follow-up bug to get this functionality in and not have to wait until August - unless somebody else wants to pick up this branch right now? -- 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. ___ 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-1733280-replay_filenames into lp:widelands
Is there an easy way to find out if the game is in fullscreen mode when we enter the screen? And can we react to fullscreen switching by (un)checking the checkbox? I haven't checked, but maybe you know about that. If we find out, we could offer dynamic changing of the checkbox state. If we can't, it doesn't make sense storing different checkbox-states for (not in) fullscreen so storing one state should be fine. Or am I misunderstanding what you mean? -- 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. ___ 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-1733280-replay_filenames into lp:widelands
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 +
> +++ src/ui_fsmenu/loadgame.cc 2018-05-16 05:04:34 +
> @@ -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 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.
___
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:~widelands-dev/widelands/bug-1733280-replay_filenames into lp:widelands
Continuous integration builds have changed state: Travis build 3521. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/379560747. Appveyor build 3326. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1733280_replay_filenames-3326. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1733280-replay_filenames/+merge/345566 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1733280-replay_filenames 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1733280-replay_filenames into lp:widelands
Continuous integration builds have changed state: Travis build 3517. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/379055612. Appveyor build 3322. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1733280_replay_filenames-3322. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1733280-replay_filenames/+merge/345566 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1733280-replay_filenames 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1733280-replay_filenames into lp:widelands
GunChleoc has proposed merging
lp:~widelands-dev/widelands/bug-1733280-replay_filenames into lp:widelands.
Commit message:
Added a checkbox to toggle filenames when loading a replay. This is on per
default, as one would never want to toggle them off except on low resolution.
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #1733280 in widelands: "Add filenames to list of replays"
https://bugs.launchpad.net/widelands/+bug/1733280
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1733280-replay_filenames/+merge/345566
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-1733280-replay_filenames into lp:widelands.
=== modified file 'src/ui_basic/table.cc'
--- src/ui_basic/table.cc 2018-05-14 08:24:46 +
+++ src/ui_basic/table.cc 2018-05-15 05:28:21 +
@@ -134,6 +134,13 @@
column.btn->set_title(title);
}
+void Table<void*>::set_column_tooltip(uint8_t col, const std::string& tooltip) {
+ assert(col < columns_.size());
+ Column& column = columns_.at(col);
+ assert(column.btn);
+ column.btn->set_tooltip(tooltip);
+}
+
/**
* Set a custom comparison function for sorting of the given column.
*/
@@ -437,7 +444,14 @@
selected(selection_);
}
-void Table<void*>::multiselect(uint32_t row) {
+/**
+ * If 'force' is true, adds the given 'row' to the selection, ignoring everything else.
+ */
+void Table<void*>::multiselect(uint32_t row, bool force) {
+ if (force) {
+ select(row);
+ return;
+ }
if (is_multiselect_) {
// Ranged selection with Shift
if (SDL_GetModState() & KMOD_SHIFT) {
=== modified file 'src/ui_basic/table.h'
--- src/ui_basic/table.h 2018-05-07 05:35:32 +
+++ src/ui_basic/table.h 2018-05-15 05:28:21 +
@@ -76,6 +76,7 @@
/// Text conventions: Title Case for the 'title'
void set_column_title(uint8_t col, const std::string& title);
+ void set_column_tooltip(uint8_t col, const std::string& tooltip);
void clear();
void set_sort_column(uint8_t col);
@@ -101,7 +102,7 @@
EntryRecord* find(Entry) const;
void select(uint32_t);
- void multiselect(uint32_t row);
+ void multiselect(uint32_t row, bool force = false);
uint32_t toggle_entry(uint32_t row);
void move_selection(int32_t offset);
struct NoSelection : public std::exception {
@@ -189,6 +190,7 @@
TableColumnType column_type = TableColumnType::kFixed);
void set_column_title(uint8_t col, const std::string& title);
+ void set_column_tooltip(uint8_t col, const std::string& tooltip);
void set_column_compare(uint8_t col, const CompareFn& fn);
void clear();
@@ -247,7 +249,7 @@
EntryRecord* find(const void* entry) const;
void select(uint32_t);
- void multiselect(uint32_t row);
+ void multiselect(uint32_t row, bool force = false);
uint32_t toggle_entry(uint32_t row);
void move_selection(int32_t offset);
struct NoSelection : public std::exception {
=== modified file 'src/ui_fsmenu/loadgame.cc'
--- src/ui_fsmenu/loadgame.cc 2018-05-13 07:15:39 +
+++ src/ui_fsmenu/loadgame.cc 2018-05-15 05:28:21 +
@@ -48,7 +48,7 @@
UI::PanelStyle::kFsMenu,
true),
- is_replay_(is_replay) {
+ is_replay_(is_replay), showing_filenames_(false) {
// Make sure that we have some space to work with.
main_box_.set_size(get_w(), get_w());
@@ -57,6 +57,10 @@
main_box_.add_inf_space();
main_box_.add(_, UI::Box::Resizing::kAlign, UI::Align::kCenter);
main_box_.add_inf_space();
+ if (is_replay_) {
+ show_filenames_ = new UI::Checkbox(_box_, Vector2i::zero(), _("Show Filenames"));
+ main_box_.add(show_filenames_, UI::Box::Resizing::kFullSize);
+ }
main_box_.add_inf_space();
main_box_.add(_box_, UI::Box::Resizing::kExpandBoth);
main_box_.add_space(padding_);
@@ -89,6 +93,12 @@
load_or_save_.table().double_clicked.connect(
boost::bind(::clicked_ok, boost::ref(*this)));
+ if (is_replay_) {
+ show_filenames_->changed.connect(
+ boost::bind(::toggle_filenames, boost::ref(*this)));
+ show_filenames_->set_state(true);
+ }
+
fill_table();
if (!load_or_save_.table().empty()) {
load_or_save_.table().select(0);
@@ -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 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);
+ }
+ entry_selected();
+}
+
+
void FullscreenMenuLoadGame::clicked_ok() {
if (load_or_save_.table().selections().size() != 1) {
return;
@@ -128,7 +155,7 @@
}
v
