Question about GUI and directory access.

[Markohs]

 I want to take out the directory-access code in savegame_frame.cc to decouple that code from the GUI and making the current code and the new one using the same routines.

It's basically about moving fill_list() and maybe one or two more functions to a new class and make savegame_frame inherit from it, leaving all filesystem-access related functions in a single file.

The problem is also I can't find a good name for the class (maybe directory_reader or file_utils), and a directory where to place the file, since the "utils" directory doesn't seem to be the correct place for it, and I don't wanna crowd the root filesystem anymore.

I'd just decide something and just implement it, but if this is of any use for trunk code and it will be integrated someday, I prefer hearing your oppinion.

Any ideas/comments?

Thank you!

[Markohs]

Something like this (patch is not final yet).

What do you think, do you think it's the good direction?

EDIT: I'm thinking in deriving even more classes from directory_reader (maybe rename it to directory_listing) specializing check_file and get_info for each of the dialogs, scenario, savegame, pakselector and I think there is another one.

[Dwachs]

Looks good.

There is also a window asking for a heightmap to load.

[Markohs]

Thank you Dwachs!

I already identified more things to decouple, the idea is making the dialogs idependant of the GUI, separating the code in:

- funcionality (processing of the XML cache, handling of the paks, addons, savegame info...)
- filesystem access
- final GUI assembly

In different classes/files. Might increase compilation time sightly but the code will be more interoperable, reusable and maintenable. At least that's the idea.

And while I do so, document the code a bit more than it's now.

Well, back to work, I'll post progress here.

[prissi]

That goes somehow against the OOP pholisophy what you described, but you implemented actually my thoughts ... Indeed, any class needing something from the directory ready should inherit from it. And the directory reader should a pure virtual function check_file(). The pakselect reads only directories that contain a ground.outside.pak, load game could ignore certain entries (and inserts files in different order that load heighmaps) and so on. Thus the task at hand knows best about file check and where to insert.

In terms of simutrans UI philosophy you are right, this is rather a control than a frame. Thus there should be something like a gui_file_enumerator_t, from which one derives a class which does comparison (via < ) and check() to determine suitability of an entry.

No I get offtopic: My personal advice would be rather focussing on 3D code. The CEgui will not become native simutrans UI for the next five years, or so as long as it requires 3D support.

Imho a better GUI for using simutrans should be close to the various native desktop GUI, as simutrans is less of a game than more of a collection of target oriented task, like a desktop application. Thus, useful cross platform libraries are only:

http://www.fltk.org/
http://www.gtk.org/ (which I do not like in appearance)
http://www.tcl.tk/ (of course ... if getting with scripts then this)
http://www.wxwidgets.org/ (this and tk cannot get more cross-platform)

Other than that, I still fail to see the advantage to use another frame buffer layer for gui compared to the frame buffer layer we have now. Most but the simpliest dialoges require functionality like convoilist, toolbars, schedules, etc. Those are simutrans-specific UI elements / actions which would integrate better with the above libraries.

However, my view is biased, as I rewrote most of the GUI code in 2007-2008.

[Markohs]

mmm... on this last month I've been thinking about it it was really worth to spend the time in CEGUI instead of focusing on the 3D game. Even I have to confess I don't like the current UI much, it's true that sufficient and I can right now plant it on top of a 3D framebuffer with no problems. CEGUI is way more flexible and it integrates way better to the 3D enviroment., but it's going to need lots of work to get it working good enough.

Of the frameworks you linked I'd personally chose wxwidgets over all.

I think I'll just finish this savegame/pak selection/loadingscreen, splitting the dialogs functionality as you pointed and focusing to the rendering of the 3D terrain. It will be more profitable and I think it's what the community is demanding more. And it's more exciting that the GUI to program. Right now I understand and know the code way better than I did one year ago, I'd like to have it another look soon.

[Markohs]

I want to post this patch to the trunk if you don't mind, will make the merging of my branch a bit easier for me, and I think it's good taking the directory access functions out of a GUI class.

I tried to fix the two .vxproj and the Makefile, I think I done it right, but just tested the _VS10 one.

