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!
Something like this (http://dl.dropbox.com/u/30024783/dir.diff) (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.
Looks good.
There is also a window asking for a heightmap to load.
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. :)
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.fltk.org/)
http://www.gtk.org/ (http://www.gtk.org/) (which I do not like in appearance)
http://www.tcl.tk/ (http://www.tcl.tk/) (of course ... if getting with scripts then this)
http://www.wxwidgets.org/ (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.
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. :)
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!
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?
I'll have it a look. :)
I think it would be wise to get the stable release out first, then we can have a while to sort out details?
sure, np, I'll make the changes Dwachs suggested, and after the release we talk when to submit it.
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)
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.
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?
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 (https://dl.dropbox.com/u/30024783/directory_reader.patch)if you are interested.
Quote from: Markohs on June 21, 2012, 11:13:57 AM
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 ;)
Quote
Still needs more work and me testing it in linux, but here's a preview (https://dl.dropbox.com/u/30024783/directory_reader.patch)if you are interested.
I could do testing in linux, but the directory_reader files are missing from the patch.
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.
Any news about this ?
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 (https://dl.dropbox.com/u/30024783/directory.diff).
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 :)
Quote from: Dwachs on September 27, 2012, 06:24:04 AM
directory-reader is missing from the patch.
Whooops. :)
Quote from: Dwachs on September 27, 2012, 06:24:04 AM
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!
Quote from: Dwachs on September 27, 2012, 06:24:04 AM
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. :)
Okay, the patch here. (https://dl.dropbox.com/u/30024783/directory_reader.patch)
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.
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.
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:
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
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:
directory_reader(const char *suffix, const char *path, bool only_directories = false );
With:
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):
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:
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:
vector_tpl<const std::string> paths;
Why this code fails?
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:
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.
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.
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!
Okay, needs a bit more work but it's not so far of finished, looks like this:
(https://dl.dropbox.com/u/30024783/Untitled24.png)
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 (https://dl.dropbox.com/u/30024783/multiple_dirs.patch), 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. ;)
What about showing something like
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").
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!
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 (https://dl.dropbox.com/u/30024783/multiple_dirs_v2.patch). 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:
(https://dl.dropbox.com/u/30024783/Untitled25.png)
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!
Quote from: Fabio on October 04, 2012, 11:47:49 PM
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!
Quote- 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).
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?)
Please continue brainstorming over there:
http://forum.simutrans.com/index.php?topic=10612.0
When dealing with the recycling bin, I had to use the Windows API, ofc. I protected that code with "#ifdef _WIN32", I think that will be enough. I'm not an expert in the Windows plattform, and I got one question:
To deal with the file interacting functions, I had to use the SHFileOperation function, given I get the filename to delete as a const char * pointer, I'm forced to use the ANSI version (8 bit per char), SHFileOperationA instead of the Unicode (16 bit per char) version, that's the default since simutrans defines UNICODE in the includes.
This works, on my computer, but dows anybody know if this can break the code on computers with unicode chars? (Japanese for example) Since all the other file I/O is done using standard C routines from stdio.h, I get char* pointers, will it interact with this functions smoothly?
Thanks!
EDIT: Well, just noticed that filenames with non-latin filenames don't even show in the loading screen with the current code, so not being able to delete them makes no difference.
Ok, this is almost finished, just a last question:
Is desirable to allow players to install paks in their home directory?
I can expand the pak selector to read form the simutrans folder and from $userdir/paks for example, and its corresponding $userdir/addons/pakname
The windows version deletes allways use the trash bin by default, it's been easy to implement.
The path here (https://dl.dropbox.com/u/30024783/multiple_dirs_v3.patch), for the case you are curious. Misses some cleanup and styling to match the rest of simutrans, but it's the next step. It also misses disabled deletes on paths shared by all users.
afaict, the windows version stores savegames in 'My Documents\simutrans\save', the pakset addons reside in 'My Documents\simutrans\addons\pakbla', so I think it would be logical to enable loading of paksets from 'My Documents\simutrans\pakbla'.
This should be implemented for linux version, too. I guess there will be not much of a difference.
documentation/coding_styles.txt states :
Quote
- Header comments: Include files which define classes do not need header
comments, because the class comment can serve as a header comment.
Other files should carry header comments like the example shown below:
/* boden.cc <- name of the file
*
* Natur-Untergrund für Simutrans. <- Short description of file contents
* Erstellt am ??? ? <- Creation date (if known)
* Überarbeitet Januar 2001 <- Additional history information
* von Hj. Malthaner <- author/creator of the file
*/
This is differente than the current ones in for example savegame_frame.h:
Quote
/*
* Copyright (c) 1997 - 2001 Hansjoerg Malthaner
*
* This file is part of the Simutrans project under the artistic licence.
* (see licence.txt)
*/
What should I put? We have to update that document a bit.
Also, Hajo stated he wanted his name out of the game, and it's still around all files.
document.txt should certainly need an upgrade. Also concerning document styles. So just change whatever you think describes the current and easily achievable final state seems appropriate. Or post new document.txt here, if you want more discussion.
Can somebody please try to compile simutrans with mingw and this (https://dl.dropbox.com/u/30024783/multiple_dirs_v4.patch) patch applied please? I don't have that environment installed. Can you check too if you can delete savegames in-game then and if it's using the windows trash folder?
I'd happily hear about others systems, I'll test on my ubuntu system too, don't have Haiku installed yet, I'll see if I can install one soon.
The patch atm:
- Removes all file I/O routines from savegeme/frame.cc.
- Adds multiple directories support to that dialog, atm to heightmaps, and scenarios.
- On Windows plattform, uses the recycling bin to delete files.
- Cleans up some old routines.
What does it miss?
- Installing paks in user folders (Needs some modifications for addons to work atm, or savegames have wrong pakset info and addons don't load)
- Disable delete button for certain files (not that important I guess since we use trash bin, but has to be implemented)
If you are okay with the code I'll submit to subversion soon, and finish those 2 last issues later.
In the way oif development of this I detected a bug thta made impossible to delete heightmaps in-game, that fix is already in svn.
In file included from gui/loadsave_frame.h:12:0,
from gui/banner.cc:25:
gui/savegame_frame.h:145:7: error: extra qualification 'savegame_frame_t::' on m
ember 'cleanup_path' [-fpermissive]
make: *** [/build/gui/banner.o] Error 1
Wooops, strange that Visual C++ didn't gave error there, fixed the patch. :)
you can fix yourself the .h if you want to go faster, savegame_frame.h 145, it had n extra savegame_frame_t::
I'm also installing MinGW here for future testing myself, Haiku will follow. It's compiling so far, but... It's taking way longer to compile than Visual C++... Anyone knows ifs the resulting executable comparable in speed?
Isn't mingw WAY slower when compiling?
Okay, it's working here on my mingw, testing haiku. Anyone has a last suggestion/comment about the code? I'll submit soon if nobody has anything against it.
It is possible that Visual C++ automatically enables paralell compiling. This must be enabled explicitly when using GNU make by specifying the -j# option, where # is the number of paralell compilation processes.
mmm... I was comparing Visual Studio debug build against mingw release (optimized). Compiling in release mode took longer, but still quite less than mingw. No, it didn't enabled multiprocessor compilation, just checked.
I think the code is maybe faster too, but didn't take extensive measurements.
Well, nothing related to this patch, however.
Save renaming fails - end up with _temp.sve always.
Is it Windows.h or windows.h? later I thinks.
Also all other includes of it use NOMINMAX; And one even tries WIN32_LEAN_AND_MEAN. Unpredictable behavior depending upon compile order...
Coding styles is somewhat out of data, just watch Prissis commited for the style du jour... re: if for while bracket spacing.
IMHO patch should wait until after next release before inclusion. i.e. Let it spend some time in the nightlies.
I find GCC quite speedy, it's make that takes forever - 6 secs before it gets GCC going. Then only another 41 for a full -O3 build...
The MSVC generated code will be significantly slower than GCC for Simutrans. It's forced to fall back onto the USE_C routines rather than the assembly. Also the lack of structure packing results in quite a chunk of extra memory needed.
Thanks for having it a look TurfIt!
About the windows.h with caps, I just copied from msdn function reference, correcting it.
According to some information I googled WIN32_LEAN_AND_MEAN comes from the old Windows 16 code and does not make much sense anymore. I'll use the NOMINMAX, sure.
Regarding the rename, what do you mean exactly? I can't see a way to rename a savegame from the GUI, is there any? Or are you experiencing failures on savegame overwrite? I don't understand the problem.
I'll have another look to the style, you are right. I think I have some "if" not bracketed because it was only one sentence, will correct that too. About the documentation I tried to follow doxygen documentation more or less like we started talking about some months ago in the "German in the code" topic. Of course I can adapt to whatever you decide.
About the nightlies, you are right, It's better to wait intil release comes out.
Also... Should I make a translation entry for the listing headers? Right now it's a fixed "Files in %s" sprintf.
And yea, it's the 'make' that's slowing down things too. Anyway I think VS it's faster compiling and executing, but I didn't measured exact times, so it might be just a personal impression. I used the last version of MinGW, with gcc 4.6.2, -O3. And on visual studio 2010 /Ot. Both with SDL. What do you mean exactly with "structure packing"? Just curious.
The game initially saves using _temp.sve then renames to the specified name. That rename is failing so I end up with _temp.sve instead of the name I told it to save under.
'if( x ) {' instead of 'if (x){' etc.
I'd say everything should be translatable...
EDIT:
packing - see #define GCC_PACKED __attribute__ ((__packed__))
although I thought it was applied to more...
WIN32_LEAN_AND_MEAN tells windows.h not to include several other include files for things that aren't important for simple Win32 programming. This would be things like the multimedia system, RPC, DDE and the shell API. So defining it should reduce compilation time.
A full optimized debug build with 3 paralell jobs takes 3 minutes for me.
Just checked and WIN32_LEAN_AND_MEAN excludes the struct SHFILEOPSTRUCTA, and the ZeroMemory and SHFileOperationA functions, so I'll stiick to just NOMINMAX
About the packed structs: I see it now, I guess the objective is saving memory, no? Because accessing to those non-natively aligned positions can be slower maybe? Not packing can maybe cancel more cache lines too on the other hand. Well, ok, I see what does it mean now.
mmm... coding_styles gives lots of options on how to space ifs, and in fact, they are different in different files. Only enforces a space between the closing parenthesis and the opening bracket in if, and double spaces around LOGICAL operators. The spaces look like they can be written freely:
Quote
Do not forget brackets along comparisons and use double spaces around
logical operators. Avoid spaces before the comparison operator:
if((i&3)==1 && ptr==NULL) {
You may also use double spaces after/before the brackets:
if( (i&3)==1 && ptr==NULL ) {
Quote
C:\code\simutrans\trunk\dataobj\dingliste.h(58): if(n>top) {
C:\code\simutrans\trunk\besch\fabrik_besch.h(63): if( field_classes>0 ) {
C:\code\simutrans\trunk\besch\fabrik_besch.h(256): if(!fields) {
C:\code\simutrans\trunk\dataobj\fahrplan.h(70): if( count == 0 ) {
C:\code\simutrans\trunk\dataobj\fahrplan.h(73): else if( aktuell >= count ) {
C:\code\simutrans\trunk\dataobj\fahrplan.h(91): if( !eintrag.empty() ) {
C:\code\simutrans\trunk\boden\grund.h(333): void set_kartenboden(bool tf) {if(tf) {flags|=is_kartenboden;} else {flags&=~is_kartenboden;} }
C:\code\simutrans\trunk\boden\grund.h(419): if(get_typ()==tunnelboden) {
C:\code\simutrans\trunk\boden\grund.h(559): if(wt == typ) {
The for spacing looks a bit weird to me, but well, rules are rules!
Quote
Use spaces in for loops like in ifs:
for( int i=0; i<10; i++ ) {
Imho your patch has the proper coding style.
The coding_style.txt is needs rework, as it is seriously outdated and some rules are clearly not respected during the last years ...
I tend to make changes, which are consistent to the coding style used in the particular file.
Here, in gui/pakselector.cc, brackets {} are missing:
+ if (i.type == LI_HEADER )
+ y += D_BUTTON_HEIGHT;
+ continue;
The opening bracket of a function should be at a new line: pakselector.cc, +std::string pakname(const char *filename).
I would like to see doxygen-like comments idented (gui/savegame_frame.h).
Quote from: Dwachs on October 16, 2012, 11:36:54 AM
Imho your patch has the proper coding style.
The coding_style.txt is needs rework, as it is seriously outdated and some rules are clearly not respected during the last years ...
I tend to make changes, which are consistent to the coding style used in the particular file.
Here, in gui/pakselector.cc, brackets {} are missing:
+ if (i.type == LI_HEADER )
+ y += D_BUTTON_HEIGHT;
+ continue;
The opening bracket of a function should be at a new line: pakselector.cc, +std::string pakname(const char *filename).
I would like to see doxygen-like comments idented (gui/savegame_frame.h).
Roger, will change all of that. Thank you!
I've decided to go myself:
Use single space between comparision ( < == > etc )
Not using spaces after parenthesis in expressions: (hi == bye)
And respect the rest of the rules in coding_styles.
Markohs, will you commit this patch before or after this weekend's release?
I would like to incorporate these changes
https://github.com/Dwachs/simutrans/tree/scripted-rework-paths
as well, which are changes to scenario directory structure, and might break your patch.
Yes, the plan is commiting ater the release, I just need to fix the bug TurfIt said.
All those bugs are caused because all of filenames are qualified (can be relative) now, what was "test.sve" it's now "save/test.sve". I had to undo multiple pak directory because the same reason, "pak128" turned "c:\simutrans\pak128", affecting umgebung_t::objfilename, wich broke things. So minor tweaks are required in some places, will have it tested and finished soon.
Anyway if you want to apply something now don't worry, commit if you wish, I'll fix my patch to match your code. Take parts of my patch if you need too.
Done. The changes are in r5989. It should not conflict too much with your patch. The only thing you might have to look at is that scenario_frame_t now sets the inherited member variable path in the constructor, and does not give the path as parameter to the savegame_frame_t class constructor.
There is a error in your patch I think, at least shows here with visual studio.
You removed #include "../simsys.h" in scenario.cc and it's needed for line 557, "chdir(umgebung_t::program_dir);" chdir is undefined.
EDIT: Ok, marged with my patch no problem, I'll have to find a solution for that "see below" default path that adds a useless section listing, plus some way for handling this better.
Before it was just pakselector the one to search for directories and check their validity (if they have a certain file inside) later. It's a bit ackward way of making it condidering the class structure, and sub-optimal because we can do that already in the find.
The initial design is not useful any longer since 2 of their 4 child objects have this non-expected behaviour. I'll design something to fit all better together.
btw, nice job with scenarios, it's looking great, testing it out!
EDIT2: it whould be a good idea imo to add the scenario dialog as a button in the top bar. Do you depend on pak mantainers to add the button or can do for every of our users somehow?
A preliminar new version of my patch integrated with yours here (https://dl.dropbox.com/u/30024783/multiple_dirs_v5.patch). Still has issues on savegames.
For saving, I'd much rather type 'test' and have it save to the correct save/test.sve than be forced to type 'save/test'. i.e. Required paths should be added by the program, not the user.
Coding style - I only mentioned it as I seen many 'if (x)' constructs. The very constructs I've observed prissi make commits that do nothing but change them to 'if( x )'. Why add extra work?
Different files and different sections in the same file have different styles simply due the evolving nature of the program. Hence why I suggested looking at recent commits for style suggestions.
Quote from: TurfIt on October 17, 2012, 12:51:29 AM
For saving, I'd much rather type 'test' and have it save to the correct save/test.sve than be forced to type 'save/test'. i.e. Required paths should be added by the program, not the user.
Yea, that's the idea, and it works, you are not suposed to write save/test, just test. The problem is using karte_t::laden with a qualified name, since on save it will write the name with the prefix in the textbox next time you try to save, when it sould just write the filename discarding the basename. I need to filter [/\] in that dialog too on user-entered data and raise an error.
Quote
Coding style - I only mentioned it as I seen many 'if (x)' constructs. The very constructs I've observed prissi make commits that do nothing but change them to 'if( x )'. Why add extra work?
Different files and different sections in the same file have different styles simply due the evolving nature of the program. Hence why I suggested looking at recent commits for style suggestions.
Ok Turfit, even I personally prefer not using those spaces, I can use them, no problem. What I'd really like to is if we all decided for one of the conventions and use it everywere, correcting code as we revisit those files eventually.
So, my proposal:
parenthesis in expressions with one space on the sides
Quote
if( x ) {
}
one on comparision
Quote
if( x < 5 ) {
}
2 on logical:
Quote
if( x && NULL ) {
}
and, following the same as above, that whould end in:
Quote
if( x == NULL && y == 1 ) {
}
Is this ok? No exceptions. ;)
Only problem is that style's not used anywhere! 8)
I'm not saying you must use the other, only relaying my observations of what's been done lately... I find myself quite schizophrenic when is comes to what looks good for spacing around operators. 'x+5' one day, 'x + 5' the next! And even worse for function arguments 'foo(bar);' 'foo( bar );'. 'if's only the tip of the iceberg ;D
Most styles was two spaces in while/if..., and one for operators, arguments etc. But I never enforced it, and only add parentheses staces from time to time. If we ever do a big translation pass, I will run simutrans also through uncrustify (or similar) to get a similar appearance.
But for new stuff, no space between if( func( and two space before/after logic stuff and single spacing between operators is appreaciated. It think this is still written in the txt. Even though some parts of the old code did not obey it either.
@prissi: Yeah, using crustify is a good idea. After some re-styling I think I don't break any rule now, if you want I can add those extra spaces inside the if expressions near the parenthesis.
Okay, I think the almost-definitive version here (https://dl.dropbox.com/u/30024783/multiple_dirs_v6.patch).
(https://dl.dropbox.com/u/30024783/Untitled26.png)
@Dwachs: I made the scenario dialogs able to load from GAMEBASE/PAK/scenario and USERDIR/scenario . The last one works almost completely, loads the map and the scenario loads without error, but the objectives are missing. I'm passing the scenario_base and scenario_name_ correctly, scenario_base is the qualified path to the directories where that scenario resides, and scenario_base the name of the scenario. Something is not working in that code, can you please check it out and see what's wrong? If it's a problem on my code just tell me I'll fix it.
I also think it whould be a good idea to implement the del_file functionality deleting the scenario folder recursively.
Okay, the path does:
- Add multiple directory support to that frame (currently just used in scenario and heightmap frame).
- Disables delete buttons on extra folder files.
- Uses System Trash bin in Win32.
- Replace all direct I/O in savegame_frame with calls to searchfolder.cc (except remove and Win32 Trash Bin remove).
- It's tested in Visual Studio, MinGW and Ubuntu
- I have fixed Tufit detected bug ( the game won't put the full path in the textbox, just the savegame name ).
Possible improvements:
- Detect a wrong filename in the save dialog, and not wait for the save to fail. ( if includes / or \ or some other character we decide not to allow).
- Give this dialogs UTF-16 support so we can work with files with non 8-bit names (Japanese and the kind). This is a bit harder than it sounds.
Quote from: Markohs on October 18, 2012, 01:03:18 AM@Dwachs: I made the scenario dialogs able to load from GAMEBASE/PAK/scenario and USERDIR/scenario . The last one works almost completely, loads the map and the scenario loads without error, but the objectives are missing. I'm passing the scenario_base and scenario_name_ correctly, scenario_base is the qualified path to the directories where that scenario resides, and scenario_base the name of the scenario. Something is not working in that code, can you please check it out and see what's wrong? If it's a problem on my code just tell me I'll fix it.
I think it is a problem in scenario_t::load_language_file, which does some extra chdir-calls.
Trash bin? Does it make clear that it uses the trash bin? Is there a way to tell it not to (like holding down shift)? I don't like it when programs put stuff in my trash bin without telling me.
Sounds reasonable Ters, I'll see what I can do. Other option is disabling the trash bin in the settings tab.
Quote from: Markohs on October 18, 2012, 08:20:38 AM
Other option is disabling the trash bin in the settings tab.
A player would still have to know that the game will use trash bin unless that setting is modified.
An idea for further development about savegames:
SAVE:
If you enter "test", it saves to USERDIR/save/test.sve
If you enter "test/test", it saves to USERDIR/save/test/test.sve; if USERDIR/save/test/ does not exist, it creates the folder.
LOAD:
Read from USERDIR/save/ and from any of its subfolder
If you delete the last file in a subfolder, the subfolder itself is deleted/moved to recycle bin
The purpose would be to let the player organize better his saves.
Along with this, every folder could have a button to collapse/expand its content; this setting should be saved in settings.xml
Quote from: Ters on October 18, 2012, 08:27:16 AM
A player would still have to know that the game will use trash bin unless that setting is modified.
I think the vast majority of our players will prefer to use the trash bin, so it should be enabled by default. How do you think we should warn our players about the trash bin being used? A tooltip?
Quote from: Fabio on October 18, 2012, 08:54:05 AM
An idea for further development about savegames:
SAVE:
If you enter "test", it saves to USERDIR/save/test.sve
If you enter "test/test", it saves to USERDIR/save/test/test.sve; if USERDIR/save/test/ does not exist, it creates the folder.
LOAD:
Read from USERDIR/save/ and from any of its subfolder
If you delete the last file in a subfolder, the subfolder itself is deleted/moved to recycle bin
The purpose would be to let the player organize better his saves.
Along with this, every folder could have a button to collapse/expand its content; this setting should be saved in settings.xml
mmm... all of those sound reasonable too, I'll think about it. The collapse/expand button is a bit harder to implement through.
Quote from: Markohs on October 18, 2012, 09:14:54 AM
I think the vast majority of our players will prefer to use the trash bin, so it should be enabled by default. How do you think we should warn our players about the trash bin being used? A tooltip?
A tooltip would be good.
If trash bin is enabled, the tooltip hovering the X delete button should say: "Move this file to trash bin" (or recycle bin? I think this is the standard English translation... could it be possible to read its actual name from Windows locale?)
If it's not enabled, you would have "Permanently delete this file".
Quote from: Markohs on October 18, 2012, 09:17:01 AM
mmm... all of those sound reasonable too, I'll think about it. The collapse/expand button is a bit harder to implement through.
Thank you for considering it!
Quote from: Fabio on October 18, 2012, 09:20:16 AM
A tooltip would be good.
If trash bin is enabled, the tooltip hovering the X delete button should say: "Move this file to trash bin" (or recycle bin? I think this is the standard English translation... could it be possible to read its actual name from Windows locale?)
If it's not enabled, you would have "Permanently delete this file".
Or maybe just replacing the "X" with "Delete" or "Move to Trash" in the button so the user doesn't have to hover the mouse over it and it's more obvious.
EDIT: Or maybe even better, replacing the "X" With two possible icons, one with a trash bin, and another with a X over the document.
No, the button would be too big, and deleting a file is not the primary purpose of this dialog.
If possible, I would replace the [X] with an icon like [(http://public.gonella.eu/simutrans/icons/icon_trash.gif)] which would be much clearer IMHO.
Tooltips would be good enough, because they would deliver the info only when needed (i.e. when you move your mouse in order to delete the file).
Icons are perfect, we just need to have two versions of them, one that clearly resembles a trash bin with a recycling image, and another thgat clearly states the item will be destroyed. Do you have any of them? I'll have to add them to the skin pak.
So:
- 1 trash bin with the recycle arrows
- 1 document with a X over it, or a destroying effect, making it clear the document won't be recoverable.
Quote from: Markohs on October 18, 2012, 09:14:54 AM
How do you think we should warn our players about the trash bin being used? A tooltip?
Delete operations should, and generally do, always ask for a confirmation, even if it's not really a delete but a move-to-trashbin operation. Windows uses these confirmation windows to alert the user whether it is about to delete a file or just put it in the trash bin.
Yea, Ters. I think the same way. But as prissi expressed already in this thread (http://forum.simutrans.com/index.php?topic=10612.msg101616#msg101616), confirmation dialogs are not part of the simutrans UI and we are forbidden of use them as designers. So the two icons (plus the tooltips) option is the most reasonable one here I think.
It's very bad design if a slip of a finger deletes a game so that only experts with a bit of luck can revive it. Even if the Windows version only send the game to the trash bin, that's only one platform. I'm not sure how well normal users know the trash bin either.
Personally, I've shunned it ever since 1995, because I delete files to free up space (at least back then). My only intentional use of it is to temporary hold files that might be to important for some program to delete (until I verify that my computer can live without them), or startup shortcuts that I've disabled. (I also delete my old save games from Explorer, not from Simutrans. Actually, I don't delete the games, but the backups, which I also make by copying in Explorer.) I'm not a typical user, though. However, I don't remember seeing my developer colleagues doing anything but Shift+Delete or some equivalent.
By the way, have you checked what happens if you try to move a file to the trash bin, but the trash bin has been disabled (or can't be enabled in the first place, which might be the case for some drives)?
Quote from: Ters on October 18, 2012, 11:11:55 AM
It's very bad design if a slip of a finger deletes a game so that only experts with a bit of luck can revive it.
Maybe, but it's the GUI we have now and we have to be consistent with it. :)
Quote from: Ters on October 18, 2012, 11:11:55 AM
I'm not a typical user, though.
No, I think not. :)
Quote from: Ters on October 18, 2012, 11:11:55 AM
However, I don't remember seeing my developer colleagues doing anything but Shift+Delete or some equivalent.
Windows exporer does that by default, del to move to trash and shift+del to delete persistentally. I don't see it as a so strange option. There is no better alternative I think. Another option whould be showing both icons, the trash delete and the permanent delete on all entries. Whould you like that option more?
Quote from: Ters on October 18, 2012, 11:11:55 AM
By the way, have you checked what happens if you try to move a file to the trash bin, but the trash bin has been disabled (or can't be enabled in the first place, which might be the case for some drives)?
No, didn't tested that. Good idea, I'll have it a look.
Quote from: Markohs on October 18, 2012, 09:55:09 AM
Icons are perfect, we just need to have two versions of them, one that clearly resembles a trash bin with a recycling image, and another thgat clearly states the item will be destroyed. Do you have any of them? I'll have to add them to the skin pak.
So:
- 1 trash bin with the recycle arrows
- 1 document with a X over it, or a destroying effect, making it clear the document won't be recoverable.
I discovered I'm terrible at painting trash bins ;)
Here's two icons:
(http://public.gonella.eu/simutrans/icons/ls-symbols-trash.png)
:down: download (http://public.gonella.eu/simutrans/icons/ls-symbols-trash.png)
taken from here: http://www.brandspankingnew.net/archive/2006/12/hohoho.html
Only they are CC-BY-SA-2.5 (http://creativecommons.org/licenses/by-sa/2.5/) licence, and I don't know if the Share Alike clause is compatible with Simutrans Licence.
Anyway, they can be a starting point for developing and testing and replaced later before release.
On a side note: when the button is disabled, can you gray them out programatically or do you need also two grayed-out versions?
Two icons would be too much. But it would be cool if when shift is pressed the icon turns from trash to remove... probably too hard to do, unfortunately...
Thank you! I'll see what I can do.
Quote from: Ters on October 18, 2012, 11:11:55 AM
that's only one platform
True! But Windows & Mac are unified well enough. I think these provide decent starting points...
http://stackoverflow.com/questions/6776469/moving-files-to-trash-can-in-linux-using-c
http://www.hardcoded.net/articles/send-files-to-trash-on-all-platforms.htm
We might want to split the "recycle bin" sub-thread?
Quote from: Markohs on October 18, 2012, 11:43:02 AM
Another option whould be showing both icons, the trash delete and the permanent delete on all entries. Whould you like that option more?
I'm just worried about hitting the delete button by accident. As I wrote, I do my save game handling in Explorer. Mostly because I make copies of all actively played save games when there has been significant changes to the game, and it's a more efficient to do that in a familiar UI I've used almost daily since 1995.
Having just a trash bin button might be the lesser evil when what I see as the proper solution is out of the question. But it should be a trash bin only if trash bins are supported on that platform. If thrash bins are unavailable, it should be a delete button. That gives at least some indication that the behavior is different if a player should encounter Simutrans on two different platforms.
What about having a button next to each saved game to get more information on it - this would open up a window where you could view a map preview, see information about in-game year, population, vehicle count, and give options to delete, move to recycle bin, rename and so on?
mmm... it's a good idea also. Man, this patch never ends, when I finish one thing 2 new ones come! :) I'll check what can I do.
Please finish this one first :) I would not care, if the delete button disappears.
I'll just replace the delete 'X' button with an image of the trash can or the 'X' depending if the trash bin is usable or not and commit it to svn after the release, yes. After that, when I guess we already have made a release, I'll commit to svn and work on kierongreen's suggestion.
BTW, as Ters sttaed, I'm atually investigating this, but on windows, if yuyou delete a file with UNDO, but the filesystem doesn't have a recyclyng bin (A USB frashdrive or a network drive), The file it's deleted without way back. Checking if I can detect this situation and show a X instead of a trash bin before the delete actually happens.¡
@Dwachs, I commented out the chdirs in scenario_t::load_language_file(const char* filename) and all worked good. I even commented it on current trunk code and it works too, since you already define correctly scenario_path in lines 74 and 75:
Quote
buf.printf("%s%s/", scenario_base, scenario_name_);
scenario_path = buf;
So, why did you put that chdir? it's really something I'm missing? Can I remove them (and the extra include we had to add) and upload to svn?
Honestly, I do not know. If it works without these chdirs, then please remove them :)
I had to extend the button_t class to be able to put images over buttons, I don't know if there is a better option and is implemented somewere, didn't find it myself.
Looks more or less like this, this is still a draft. Anyway comments are ofc welcome. I know the images are not well aligned still, my skills with gimp are not good and it's the frist time I ever used makeobj. ;)
About the grayed out version, I'll try to grey them out programatically, fabio. It will be better like this, so if this buttons are used somewere else will be easier to just require one image.
(https://dl.dropbox.com/u/30024783/Untitled27.png)
Hey, what are my savegames doing in your directory? That's my naming convention. ;D
I can't think of anyplace in the code with images on buttons, excepting the window titlebar, but those aren't really buttons... Comments on what you've done difficult without a patch! But the screenshots pretty.
When would these buttons ever be grayed out?
I trust Shift will bypass the trash as per expected trashbin convention?
Quote from: TurfIt on October 21, 2012, 11:59:09 PM
Hey, what are my savegames doing in your directory? That's my naming convention. ;D
Ahahahahaha :D
Quote from: TurfIt on October 21, 2012, 11:59:09 PM
I can't think of anyplace in the code with images on buttons, excepting the window titlebar, but those aren't really buttons... Comments on what you've done difficult without a patch! But the screenshots pretty.
I'll post the patch soon here, atm the code is too ugly to show. :)
Quote from: TurfIt on October 21, 2012, 11:59:09 PM
When would these buttons ever be grayed out?
Didn't decide yet. but I guess on the base (default) directory they will be enabled. e.g. $USERDIR/maps and disabled extra dirs, $PROGRAMDIR/$PAKDIR/maps .
Applied to savegames, they will never be disabled, will show a trash bin on windows and a X on the rest of systems.
Even if I decide to not disable I'll implement it anyway I think, for the case somebody decides to make use of those image buttons.
Quote from: TurfIt on October 21, 2012, 11:59:09 PM
I trust Shift will bypass the trash as per expected trashbin convention?
Yep, but won't change the images when you press shift, that whould be a lot of work I think.
BTW, it whould be nice to create a image that represents a "Load with addons" button, since this code is also used in pakselector. Any ideas?
This new button displays a roundbutton, the image aligned to the left, and optional text to the right.
Or just not show the buttons instead of disabling/greying. Extend the size of the filename display.
Dynamic update with shift might be simple - use of event_get_last_control_shift() within ::zeichnen(). Used elsewhere.
Any idea for the "load with addons" button image, Fabio?
You are the artist!! :)
(https://dl.dropbox.com/u/30024783/Untitled28.png)
What does exactly Load with addons do?
Is it an option? If so, I would rather see a checkbox than a button... ???
Well that button already exists in simutrans, I didn't created it. It allows you to load the pak files in $PROGRAMDIR/addons/$PAKNAME or not. And right now it's a button. Do you install your addons in the $PROGRAMDIR/$PAKNAME directory straight? That's how most people do, I guess, but the addons/ folder is the way to go imho.
Please do not put too much stuff into this patch. Buttons with images are not needed for the multiple directory patch. Same goes for two different images for file deletion. (Apart from being most confusing.) Rather show the button only when possible and then do either trashcan (on windows / Mac OS) or fdelete. No need for differenciating. I am very much against blowing up code for very limited gain.
Considering all the other things in simutrans, only one user has ever complained about the delete function. Given the various other pitfalls, two icons instead of the x will rather case more confusion than help. If in doubt, add a tooltip which explain whatever action will result.
mmm... saw this coming. :)
Okay, no imagebuttons then. I'll just take the standard (text, with a X) button and will just implement the shift modifier plus a tooltip. Will submit after the release.
Was fun implementing them anyway, learned quite a lot about the gui with this experiments. I'll go back to the OpenGL backend, have a Ters patch somewere in my HDD that I want to get my hands over. I'll squeeze some fps more and make it work properly. I really think it will go faster than the current 2D engine.
Anyway, I really think we should completely revamp the GUI some day, it looks not good imho, too simple and obsolete. But you are right, that's a completely different project and outside the scope of this patch.
EDIT: The complete patch here (https://dl.dropbox.com/u/30024783/multiple_dirs_v7.patch). Ill submit it to svn after release if nothing extra is requested. (might have wrong CRLF, will check on submit)
Notes:
- Had to use slist_tpl to save the internal list, it's not efficient because on inserts it has quadratic cost, but given we'll have about 50 entries at maximum should be no problem. I'd have used a vector, but vector_tpl doesn't provide a method to insert on the array by iterator, just by index, so I'd have had to change the algorithm to not use FOR( . I think it's ok like this.
- On Windows, uses trash bin, SHIFT+CLICK on the delete button deletes the file.
- Directories will be listed even if empty, gives the user information of where it's looking for files, and notes wich are empty. I think it's ok like this too.
- I re-styled the code, should conform to the coding style.
- Just scenario and heightmap-selectors are multidirectory now. For pak selector to be multidirectory whould require some modification in the code on how umgebung_t::objfilename is handled, similar changes to what Dwachs made to handle base_directory and scenario_name.
- Savegame multidirectory loading/saving makes not much sense in multidirectory if we don't implement the changes Fabio suggested to make user able to create/delete dirs from this UI.
Suggestion: Do not show path title for empty directories
Also add <string> to the savegame_frame.h to actually compile this.
Ok, done (both)
Last version. (https://dl.dropbox.com/u/30024783/multiple_dirs_final.patch)
here is an update: killed some compiler warnings, changed typo NIMINMAX.
I changed the place, where scenarios are looked up: As scenarios are bound to a certain pakset, these scenarios are looked up in addons/pak-whatever/scenario/ instead simply scenario/ .
Imho you can commit it.
Thanks, Dwachs. :) Uploaded at 6024.
Somehow this patch is not finisched: It does not show correct (complete) pathes and furthermore, it show even patches if thes is only one path. I though you wanted to fix this. At least the superflous entry when only one is there I removed.
Looking close at your code I have some questions about coding style. What kind of comment is "/*!" which is new to me. Since those functions are anyway no completely documented (@param etc) would be a normal comment "//" not better for a single liner? Same for \author, should it be not rather @author?
And please use types with _t at the end and a useful name. entry was not a good type name; chances are high that this is used already elsewhere or will be used. dir_entry_t is a good name (in simutrans conventions) as it carries the notion of being a type and has to do with directories. (I know that some basic things like "koord" defies this convention.)
And a really stupid question: At which point the entries will be freed. Is this automatically when the slist is destroyed?
About the complete paths, I left relative paths on all the dialogs on purpose, I thought it was more understandable to our users to see "save/" than "C:\users\Markohs\Documents\Simutrans\save", another option whould have been showing coloured "USERDIR\save" or something similar. I can change it, it all comes for example from here:
loadsave_frame.cc, line 73:
loadsave_frame_t::loadsave_frame_t(karte_t *welt, bool do_load) : savegame_frame_t(".sve",false,"save/")
Compared to:
scenario_frame.cc, line 46
scenario_frame_t::scenario_frame_t(karte_t *welt) : savegame_frame_t(NULL, true, NULL, false)
{
static cbuffer_t pakset_scenario;
static cbuffer_t addons_scenario;
pakset_scenario.clear();
pakset_scenario.printf("%s%sscenario/", umgebung_t::program_dir, umgebung_t::objfilename.c_str());
addons_scenario.clear();
addons_scenario.printf("addons/%sscenario/", umgebung_t::objfilename.c_str());
this->add_path(addons_scenario);
this->add_path(pakset_scenario);
I can use absolute paths on all dialogs if you think it's better. It just needs some tweaking.
About not showing the header if there is just one section, I can fix it too, I thought you only refereded to remove headers on empty sections. Fixing it.
About the naming convention, you are right, I'll fix it too.
The entries are destroyed on the slist deletion, yes, I used memcheck to check for memory leaks at some point of development and found none, I think all is working good.
savegame_frame_t::~savegame_frame_t() destroys the slist and frees the char buffers if they exist, on void savegame_frame_t::fill_list() you can see:
void savegame_frame_t::fill_list()
{
const char *suffixnodot;
searchfolder_t sf;
...
FOR(searchfolder_t, const &name, sf) {
fullname = new char [path_c_len+strlen(name)+1];
sprintf(fullname,"%s%s",path_c,name);
if(check_file(fullname, suffix)){
if (!section_added) {
add_section(path);
section_added = true;
}
add_file(fullname, name, get_info(fullname), not_cutting_extension);
}
else{
// NOTE: we just free "fullname" memory when add_file is not called. That memory will be
// free'd in the class destructor. This way we save the cost of re-allocate/copy it inside there
delete fullname;
}
}
About the search pathes, the class doesn't free them, that's why the are declared as static variables, so they have a persistant memory storage area.
Quote from: prissi on November 06, 2012, 09:50:30 PM
Looking close at your code I have some questions about coding style. What kind of comment is "/*!" which is new to me. Since those functions are anyway no completely documented (@param etc) would be a normal comment "//" not better for a single liner? Same for \author, should it be not rather @author?
Oh, I forgot this. Well, I tried to follow more or less what we talked about but never ended fixing in http://forum.simutrans.com/index.php?topic=4672.125 (http://forum.simutrans.com/index.php?topic=4672.125) , /** it's exactly the same than /*! , and yes, I should have sticked to /** and @param and not \param, since it's way more common in the rest of the code. Changing this too. Even I did not comment 100% of the functions, I did comment many, and I'd let /** stay and not //, since doxygen won't pick //
You can see what does it generate in:
https://dl.dropbox.com/u/30024783/doxgui/html/classsavegame__frame__t.html (https://dl.dropbox.com/u/30024783/doxgui/html/classsavegame__frame__t.html)
EDIT: I see you already changed some of the things you commented me here, I'll fix the rest soon. :)
I though doxygen would pick up /// for one liner. Well, I hacked a removal of the sole entry into the code. Not sure, if this was the right place, though.
About the idea with "PROGRAM_DIR"+"\save" etc. with different colors: That seem are very good idea.
Before doing stuff, just use the latest revision!
USERDIR and PROGRAM_DIR aren't pretty on the eyes, and I suspect that is especially not so for those that are not computer experts. It should follow the nomenclature for the platform, such as "My documents" or "Documents" for the former on Windows. (I wonder if most non-hardcore Windows users know what c:\ is nowadays.)
Quote from: Ters on November 07, 2012, 05:46:18 AM
It should follow the nomenclature for the platform, such as "My documents" or "Documents" for the former on Windows.
I agree. I wonder if there's a way to retrieve the localized denomination for those from some API.
Yes, Fabio, it's possible and I'll do it that way. Problem will be wide char languages I think, dunno how will it work in japanese. Making some tests.
On unix-like platforms "~user/" will be clear enough I guess
Quote from: Markohs on November 07, 2012, 12:59:54 PM
On unix-like platforms "~user/" will be clear enough I guess
You could also use ~markohs/ grabbing the active user (but I don't usually use *nix systems, so I might be wrong).
well but then for the program dir?
MAybe rather have two text for translator "PROGRAM_DIR" "USER_DIR" or maybe "HOME_DIR" "MY_DOCUMENT_DIR" for Unix/mac. Also "Programme" or "Program files (x86)" is not neccessarily the location of the program folder
To finish this I'd like to add some code that might not work in old Windows 98/95 machines, are we still giving support to those platforms? The are more than 14 years old...
The code in question is:
CHAR szPath[MAX_PATH];
SHGetFolderPathA(NULL, CSIDL_MYDOCUMENTS, NULL, 0, szPath);
SHFILEINFOA sfi = {0};
SHGetFileInfoA(szPath,-1,&sfi,sizeof(sfi), SHGFI_DISPLAYNAME);
printf("%s %s", szPath, sfi.szDisplayName);
That results in "C:\Users\Markohs\Documents" and "My Documents" in my w7 system (I have it in english).
For program dir I'd just print "Simutrans Installation" or some fixed string that whould be translated in each language. For user_dir I'd use the code above in windows, and ~/simutrans/save in linux.
Looks like MSDN labels everything as from 2000 Pro/Server or later, even functions I know have been around "forever". But if it compiles with #define WINVER 0x400 (which seems to be the default in the headers for mingw32), it should work on Win95.
oh, great! I guess they did that in msdn because those platform are outside of the supported platforms. thank you ters.
I'll change it then, using regedit in the entry refered in dr_queryhomedir (or similar) warns that those entries are deprecated.
The results in "C:\Documents and Settings\prissi\My Documents\simutrans" in windows XP. Not sure if this can be fit into the dialoge. Maybe some shorten of paths "C:\Docu...si\My Documents\simutrans" (i.e. take out x characters from the 8th onwards in steps of five until width fits dialog width.)
ShellGetFolderA will not return the user path you want. In this form it will only work in Win2000, in Xp and higher it will return the user path of the default users. It must be -1 for XP and up for the current user. See http://msdn.microsoft.com/en-us/library/windows/desktop/bb762181%28v=vs.85%29.aspx
Even ignoring everything before "My Documents" would do the job, as the full path doesn't matter much in Windows systems. Thus "My documents\simutrans" should be enough.
SHGetFileInfoA doesn't translate C:\Documents and Settings\prissi\My Documents to "My Documents"?
The idea is that SHGetFileInfoA turns "C:\Documents and Settings\prissi\My Documents" or "C:\Users\prissi\Documents" to "My documents", at least in Windows 7.
I've tested that code in XP too and works, can anybody test it too plz?
Download (https://dl.dropbox.com/u/30024783/testpath.exe), might need install of http://www.microsoft.com/en-us/download/details.aspx?id=30679 (http://www.microsoft.com/en-us/download/details.aspx?id=30679)
On XP in spanish I get:
Quote
SHGetFolderPathA: C:\Documents and Settings\Administrador\Mis documentos SHGetFileInfoA: Mis documentos
On windows 7 english:
Quote
SHGetFolderPathA: C:\Users\marcos\Documents SHGetFileInfoA: My Documents
I just made a console Win32 applicattion in VS2010 and the code is:
Quote
// testpath.cpp : Defines the entry point for the console application.
//
#include "stdafx.h"
int _tmain(int argc, _TCHAR* argv[])
{
CHAR szPath[MAX_PATH];
SHGetFolderPathA(NULL, CSIDL_MYDOCUMENTS, NULL, 0, szPath);
SHFILEINFOA sfi = {0};
SHGetFileInfoA(szPath,-1,&sfi,sizeof(sfi), SHGFI_DISPLAYNAME);
printf("SHGetFolderPathA: %s SHGetFileInfoA: %s", szPath, sfi.szDisplayName);
}
It does not return the path to my files on the first hand, since in XP and later you have to pass -1 as third parameter to SHGetFolderPathA.
Also maybe the reduction does not work because my "Dokumente" folder is on another disk. That is not true for default user though. Even more, if installed protable, the file will not be in my document anyway but in the simutrans folder. (Of course you know this.) Even "user_dir" could point quite well to other locations than my document.
Strange, works here in my computer, and passing -1 as third parameter will return the documents folder of the default user, that's not what we want... I'll test passing it the user as the third parameter. I'll move my documents folder too to test.
When you refer to the portable installation do you refer to "-singleuser" param and 'multiuser = !(contents.get_int("singleuser_install", !multiuser)==1 || !multiuser); ' in simmain.cc?
How did you changed the my documents folder prissi? I did change on explorer->right-click->properties->change destination and accept. And SHGetFolderPathA returned the correct path (with third parameter NULL), and FileInfoA showed the name of the directory:
(https://dl.dropbox.com/u/30024783/t.png)
Using PIDL looks like it's working better. Even it works quite different across windows versions.
Prissi, using -1 as third parameter won't work, that will return the default user document path, and that's not what we want. NULL is working good here.
Quote
CHAR szPath[MAX_PATH];
SHGetFolderPathA(NULL, CSIDL_MYDOCUMENTS, NULL, 0, szPath);
LPITEMIDLIST pidl=NULL;
if (!SUCCEEDED(SHGetFolderLocation(NULL, CSIDL_MYDOCUMENTS, (HANDLE) -1, 0, &pidl)) ){
printf("errr");
}
SHFILEINFOA sfi = {0};
if (!SUCCEEDED(SHGetFileInfoA((LPCSTR)pidl,-1,&sfi,sizeof(sfi), SHGFI_PIDL| SHGFI_DISPLAYNAME) )) {
printf("errr");
}
printf("SHGetFolderPathA: %s SHGetFileInfoA: %s", szPath, sfi.szDisplayName);
Windows 8 SPANISH (strange, FileInfoA doesn't translate)
SHGetFolderPathA: C:\Users\marcos\Documents SHGetFileInfoA: Documents
Windows 7 english:
SHGetFolderPathA: C:\Users\marcos\Documents SHGetFileInfoA: Documents
Windows XP Spanish:
SHGetFolderPathA: C:\Documents and Settings\Administrador\Mis documentos SHGetFileInfoA: Mis documentos
Windows XP Spanish moving My documents folder:
SHGetFolderPathA: E:\SIMUTEST SHGetFileInfoA: Mis documentos
Updated the binary if you want to test. https://dl.dropbox.com/u/30024783/testpath.exe (https://dl.dropbox.com/u/30024783/testpath.exe)
EDIT: NOTE: Windows 7 and 8 in the registry key Software\\Microsoft\\Windows\\CurrentVersion\\Explorer\\Shell Folders (the one we use now) has an entry saying:
!Do not use this registry key
Use the SHGetFolderPath or SHGetKnownFolderPath function instead
So might be time to change this anyhow.
Windows 7 Czech: Documents
(might be Dokumenty...)
Anyway, where are you planning to put this code? And what about the case when userdir != My documents? (Like for portable installations?)
All those OS specific stuff should go into simsys.cc. (Same is true for the for the deletion too).
The PC here at work is english, so for now I cannot comment.
Right now we have:
umgebung_t::user_dir
umgebung_t::program_dir
I planned to add:
umgebung_t::display_user_dir
umgebung_t::display_program_dir
And use the second ones when printing paths, instead of the first ones. For the file open/save dialogs and anywere it's applicable.
Yes, I planned to implement those in simsys.cc
I also thought it might be a good idea creating utils/fileutils.cc and implement all that OS specific functions there, what do you think?
The portable installation is already coded and don't plan to modify it, the same if can apply. Will just query for display_user_dir if not multiuser.
in simmain.cc
// init dirs now
if(multiuser) {
umgebung_t::user_dir = dr_query_homedir();
}
else {
// save in program directory
umgebung_t::user_dir = umgebung_t::program_dir;
}
I really do not see the purpose of a display name. Sorry this is a stupid concept introduced by Mircrosoft. Those name translation just adds confusion. And it is keeping me stupid, which I detest.
Thus would prefer a generic term like "USER_DIR" or the real full path.
Furthermore on any portable installation and when not installing into program files (anyway recommended), you will see the full dir of simutrans and "My documents". Making stuff just for one platform is no progress and will rather add confusion on multi platform programs. Not to mention systems like Android or Haiku, which effectively are single user OS.
M.... okay, will do it that way.
It's the simplest and way of doing it anyway, much less code to write. Using "USER_DIR" and "SIMUTRANS_DIR" then.
Is this ok then?
(https://dl.dropbox.com/u/30024783/Untitled29.png)
Isn't USER_DIR just a display name also? And I doubt less confusing, though perhaps to different groups. Personally, I just need the path relative to the save game directory. No need to prefix it with anything. That just wastes space.
It might be a stupid Microsoft invention, but on a Microsoft OS it's the convention AND what the common user expects to find.
E.g. if I want to copy my saves on a pen drive to play them on another pc, I, user, would check where the files are saved and go browsing them.
If I see My documents, I know where they are, if I see c:\... real location, I might be able, too.
But USER_DIR will be unintelligible for most.
So in the end it is rather "pre-installed" or "user supplied" files. Since one usually will never see those paths except for scenarios (since normally simutrans saves everything else in one path) maybe rather just leave the full path.
I am torn. The program path will be ugly on many systems and especially on portable devices. Also program and user path will most likely end with "simutrans".
If more user prefer "my document" ok let it be it.
EDIT: On windows 7 (german) the name shown by testpath.exe is "Documents" which is neither english nor german. Sounds not helpful to me, as normally this folder is called now "Bibliotheken\Dokumente" by Windows 7. Thus "Documents" will rather add to confusion as it exists nowhere in the system.
Given this, I vote for the full path or "local settings" or similar.
Yeah, it's a non trivial decision, what Fabio and Ters say makes sense, but I can understand your reasoning too. I think I'll submit it with SIMUTRANS_DIR and USER_DIR prefixes and you take the final decision, after all you are the boss. ;)
Quote from: prissi on November 12, 2012, 08:33:16 PM
EDIT: On windows 7 (german) the name shown by testpath.exe is "Documents" which is neither english nor german. Sounds not helpful to me, as normally this folder is called now "Bibliotheken\Dokumente" by Windows 7. Thus "Documents" will rather add to confusion as it exists nowhere in the system.
Documents should be correct name in English on Windows Vista/7 (before that, it would have been My Documents). That it is not translated might have something to do with how, or rather at which level, these things are translated in Vista/7. (More specifically, one might say that Documents is the display name for the directory, but not for the folder. The distinction between directories and folders have become more prominent from Vista onward.)
By the way, SHGetFolderLocation seems to be Windows 2000 and later only. CSIDL_MYDOCUMENTS is for the virtual folder that is a combination of My Documents and Shared Documents, and therefore only available on Windows Vista and later.
A possibility could be to add one separator in the dialog for files form the user directory, and another for files from the simutrans directory, with subseparators under that containing only the relative paths for directories below that, and file buttons below that just like now. Might be too complicated and/or cluttered, though.
On german XP and window 2000 it returned indeed "Dokumente" which was the expected name. Thank Mircrosoft for adding confusion with every new version. It also does not solve the unix/Mac/haiku problem. Luckily for now it only affects scenarios.
In the end I leave it to makohs, since he spent most of the time on it.
CSIDL_PERSONAL might be better than CSIDL_MYDOCUMENTS, will test tomorrow on the computer where I have the virtual machines.
I just commited the patch with prefixes @6044 , I'll update it if we get a better idea of what we want.
(https://dl.dropbox.com/u/30024783/Untitled30.png)
Did you define WINVER to something for those example snippets you posted on page 5? I had to, or the Win2000+ stuff wouldn't compile.
That code didn't make to repository at the end because of the lack of a reliable way of getting the My documents folder name.
Right now the old method is used:
Quote
DWORD len = PATH_MAX - 24;
HKEY hHomeDir;
if (RegOpenKeyExA(HKEY_CURRENT_USER, "Software\\Microsoft\\Windows\\CurrentVersion\\Explorer\\Shell Folders", 0, KEY_READ, &hHomeDir) != ERROR_SUCCESS)
return 0;
RegQueryValueExA(hHomeDir, "Personal", 0, 0, (BYTE*)buffer, &len);
strcat(buffer,"\\Simutrans");
It's implemented in this code at the end:
Quote
size_t path_len = strlen(umgebung_t::program_dir);
if (strncmp(name.c_str(),umgebung_t::program_dir,path_len) == 0) {
sprintf(label_text,"%s %s/%s", translator::translate("Files from: "), translator::translate("SIMUTRANS_DIR"), name.c_str()+path_len);
}
else {
sprintf(label_text,"%s %s/%s", translator::translate("Files from: "), translator::translate("USER_DIR"), name.c_str());
}
Quote from: Markohs on November 12, 2012, 11:58:23 PM
CSIDL_PERSONAL might be better than CSIDL_MYDOCUMENTS, will test tomorrow on the computer where I have the virtual machines.
I just commited the patch with prefixes @6044 , I'll update it if we get a better idea of what we want.
(https://dl.dropbox.com/u/30024783/Untitled30.png)
Hmm, it is totally unclear what USER_DIR and SIMUTRANS_DIR is. It is actually quite annoying. You would like to see its full directory name, but that might get too long. Is it perhaps an idea to make buttons of USER_DIR and SIMUTRANS_DIR? If you click on it you will see the full name of that directory. Or similar functionality in another way?
A tooltip over it so the full path is shown when you mouseover is enough?
It's hard to reach a point everybody agrees, so maybe just a tooltip will be enough, and let USER_DIR and SIMUTRANS_DIR like it's now.
A bit hidden, but a tooltip would not be bad, I guess. It won't pollute the dialog anyway.
At this point I would prefer a shortened full path like c:\...\simutrans\maps with the tooltip showing the full path.
Looks like we are finally reaching a consensus. :)
So fullpath (shortened) with a tooltip?
For me it would be splendid.
I just wonder if it could be doable a link (clicking on the path like a hyperlink or an arrow button) to invoke the OS shell to open that folder.
Quote from: Fabio on November 13, 2012, 03:52:31 PM
I just wonder if it could be doable a link (clicking on the path like a hyperlink or an arrow button) to invoke the OS shell to open that folder.
Doable, but I doubt prissi will see this as a good idea, let's hear what the people say. ;)
I think invoking an OS shell will be difficult on Linux (and other open *NIXes), because that OS has no standard shell. Simutrans might have to target the different GUI frameworks directly to use their standard dialogs.
The
<drive:>\..\<end 1-2 folders>
format is probably the cleanest and easiest to understand from my point of view. Tooltip would be more than sufficient to get the actual path.
I wrote actually the shortened path about.
If the text (proportional_len) is too long.
Take the first 8 letters (to know about where you are).
Add three dots and then remove letters for number 9 on until its fits.
The tooltip must be done manuall, i.e. during ist getroffen event check.
Sorry to bring this alive again, but I was writing the shortened path code now and reading prissi's comment:
Do you mean I need to take savegame_frame_t since it inherits from gui_frame_t and re-define 'getroffen' to show the tooltip too, apart from returning if it's inside the window or not too?
Whoudn't be better to extend gui_label_t to have tooltips? No idea, just asking. :)
Okay, last update to this post. This patch is finished and uploaded to svn in a few minutes, implemented the tooltips that you requested.
I want to remind pak creators you now have the possibility to add heightmaps inside the pak folder, that will be listed with the "maps" directory in the user dir too.
(https://dl.dropbox.com/u/30024783/Untitled36.png)