bugFreeciv - Bugs: bug #16887, do not list files twice in the...

 
 
Show feedback again

bug #16887: do not list files twice in the save dialogs

Submitted by:  Matthias Pfafferodt <syntron>
Submitted on:  Mon 18 Oct 2010 03:08:10 PM UTC  
 
Category: generalSeverity: 3 - Normal
Priority: 5 - NormalStatus: None
Assigned to: Matthias Pfafferodt <syntron>Open/Closed: Open
Release: Operating System: None
Planned Release: 

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Fri 20 Jul 2012 12:08:55 AM UTC, comment #17:

> Part 2 will be raised as new ticket if needed.


There's bug #19168 for handling mapimg save dir now.

Marko Lindqvist <cazfi>
Project Administrator
Sat 13 Nov 2010 08:38:53 PM UTC, comment #16:

This patch is only about part 1 as listed in comment #5. It prevents the client(s) to list all files twice (once with absolute and once with relative path). See file #11108.

Part 2 will be raised as new ticket if needed.

Matthias Pfafferodt <syntron>
Project MemberIn charge of this item.
Wed 03 Nov 2010 10:55:28 PM UTC, comment #15:

This patch is now splitted into two parts:

1. generate a full path from a given relative path

  • remove all mapimg dependend code
  • only expansion of ~/ to the home directory on *nix systems

2. add get_mapimg_dirs()

  • defines the directories used to save map images
  • defaults to the home directory; if this is not available use the save dirs
  • this will be moved into another ticket

