Question about GUI and directory access.

[Markohs]

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.

[Fabio]

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

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!

[Markohs]

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.

[Fabio]

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 [] 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).

[Markohs]

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.

[Ters]

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.

[Markohs]

Yea, Ters. I think the same way. But as prissi expressed already in this thread, 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.

[Ters]

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

[Markohs]

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.

I'm not a typical user, though.

No, I think not.

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?

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.

[Fabio]

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:



download

taken from here: http://www.brandspankingnew.net/archive/2006/12/hohoho.html

Only they are CC-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...

[Markohs]

Thank you! I'll see what I can do.

[VS]

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?

[Ters]

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.

[kierongreen]

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?

[Markohs]

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.

[Dwachs]

Please finish this one first I would not care, if the delete button disappears.

[Markohs]

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

[Markohs]

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

   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?

[Dwachs]

Honestly, I do not know. If it works without these chdirs, then please remove them

[Markohs]

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.

[TurfIt]

Hey, what are my savegames doing in your directory? That's my naming convention. 

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?

[Markohs]

Hey, what are my savegames doing in your directory? That's my naming convention. 

Ahahahahaha

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.

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.

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.

[TurfIt]

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.

[Markohs]

Any idea for the "load with addons" button image, Fabio?

You are the artist!!

[Fabio]

What does exactly Load with addons do?
Is it an option? If so, I would rather see a checkbox than a button...

[Markohs]

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.

[prissi]

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.

[Markohs]

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

[prissi]

Suggestion: Do not show path title for empty directories

Also add <string> to the savegame_frame.h to actually compile this.

[Markohs]

Ok, done (both)

[Markohs]

Last version.

[Dwachs]

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.

[Markohs]

Thanks, Dwachs. Uploaded at 6024.

[prissi]

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?

[Markohs]

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:

Code Select Expand


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

Code Select Expand


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:

Code Select Expand


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.