Do you mind me commiting it to the svn, prissi/Dwachs?

https://dl.dropbox.com/u/30024783/directory_reader.patch

Thank you!

[Dwachs]

I think it is a good idea to separate this out of gui code.

But I think there is still code duplication at work: in utils/searchfolder.* there are routines to find all files in a directory matching some string expression. This should be unified in some way. Ie directory_reader should use the functionality of searchfolder (there is already an iterator to iterate through all matched filenames!). Maybe it would
be easier to call searchfolder_t stuff in savegame_frame.cc ?

I have also a wish: Is it possible to extend the functionality to load stuff from two directories? Ie heightmaps/scenarios from the installed pak-directory and from the addons/pak directories?

[Markohs]

I'll have it a look.

[kierongreen]

I think it would be wise to get the stable release out first, then we can have a while to sort out details?

[Markohs]

sure, np, I'll make the changes Dwachs suggested, and after the release we talk when to submit it.

[Markohs]

Two options:

Adding the multiple directory searching in the directory_reader, making a alternate constructor that accepts a vector_tpl ( I know you prefer that to std::vector ), using as many searchfolder_t as necessary.

*OR*

I can also add the multiple directory support to seachfolder_t straight, but do you think that will be useful? afaik searchfolder_t is used in:

- obj_reader.cc
- translator.cc
- simmain.cc

Another thing I was thinking was enumerating the possible extensions in directory_reader.cc, or searchfolder_t and pass a enumerated type to the creator, instead of a string, what do you think? Could be cleaner, and so we could have a global enum listing the file extensions simutrans uses (even allowing for example savegames have multiple extensions, thus easily making loadsave dialogs managing multiple extensions easily)

[prissi]

When calling the directory read explictly from the dialogues constructor with a slist to add files to it from an individual searchpath, those multiple directories coudl be easily customized for each dialogue.

[Markohs]

Anyone knows why gui/savegame_frame.cc, fill_list(), the unix version ignores the 'only_directories' parameter? Reading:

http://linux.die.net/man/3/readdir

About the 'd_type' element, says it's not supported on all unix systems, might it be that Haiku doesn't report it?

[Markohs]

Almost finished this, one of the problems that arised in multidirectory is that the gui controls by default don't show the path to the file and assume they are all in one directory to load them(that's obvious), so when I return the entries I have to give the full path in the button, so the game knows where is the file that the user clicked.

I also had to work around the fact the frame classes handled the extension with a dot ".sve", and the objreader functions supplied them without a dot "sve". Still needs more work and me testing it in linux, but here's a preview if you are interested.

[Dwachs]

Almost finished this, one of the problems that arised in multidirectory is that the gui controls by default don't show the path to the file and assume they are all in one directory to load them(that's obvious), so when I return the entries I have to give the full path in the button, so the game knows where is the file that the user clicked.

We can worry about this later

Still needs more work and me testing it in linux, but here's a preview if you are interested.

I could do testing in linux, but the directory_reader files are missing from the patch.

[Markohs]

Oh, right, I'll finish it soon and test it tomorrow at work were I have a linux machine. I'm almost sure I'll remove directory_reader.cc anyway, because using the searchfolder_t shrinked that class to be too small for it to be a significant class, and it's not sub-classed anywere else than in savegame_frame, nor I think it's gonna be used anywere else.

[Dwachs]

Any news about this ?

[Markohs]

I had to switch my time to personal work and coudn't finish it completely, but I had advanced quite a lot. I hope to be back to simutrans soon. I'll squeeze some time this weekend to finish it.

If I remember correctly it was working but it needed modifications to add some ideas you exposed here (merging two classes I think).

Searching multiple directories is supported even it's non-tested function yet, two searching functions:

int searchfolder_t::search(const std::string &filepath, const std::string &extension, const bool only_directories, const bool prepend_path )
int searchfolder_t::search(vector_tpl<const std::string> &filepaths, const std::string &extension, const bool only_directories, const bool prepend_path )

The current patch if you are curious is here.

[Dwachs]