(file #11107, file #11108)

Matthias Pfafferodt <syntron>
Project MemberIn charge of this item.
Wed 03 Nov 2010 10:43:30 PM UTC, comment #14:

> ~/.freeciv [and its analog on other platforms] is not perfect,
> but in the absence of a better place, i think it'll do.


This would mean using get_save_dirs() for the client. Furthermore, the server needs a check of the path for the saved image file ...

> There is a chance for the user to select another directory,
> right?


Yes

Matthias Pfafferodt <syntron>
Project MemberIn charge of this item.
Thu 28 Oct 2010 07:57:39 PM UTC, comment #13:

~/.freeciv [and its analog on other platforms] is not perfect, but in the absence of a better place, i think it'll do. There is a chance for the user to select another directory, right?

David Lowe <doctorjlowe>
Thu 28 Oct 2010 02:05:44 PM UTC, comment #12:

> Just a question more: Is the user expect the image to save in
> the current directory or in the home directory (as for logs
> e.g.)?


I do not know. Perhaps some other opinions are needed. Where do you expect a program to save images? (current directory, home directory or ...)

Matthias Pfafferodt <syntron>
Project MemberIn charge of this item.
Sat 23 Oct 2010 11:59:09 AM UTC, comment #11:

Just a question more: Is the user expect the image to save in the current directory or in the home directory (as for logs e.g.)?

pepeto <pepeto>
Project Member
Wed 20 Oct 2010 04:34:42 PM UTC, comment #10:

> > Thus, it should return NULL for all other operating systems.
> > Is this OK:
>
> Yes. Note that the definition of init should be pushed inside
> the conditional block to avoid compilation warnings.


As the function is only used inside of shared.c the definition is now static (and inside a #ifdef).

changes:

  • define current_dir() as static within shared.c and protect it by #ifdef's
  • rename get_current_dir() to get_mapimg_dirs()

(file #10848)

Matthias Pfafferodt <syntron>
Project MemberIn charge of this item.
Wed 20 Oct 2010 12:19:48 PM UTC, comment #9:

> Thus, it should return NULL for all other operating systems. Is
> this OK:


Yes. Note that the definition of init should be pushed inside the conditional block to avoid compilation warnings.

> This function is used for fileinfolist_infix() which needs an
> string vector as argument. It could be renamed to
> get_mapimg_dirs() which
>
> * uses the current directory on *nix
> * uses the directories returned by get_save_dirs() on other
> systems
>
> The mapimg file saving from the client is special as it is
> handled by the client and not by the server as are save games.


This looks a good compromise to me.

pepeto <pepeto>
Project Member
Wed 20 Oct 2010 12:08:14 PM UTC, comment #8:

> Maybe, I haven't be clear enough. As current_dir() is a part
> of the public interface, it must work in any environment. If
> it is used in Windows, it will probably produce an error.
> #ifdef must be added inside this function.


I did add some ifdef's to the function but they are not included in the patch. Before I do it again, I would like to discuss that is needed. At the moment the function current_dir() will only work for *nix systems. Thus, it should return NULL for all other operating systems. Is this OK:

> get_current_dir() doesn't make sense. The current directory is
> a single directory, so there is no reason to return a string
> vector.


This function is used for fileinfolist_infix() which needs an string vector as argument. It could be renamed to get_mapimg_dirs() which

  • uses the current directory on *nix
  • uses the directories returned by get_save_dirs() on other systems

The mapimg file saving from the client is special as it is handled by the client and not by the server as are save games.

Matthias Pfafferodt <syntron>
Project MemberIn charge of this item.
Wed 20 Oct 2010 11:39:13 AM UTC, comment #7:

Maybe, I haven't be clear enough. As current_dir() is a part of the public interface, it must work in any environment. If it is used in Windows, it will probably produce an error. #ifdef must be added inside this function.

get_current_dir() doesn't make sense. The current directory is a single directory, so there is no reason to return a string vector.

pepeto <pepeto>
Project Member
Wed 20 Oct 2010 11:13:14 AM UTC, comment #6:

updated patch:

  • include get_current_dir() from gna patch #2019
  • cleanup #ifdef's

(file #10845)

Matthias Pfafferodt <syntron>
Project MemberIn charge of this item.
Tue 19 Oct 2010 10:11:53 PM UTC, comment #5:

If current_dir() is not portable, it should also be conditional.

Not that:
can be simplified by:

pepeto <pepeto>
Project Member
Tue 19 Oct 2010 09:57:08 PM UTC, comment #4:

> Why returning as a read-or-write data?


Done; see below

> Please at least add comment to "if (tok[0] != '/')" that "/"
> is not root directory in some environments. (In windows
> absolute path would start with letter as in "C:")


Done; see below

> Also check that we don't already define such a string as a
> macro somewhere. IIRC we have PATH_SEPARATOR at least ('/'
> or '\') which you fail to use in this patch btw.


PATH_SEPARATOR is the separator between different paths, i.e. ";" (windows) or ":" (linux)

updated patch:

  • current_dir(), user_home_dir() and user_username() return const data
  • the check is only defined for *nix systems

(file #10828)

Matthias Pfafferodt <syntron>
Project MemberIn charge of this item.
Tue 19 Oct 2010 03:11:46 PM UTC, comment #3:

Please at least add comment to "if (tok[0] != '/')" that "/" is not root directory in some environments. (In windows absolute path would start with letter as in "C:") Also check that we don't already define such a string as a macro somewhere. IIRC we have PATH_SEPARATOR at least ('/' or '\') which you fail to use in this patch btw.

Marko Lindqvist <cazfi>
Project Administrator
Mon 18 Oct 2010 09:39:51 PM UTC, comment #2:

> Is this really portable?


It is not portable. For non-unix systems or systems without the environment variable $PWD the current behaviour is not changed. I have no knowledge about such systems to get the current directory there.

> Why returning as a read-or-write data?


No reason - it chould be const char *.

Matthias Pfafferodt <syntron>
Project MemberIn charge of this item.
Mon 18 Oct 2010 08:06:15 PM UTC, comment #1:

Is this really portable? Why returning as a read-or-write data?

pepeto <pepeto>
Project Member
Mon 18 Oct 2010 03:08:10 PM UTC, original submission:
  • do not duplicate file names if one path is given as relative and as full path
  • see also gna patch #2019
Matthias Pfafferodt <syntron>
Project MemberIn charge of this item.

 

(Note: upload size limit is set to 1024 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

 

Depends on the following items: None found

Digest:
   patch dependencies.

 

Carbon-Copy List
  • -unavailable- added by doctorjlowe (Posted a comment)
  • -unavailable- added by cazfi (Posted a comment)
  • -unavailable- added by pepeto (Posted a comment)
  • -unavailable- added by syntron (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 15 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 13 Aug 2014 12:11:54 AM UTCcazfiStatusReady For Test=>None
      Planned Release2.4.0=>
    Sat 13 Nov 2010 08:38:53 PM UTCsyntronAssigned toNone=>syntron
      Summarygenerate a full path from a given relative path=>do not list files twice in the save dialogs
    Wed 03 Nov 2010 10:55:28 PM UTCsyntronAttached File-=>Added 20101103-trunk-add-get_mapimg_dirs.patch, #11107
      Attached File-=>Added 20101103-trunk-generate-a-full-path-from-a-given-relative-path.patch, #11108
    Wed 03 Nov 2010 10:43:43 PM UTCsyntronPlanned Release2.3.0=>2.4.0
    Wed 20 Oct 2010 04:34:42 PM UTCsyntronAttached File-=>Added 20101020.1-trunk-generate-a-full-path-from-a-given-relative-path.patch, #10848
      StatusIn Progress=>Ready For Test
    Wed 20 Oct 2010 12:08:14 PM UTCsyntronStatusReady For Test=>In Progress
    Wed 20 Oct 2010 11:13:14 AM UTCsyntronAttached File-=>Added 20101020-trunk-generate-a-full-path-from-a-given-relative-path.patch, #10845
      StatusIn Progress=>Ready For Test
    Tue 19 Oct 2010 09:57:08 PM UTCsyntronAttached File-=>Added 20101019-trunk-generate-a-full-path-from-a-given-relative-path.patch, #10828
    Mon 18 Oct 2010 09:39:51 PM UTCsyntronStatusReady For Test=>In Progress
    Mon 18 Oct 2010 03:08:10 PM UTCsyntronAttached File-=>Added 20101018-trunk-generate-a-full-path-from-a-given-relative-path.patch, #10819
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup