Re: patch: MC for Win32

[Pavel Roskin <proski gnu org>]

Hi, Franco!

I'm trying to apply your patch (and applying it partially) and here are
some notes.

> - I took a CVS checkout from 27.Okt as a basis

Strange that you are referencing gtkedit in user.h.  Well, it's my guilt.  
Miguel asked me not to remove the gnome directories from the main branch.  
I should have argued that if there are any CVS problems they should be
fixed in CVS, not worked around.  Besides, it there are such problems,
it's better if they are easy to notice (the whole directory disappears)  
rather than when they are subtle (one file is missing somewhere).

I established an module "mc-text" on CVS but you were obviously using the 
complete checkout.  Sorry for inconvenience :-(

I'm fixing this error.  The unused files have been removed from the head 
branch.  Now it's safe again to use "cvs update -d" without risking to get 
GNOME-related files.

> You can build mc.exe by changing to the pc subdir and run
> $> make -fMakefile.MIN  

Please leave a space in all documentation (FAQ etc) as it's safer and less 
confusing.

> I added the two files
> 
> mc/pc/regex.c
> mc/pc/regex.h

I've committed them.  We'll remove them later and a few lines in ChangeLog
won't increase the size of the distribution too much :-)

> mc/edit/edit.c
> - Open file in binary mode on Win32 platforms.

Please don't break things too much.  It should apply to OS/2 as well.  
Then why _OS_NT and not OS2_NT?  If your idea is wrong, then OS/2 porters
will fix it for you, but let's not require them to do it everywhere.

Besides, if you expect this fix to be used in many places, it's better to
define something as "b" and use it instead of ifdefs:

#ifdef OS2_NT
#define FOPEN_BINARY "b"
#else
#define FOPEN_BINARY
#endif

fopen(file, "r" FOPEN_BINARY)

> mc/pc/Makefile.MIN
> - removed ".exe" from mingw-binary names 
>    to make build possible on cross-compilers

Does it work in Windows?

> - fixed compiler and linker flags for use of glib and gettext
> - fixed order of flags for linking mc.exe

Fine, but please avoid double spaces - nobody needs them.

> mc/pc/Makefile.PC
> - added/removed files to/from build

You don't update files for OS/2.  It's easier that you keep things in 
sync, or somebody will think whether there is a reason for that 
difference.

> mc/pc/chmod.c
> - fixed includes for cross-compiler
> - added gettext N_() and _() makro calls

Fine.  Committed.

> mc/pc/config.h
> - a few fixes

I don't understand some of them.  Perhaps backslash in includes should be 
elininated globally.  Everything else requires explanation.

> mc/pc/cons_nt.c
> - changes regarding the calculation of the size of the console window on Win32
>   fixes a bug with Win2k

I have Win2000 now.  I'll apply it when I check it.

> mc/pc/dirent.h
> - added comment that file seems to be no longer needed

It's not just a comment.  Be honest.  I read your patch.  If the file is 
not needed - remove it.

> mc/pc/drive.c
> - added gettext N_() and _() makro calls
> - added workaround for a crash that happens 
>    when Changing drive while panel not in DirListing mode

The gettext part is fine.  Please use better coding style for the new 
code.

> mc/pc/key_nt.c
> - fixed a problem with AltGr Key
> - added/changed dummy functions (mouse dummys)

The same comment applies.  Please never put "return" to the same line as
"if" since it's hard to debug.  Not to mention whole functions.

> mc/pc/slint_pc.c
> - added comment -> DO NOT USE GETTEXT HERE

Not very useful comment.  There are documented means for that 
(POTFILES.ignore if I remember correctly).

> mc/pc/util_nt.c
> - fixed includes
> - added gettext N_() and _() makro calls
> - fixed problem with the display of free disc space
> - added dummy functions

Cannot you just use global.h - it's supposed to take care of includes?
Please avoid C++ comments in C programs.

> mc/slang/slerr.c
> - fixed includes

Why is this fix needed?  Why is it Win32-specific?  I would think that you 
should rather fix something else.  Same applies to all slang files.

> mc/src/cmd.c
> - fixed includes

How about global.h?  Doesn't it already include sys/time.h?
It it posible to add the top-level directory to the includes so that 
"vfs/vfs.h" works?

> mc/src/command.c
> - added handling of internal SET command for Win32

I don't like that you are mixing new features and pure build fixes.  But 
how about converting the command to lowercase.  Otherwise "Cd" is not 
supported, but "Set" is.

> mc/src/ext.c
> - no shellbang in temporary command file on windows

Will apply.

> mc/src/file.c
> - fixed includes
> - commented out the detection of cyclic symlinks on Win32
>   this is only a Quick Fix

Will apply the second part.  I don't like your approach to fixing the 
includes.

> mc/src/global.h
> - fixed includes

What's wrong with utime.h?  Why don't you want to include it even if 
HAVE_UTIME_H is defined?

> mc/src/main.c
> - fixed includes
> - director_history_add is broken on Win32 - mc crashes -
>    made it a dummy function on Win32
>   THIS definately has to be fixed
> - made init_xterm_support a dummy on Win32 - there is no xterm there
> - made mc ignore Ctrl-C breaks on Win32
> - defined the gettext LOCALEDIR for Win32
> - added comment that gfree(home_dir); might no longer be needed on Win32

Please don't include local files (in quotes) before system includes.

> mc/src/myslang.h
> - fixed includes

Again, you must be doing something wrong.

> mc/src/treestore.c
> - commented out 
>    process_special_dirs (&special_dirs, CONFDIR "mc.global");
>    for Win32

Why?

> mc/src/user.h
> - fixed includes

Again: there is no such thing as gtkedit in the CVS MC.

> mc/src/util.c
> - fixed includes
> - fixed parameters for bizp2 might be also needed for other platforms

Will apply after testing.

> - fixed TEMPDIR for Win32

What was wrong with it?

> mc/vfs/vfs.h
> - fixed includes

Again, what's wrong with utime.h?  If there are any problems with it it's 
better to use it in global.h only and move all tricks there.

-- 
Regards,
Pavel Roskin