directory-reader is missing from the patch.

I asked, because I am in the process of reworking the scenario-load process. If your patch would be ready, then it might be possible to read scenarios (and heightmaps) from addons/ in addition.

No need to hurry

Btw nice to have you back in action

[Markohs]

directory-reader is missing from the patch.

Whooops.

I asked, because I am in the process of reworking the scenario-load process. If your patch would be ready, then it might be possible to read scenarios (and heightmaps) from addons/ in addition.

Oh great, I'll post it on weekend then!

No need to hurry

Btw nice to have you back in action

Thank you.

I wish I could have all my time for this. But soon I will.

[Markohs]

Okay, the patch here.

I'll make a compilation of what has been done since I can't remember very well what was missing.

- Moved all file access code from gui/savegame_frame to a new class named utils/directory_reader. This gave some problems in some dialogs that have been fixed.
- Extended the utils/searchfolder class to be able to read directory entries, to customize printing the full path of just the file path, and able to search in multiple directories.

So now, have it a look, I'll commit to svn if you think it's ok.

You tell me Dwachs what's needed for the scenarios/heightmaps, or what can be imrpved here. Right now comes to my mind two possibilities:

1) Make directory_reader.cc use searchfolder.cc
2) Make directory_reader (and savegame_frame) able to read from multiple directories, that will require some coding to add to trhe gui elements the full path of the items.

EDIT: Updated patch, completed a few new steps, tested in linux and I think it works completely but coudn't test fully because on virtualbox coudn't get the graphincs working good.

[Markohs]

I think I'll better first make simutrans able to load heightmaps from the "maps" user folder and from a folder named "maps" inside the pak directory better. So I see what's missing. When I have that running you will be able to do the same to scenarios.

[Markohs]

Just one question:

I updated to head and MSVC 2010 keeps recompiling a few files even if I didn't touch anything. This didn't happened before, is this intended, you changed something or it's something on my setup that might be the problem? I say just to skip me time here checking what's wrong if it's just normal.

The files it re-compiles ALLWAYS are:

Code Select Expand


1>------ Build started: Project: Simutrans, Configuration: Debug Win32 ------
1>Build started 01/10/2012 1:12:23.
1>InitializeBuildStatus:
1>  Creating "Debug\Simutrans.unsuccessfulbuild" because "AlwaysCreate" was specified.
1>PreBuildEvent:
1>  Microsoft (R) Windows Script Host Version 5.8
1>  Copyright (C) Microsoft Corporation. All rights reserved.
1> 
1>ClCompile:
1>  gameinfo.cc
1>  Skipping... (no relevant changes detected)
1>  simworld.cc
1>  simsys_w.cc
1>  simmain.cc
1>  welt.cc
1>  settings_stats.cc
1>  server_frame.cc
1>  loadsave_frame.cc
1>  banner.cc
1>  umgebung.cc
1>  network_file_transfer.cc
1>  network_cmd_ingame.cc
1>  loadsave.cc
1>  All outputs are up-to-date.
1>ResourceCompile:
1>  All outputs are up-to-date.
1>ManifestResourceCompile:
1>  All outputs are up-to-date.
1>LINK : warning LNK4075: ignoring '/INCREMENTAL' due to '/LTCG' specification
1>  Generating code
1>  Finished generating code
EDIT: Solved, noticed the post-build event

[Markohs]

 Question, maybe somebody can help me figure how to design this, I'm not very familiar with the simutrans vectors.

I'd like to overload this:

Code Select Expand


directory_reader(const char *suffix, const char *path, bool only_directories = false );


With:

Code Select Expand


directory_reader(const char *suffix, vector_tpl<const char *>&paths, bool only_directories = false );


I can do that, now assuming I overload savegame_frame_t the same way to accept a array in place than just a const char* for the path, look at (from load_relief_frame.cc, the height map dialog):

Code Select Expand


load_relief_frame_t::load_relief_frame_t(settings_t* const sets) : savegame_frame_t(NULL, "maps/")


How can I express an array inline in that creator? Something like:

Code Select Expand


