[Patch] Simple preview for save game files

[Geoff the Medio]

Only remaining complaint about the function is that the layout breaks a bit if the font is too big. Making the left columns slightly wider might help, or giving them a bit of stretch when resizing...

[Mitten.O]

Wow didn't even realize the font size could be changed.
The static sizes are calculated using the font now.

[Geoff the Medio]

I had to add mutable access to the header row
to the protected interface to keep it synced.

There's already the const ColHeaders() function, which while oddly named, could have a non-const overload added. This would be better than adding an inconsistently-named equivalent, "MutableRowHeaders()".

Also, note the formatting (capitalization) and style (indicates what is returned vs. I'm not sure what exactly) of the comments... "Allows mutable access to the header row" vs. "returns the row containing the headings for the columns, if any. If undefined, the returned heading Row will have size() 0." Thanks for aligning the comment with the surrounding ones, though.

Also, in the actual file, please try to make the code formatting consistent with the surroundings when editing an existing file...

Code: Select all

ListBox::Row& ListBox::MutableRowHeaders(){
    return *m_header_row;
}

vs

Code: Select all

const ListBox::Row& ListBox::ColHeaders() const
{ return *m_header_row; }

Indenting code within namespace {} would also be appreciated.

[Mitten.O]

Done and done.

[Geoff the Medio]

Seems OK now.

Might be nice if the save version of the dialog (as opposed to when loading) would have a default file name.

If a Linux user could test and report, it would be appreciated...

[Dilvish]

Substantively, the patch appears to work largely as expected on Ubuntu Linux, subject to the following multiplayer problems: The multiplayer file load dialog still lists all the old incompatible save files, but that's not a big deal. Multiplayer games (whether or not given an explicit '.mps' extension) are saved with the .sav extension added and therefor not showing up on the multiplayer load dialog. They are therefore however showing up on the single player load dialog; this latter aspect would actually be appealing to me if they were identified there as multiplayer saves.

As for subjective impression, I think the basic idea is really good, but I have a few issues with details. My biggest issue is that I really dislike losing the filename-- I often save files named according to the aspect I wanted to come back and test/review later, and that's much more of a pain if I can't actually see the filename in the file dialog. Even as just a player I would much miss being able to use the filenames. If I could add a note when saving (and see that in the load dialog) then that could be a reasonable substitute for seeing the filename.

Also, player name and empire are not seeming particularly helpful to me as for choices of 'preview' information, though this may be (apparently is) largely a matter of personal preference -- for me, number of AIs and galaxy setup info, particularly number of systems, seems more of interest. I know we want to avoid a plethora of options, but perhaps there could be some way to control what info is included in the preview?

[Mitten.O]

As it happens, GalaxySetupData is in MultiplayerCommon and in the beginning of a save file to boot.
Therefore it would not be that much trouble to show it, assuming we return to using a side panel
and drop the empire name.
What do people think?

On showing the file name in the list, I kind of agree that it would be useful.
That is why I had it there originally. We could drop the empire
(or the player name, if we chose to have the side panel)
and put the file name back.

I'll see about the other issues in the meanwhile.

[Mitten.O]

• Gives a simple default name when saving
• Focuses on the name edit when saving
• Lets choose by pressing enter when filename focused

The sum effect is that the user can just press enter when the dialog opens.

The autogenerated name is "save-" + ISO date time +".sav".
Encoding more information in the name like auto save does would make it annoyingly long.
This should be unique, fairly concise and contain useful information.
My only worry is that on windows : is illegal. Does boost sanitize it for us, or do we have to?
The boost reference says this is implementation defined.

EDIT: Also supports multiplayer save segregation.

[Geoff the Medio]

Maybe add a bunch of extra columns (file name, starting species, for now, and some could be removed later if they're found not useful in practice?

Making it configurable with an options DB setting what info to show wouldn't be very complicated.

There is already some save name with date and time generation code used for autosave naming... It might help to look at / use that. It's known to be safe on all current OS running FreeOrion.

Having MP and SP saves be the same extension and loadable as eachother should be fine, though which empire to control in SP after starting an MP game is an issue. (Loading an SP game as MP is not a problem, as the lobby exists to assign players to empires...)

Separate directories for MP and SP saves might be appropriate, though.

I'd like to use the save file selecting dialog in the MP game setup screen as well, instead of the droplist that is there now.

[Mitten.O]

Generated save game name now shares timestamp code with autosaves.

Added the filename to the table. I think an option for this would be overkill.

[Mitten.O]

I'd like to use the save file selecting dialog in the MP game setup screen as well, instead of the droplist that is there now.

The lobby gets that list from the server and identifies the choice using an index in the list it got.
Do we even know that the multiplayer save resides on the local computer?
Regardless, I'd consider messing with it out of scope for this patch.

[Dilvish]

I'd like to use the save file selecting dialog in the MP game setup screen as well, instead of the droplist that is there now.

Regardless, I'd consider messing with it out of scope for this patch.

V12 seems to work fine for me, and it sounds reasonable to me to want to avoid too much scope creep -- Geoff, I support it being committed as is. I do hope that both this MP suggestion and the extra columns suggestion could be followed up on sometime.

[Geoff the Medio]

Committed, after slight edits.

Send me a pm with a real name for the credits, if so inclined.

I think the path selection line should go on the top, as is I think the standard in most such dialogs.

An "up" button next to the path selection box would also be useful.

I find the column stretching a bit awkward... the player and empire names don't need so much space when the dialog gets big, but often I find the filename doesn't fit, which is annoying when there's a bunch of wasted space in the preceeding columns.