Question about GUI and directory access.

[Dwachs]

Please continue brainstorming over there:

http://forum.simutrans.com/index.php?topic=10612.0

[Markohs]

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.

[Markohs]

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, 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.

[Dwachs]

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.

[Markohs]

documentation/coding_styles.txt states :

- 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:

/*
* 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.

[prissi]

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.

[Markohs]

Can somebody please try to compile simutrans with mingw and this 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.

[TurfIt]

Code Select Expand


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


[Markohs]

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?

[Markohs]

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.

[Ters]

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.

[Markohs]

 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.

[TurfIt]

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.

[Markohs]

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.

[TurfIt]

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...

[Ters]

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.

[Markohs]

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.

[Markohs]

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:

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  ) {

  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!

Use spaces in for loops like in ifs:
for(  int i=0;  i<10;  i++  ) {

[Dwachs]

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:

Code Select Expand


+ 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).

[Markohs]

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:

Code Select Expand


+ 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.

[Dwachs]

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.

[Markohs]

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.

[Dwachs]

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.

[Markohs]

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. Still has issues on savegames.

[TurfIt]

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.

[Markohs]

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.

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

if( x ) {
}

one on comparision

if( x < 5 ) {
}

2 on logical:

if( x  &&  NULL  ) {
}

and, following the same as above, that whould end in:

if( x == NULL  &&  y == 1 ) {
}

Is this ok? No exceptions.

[TurfIt]

Only problem is that style's not used anywhere! 
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 

[prissi]

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.

[Markohs]

@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.



@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.

[Dwachs]

@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.

[Ters]

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.

[Markohs]

Sounds reasonable Ters, I'll see what I can do. Other option is disabling the trash bin in the settings tab.

[Ters]

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.

[Fabio]

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

[Markohs]

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?