Re: patch: MC for Win32

[Franco Bez <franco bez web de>]

Hi Pavel,

Am Donnerstag, 29. November 2001 03:56 schrieb Pavel Roskin:
> 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. 

I couldn't find the prototypes of "undefined functions" anywhere else - this 
was just a quick shot.

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

Good - this makes the source tree more easy to understand. ( I'm still having 
some problems with this.)

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

Accepted - will try to remember - I promise.

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

We could also use OS2_NT - no problem with me. ( Are there any OS2 porters. 
Is there any OS2 anymore?)

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

I'm not shure if there are really many places for this. The (temporary) 
Commandfiles for instance should be opened in TEXT mode.
There are not many places in MC's source where files are opened.

> > mc/pc/Makefile.MIN
> > - removed ".exe" from mingw-binary names
> >    to make build possible on cross-compilers
>
> Does it work in Windows?

YES - Windows auto-adds the extension, it searches for .bat .com .exe in that 
order.

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

Again - I think OS2 is dead - isn't it. Maybe remove OS2 - like we did with 
GNOME.

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

Backslash in includes should be removed - otherwise no cross-compiler will 
work with the source.

Comments:
removed 
   #define USE_O_TEXT
Was once needed for CR/LF translation in MC's editor, but the strategy was 
changed by someone else (Miguel??) there is an #error DO NOT USE USE_O_TEXT 
somewhere in the code.

pid_t, mode_t and pipe
these #defines are no longer needed / allready defined somewhere else (mingw 
headers if i remember right) 
I got "redefinition of" errors, and decided to take the other definition.

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

This BUG is not strictly Win2k specific, just it apears on Win2k by default:
Description:
On Windows there's a WINDOW size, and a Buffer size for the Console Window.
We used the Buffer size. Until Win2k the Buffer size was equal to the Window 
size by default. (On NT4 you can change it, and provoce the BUG).
But since Win2k, M$ changed  the buffer to 300 lines, so you can now scroll 
back in the commandwindow to read lengthy output.
Now MC tried to create a Console Window with 300 lines, and crashed.

So I changed the code to use the Window size now. IMHO this is also what the 
user expects.

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

OK let's remove it. (What about OS2 ?)

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

I'll try - what's anoying You , what should I avoid?

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

I'll try to remember this. (I like compact code - then I have more oversight 
when the file is open in an editor) But I will try to remember to avoid it 
with MC.

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

I didn't know this.

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

Good. ( Ithink there's still a lot of cleanup to do)

> Please avoid C++ comments in C programs.

OOPS - have I missed one Comment. I usually do C++ only. So it's hard to 
ALLWAYS remember that MC is a C Program.

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

There is no slang.h with WINDOWS. But I found that slang-mc.h provides all 
prototypes needed. So I used slang-mc.h inplace of slang.h with the Windows 
port. For Un*x platforms slang.h is usually available - so it's WINDOWS 
(perhaps OS2???) specific.

> > mc/src/cmd.c
> > - fixed includes
>
> How about global.h?  Doesn't it already include sys/time.h?

True, Maybe need to fix pc/config.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.

I had this feature for quite a while in my mc-4.1.36 based port, so I moved 
it over. 
The problem was that I had  the changes I did with mc-4.1.36 as a basis.
These changes contain both Build-fixes and features. I wanted to move all 
over to the new code base, so I can start working on a single Code base. That 
would make things much easier for me.

>  But
> how about converting the command to lowercase.  Otherwise "Cd" is not
> supported, but "Set" is.

We should look at this in more detail when MC for Win32 is more stable.

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

You're right - we should fix this in global.h / config.h

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

Maybe fix config.h then.
( There is no utime.h just a sys/utime.h - at least with mingw.)

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

Again there's no slang.h with WINDOWS, but slang-mc.h has all we need - Why 
not use it then?

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

If I remeber right - MC crashed there - just a quick fix. (HACK)

> > mc/src/user.h
> > - fixed includes
>
> Again: there is no such thing as gtkedit in the CVS MC.

We'll have to fix this. Somehow edit/edit-widget.h didn't work, but 
gtkedit/edit-widget.h did.

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

Windows has it's own way with the TempDir. It provides API functions for that 
purpose. I'm using them here.

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

Again, no <utime.h> just <sys/utime.h> with (at least) mingw.


Conclusion,

it's still a long way to make MC stable on Win32. 

We'll have to do a lot of cleanup - check config.h globals.h

I'll have to get used to MC's official coding style 
(is there any **short** readme on this topic?)

We'll have to check if the support for OS2 and for the other WIN32 Compilers 
(Borland etc..) is still worth supporting, or if it should be removed.


Ciao,
  Franco