load_relief_frame_t::load_relief_frame_t(settings_t* const sets) : savegame_frame_t(NULL, {"maps/","pak128/maps})


Is this possible?


And another question, having a member variable like:

Code Select Expand


    vector_tpl<const std::string> paths;


Why this code fails?

Code Select Expand



directory_reader::directory_reader(const char *suffix, vector_tpl<const char *>&paths, bool only_directories){
...
    FOR(vector_tpl<const char *>, const i, paths) {
        if (i==NULL)
            this->paths.append(std::string(SAVE_PATH));
        else
            this->paths.append(std::string(i));
    }
...



The compiler error is:

Code Select Expand


1>------ Build started: Project: Simutrans, Configuration: Debug Win32 ------
1>Build started 01/10/2012 19:18:59.
1>InitializeBuildStatus:
1>  Touching "Debug\Simutrans.unsuccessfulbuild".
1>ClCompile:
1>  directory_reader.cc
1>c:\code\simutrans\trunk\utils\../tpl/vector_tpl.h(96): error C2678: binary '=' : no operator found which takes a left-hand operand of type 'const std::string' (or there is no acceptable conversion)
1>          c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\include\xstring(707): could be 'std::basic_string<_Elem,_Traits,_Ax> &std::basic_string<_Elem,_Traits,_Ax>::operator =(std::basic_string<_Elem,_Traits,_Ax> &&)'
1>          with
1>          [
1>              _Elem=char,
1>              _Traits=std::char_traits<char>,
1>              _Ax=std::allocator<char>
1>          ]
1>          c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\include\xstring(761): or       'std::basic_string<_Elem,_Traits,_Ax> &std::basic_string<_Elem,_Traits,_Ax>::operator =(const std::basic_string<_Elem,_Traits,_Ax> &)'
1>          with
1>          [
1>              _Elem=char,
1>              _Traits=std::char_traits<char>,
1>              _Ax=std::allocator<char>
1>          ]
1>          c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\include\xstring(766): or       'std::basic_string<_Elem,_Traits,_Ax> &std::basic_string<_Elem,_Traits,_Ax>::operator =(const _Elem *)'
1>          with
1>          [
1>              _Elem=char,
1>              _Traits=std::char_traits<char>,
1>              _Ax=std::allocator<char>
1>          ]
1>          c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\include\xstring(771): or       'std::basic_string<_Elem,_Traits,_Ax> &std::basic_string<_Elem,_Traits,_Ax>::operator =(_Elem)'
1>          with
1>          [
1>              _Elem=char,
1>              _Traits=std::char_traits<char>,
1>              _Ax=std::allocator<char>
1>          ]
1>          while trying to match the argument list '(const std::string, const std::string)'
1>          c:\code\simutrans\trunk\utils\../tpl/vector_tpl.h(92) : while compiling class template member function 'void vector_tpl<T>::append(T &)'
1>          with
1>          [
1>              T=const std::string
1>          ]
1>          c:\code\simutrans\trunk\utils\directory_reader.h(49) : see reference to class template instantiation 'vector_tpl<T>' being compiled
1>          with
1>          [
1>              T=const std::string
1>          ]
1>
1>Build FAILED.
1>
1>Time Elapsed 00:00:00.82
========== Build: 0 succeeded, 1 failed, 0 up-to-date, 0 skipped ==========



Looks like he can't assign the const std::string value to an element of the array. How I'm suposed to create arrays of const std::string values?

I'm sorry this might be obvious to you but I'm a bit lost, I'd appreciate a helping hand.

[Dwachs]

You cannot have vectors of 'const string', as the elements of this vector are constant = useless. Try without 'const'.

There is no way (afaik) to initialize a vector_tpl inline.

[Markohs]

mmm... ok, thank you!

Anyway I think constant string pointers can be useful, no? variable arrays of immutable strings, could be useful, no?

Well, anyway, I'll use non const strings to see how it goes.

About the init of a vector_tpl, that will force me to use a zero-terminated array I guess, or a const char**. Well, if there is no other option, let it be that way.

Or maybe this inheritance design doesn't make much sense, just to keep compatiblity with the old class, after all there are just 5 classes that inherit from there. I'll think about it.

Thanks for your help!

[Markohs]

Okay, needs a bit more work but it's not so far of finished, looks like this:



Any suggestion on how to show the values so the dialog looks good?

Maybe we can use a sub-panel for each directory searched? Maybe disable delete for the ones in the pak dir?

the "c:\simutrans\pak128" it's extracted from "umgebung_t::objfilename". Just searching for "pak128/maps" doesn't work because simutrans runs with the user data dir as CWD, afaik.

The patch is here, note it's not finished, I know it lacks things, but feel free to comment whatever you want.

the patch *includes* directory_reader.cc and .h, if when you patch they don't show up on your "utils" folder, might be some version problem on the way I created the patch, but it's there (last time I had to copy/paste from the patch file itself to my HDD and crete the files manually, dunno why).

Sometimes I wonder if I ask you guys too much. It's not that I can't do it, just want to get an idea of how whould you like to see it done. If I ask too much, just tell me and I'll do it my way.

[Dwachs]

What about showing something like

Code Select Expand


Files from pak128/maps
x map1
x map2
Files from userdir/bla
x map3
x map4

That is separate the entries by an extra line, which identifies the directory. On the buttons only show the filename.

As to the initializer of the vector: you can do this in the constructor of the child class, vector.append("sljfg").

[Markohs]

mm... yea, that's a good idea. I'll see what can I do.

About the vector.append, makes sense, no need for a function.

Thank you!

[Markohs]

Close to finished now, the limitation is you can only use multidirectory dialogs when they don't have a "save" option, it doesn't make much sense in that case. afaik the suitable dialogs are the the scenario and heightmap dialogs.

Ok, the patch here. Some comments:

- I still need to document and tidy the code a bit.
- Had to modify that linked list to add "headers" too, that represent the "Files from x:" message, maybe it was not the best option. I allways store the name of the file just in the button and the full path in another variable named "info". This made necessary a slight modification of all dialogs functions.
- I kept the "add_path" function because I feel like it's more correct, for the case than in future extensions of the class adding a path means more actions are needed than just extending the vector. Maybe it's a bit stupid but feel more confortable like that.
- directory_reader.cc it's gone again, since all file I/O has moved to searchfolder.cc. It might be necessary a decouple in the future for CEGUI/Simutrans3D but atm, it feels ok like that, more compact.
- I think the memory management doesn't elak, but I'm not 100% sure
- Is really a problem using a slist of std::strings? the comments on the file say I should not do it.
- It's untested in linux/haiku, just tested on my windows 7 system.

It's my first time coding this part of simutrans and using those tpl structures, might have made huge mistakes. I'd be happy to hear about whatever you think it's important to say about the code, go on.

Looks like this:

[Fabio]

The second string Files from c:\... should be pushed down a couple of px.
And it would be nicer for players if path (under Windows) wouldn't mix slashes and backslashes (/ & \). Maybe replacing them just before displaying with only one (\ backslash being usually default on Windows).
Nice job!

[Markohs]

The second string Files from c:\... should be pushed down a couple of px.
And it would be nicer for players if path (under Windows) wouldn't mix slashes and backslashes (/ & \). Maybe replacing them just before displaying with only one (\ backslash being usually default on Windows).
Nice job!

Makes sense, I'll work on those two, thank you!

[Dwachs]

- Is really a problem using a slist of std::strings? the comments on the file say I should not do it.

Do you refer to a comment like 'Never ever use cstring here' ?
You can forget about this comment, it is targeted at cstring_t, which were hand-made classes and are long gone from the code base.

In order to use slist_tpl, the datatype must be trivially copyable (pointers, integers,...) or must have a copy-constructor (which should be the case with std::string and plainstring).

[VS]

Does it make sense to have delete button in these dialogs? If they're for scenarios and heightmaps, deleting them from Simu (no trash bin to revert) feels like a dumb move for the user...

(Would there be interest in patch that adds recycle bin support on windows?)