re: programs/winefontcfg: Add winefontcfg

[Dan Kegel]

I think those static function prototypes in winefontcfg.h should not be there.
.h files are for things shared between files;
your prototypes were probably put there to
avoid compiler errors.  Better to put them in the .c file
if they're  not used by multiple files.

programs/winefontcfg: Add winefontcfg

[Dmitry Timoshkov]

"Dan Kegel"  wrote:
> I think those static function prototypes in winefontcfg.h should not be there.
> .h files are for things shared between files;
> your prototypes were probably put there to
> avoid compiler errors.  Better to put them in the .c file
> if they're  not used by multiple files.

Or even better completely avoid forward declarations by properly
arranging internal functions. winefontcfg.h is not needed at all.

Some other comments: INT_PTR is not appropriate return type for window
procs, global variables should be made static.

programs/winefontcfg: Add winefontcfg

[Nigel Liang (梁乃強)]

On 7/11/07, Dmitry Timoshkov  wrote:
> "Dan Kegel"  wrote:
> > I think those static function prototypes in winefontcfg.h should not be there.
> > .h files are for things shared between files;
> > your prototypes were probably put there to
> > avoid compiler errors.  Better to put them in the .c file
> > if they're  not used by multiple files.
>
> Or even better completely avoid forward declarations by properly
> arranging internal functions. winefontcfg.h is not needed at all.
>
> Some other comments: INT_PTR is not appropriate return type for window
> procs, global variables should be made static.
>
> --
> Dmitry.

Hi,

Sent in try 2: http://www.winehq.org/pipermail/wine-patches/2007-July/041356.html

Thanks for the comments!
-Nigel

programs/winefontcfg: Add winefontcfg

[Dan Kegel]

On 7/11/07, Nigel Liang (梁乃強)  wrote:
> Sent in try 2: http://www.winehq.org/pipermail/wine-patches/2007-July/041356.html

Not all the locals in  FontLinkListBoxWndProc really need to be static.
(e.g. 'dst')
Those that really need to be static should probably be initialized to
zero, and cleared to zero after they're consumed.
(I'm thinking of 'src' in particular; it's just scary to
have a value hanging around in there after you do the
move; what happens if you get a stray WM_BUTTONUP message
later?)

The code indentation style is nonstandard; I think wine
usually uses 4 space, not 2 space.
(OK, OK, I probably submitted lots of code that wasn't 4 space
indented, but I'm sorry :-)

You still have nonportable initializations like
+  const WCHAR fontlinktermstr[] = {0,0};
Wine uses ANSI C.  You cannot initialize nonstatic arrays
at compile time.

You have some globals with names that aren't obviously global
or easy to search for:
+static PROCESS_INFORMATION pi;
+static HWND hChild, hParent;
This probably isn't too bad, but pi really is too
short for a global variable name.

- Dan

programs/winefontcfg: Add winefontcfg

[Vijay Kiran Kamuju]

Hi,

Is it possible to have a separate tab for fonts in winecfg, rather
than a separate program.
Its my opinion of winefontcfg, rather than having a collection of
configuration tools. Better have one.

Thanks,
VJ

On 7/12/07, Dan Kegel  wrote:
> On 7/11/07, Nigel Liang (梁乃強)  wrote:
> > Sent in try 2: http://www.winehq.org/pipermail/wine-patches/2007-July/041356.html
>
> Not all the locals in  FontLinkListBoxWndProc really need to be static.
> (e.g. 'dst')
> Those that really need to be static should probably be initialized to
> zero, and cleared to zero after they're consumed.
> (I'm thinking of 'src' in particular; it's just scary to
> have a value hanging around in there after you do the
> move; what happens if you get a stray WM_BUTTONUP message
> later?)
>
> The code indentation style is nonstandard; I think wine
> usually uses 4 space, not 2 space.
> (OK, OK, I probably submitted lots of code that wasn't 4 space
> indented, but I'm sorry :-)
>
> You still have nonportable initializations like
> - const WCHAR fontlinktermstr[] = {0,0};
>
> Wine uses ANSI C.  You cannot initialize nonstatic arrays
> at compile time.
>
> You have some globals with names that aren't obviously global
> or easy to search for:
> +static PROCESS_INFORMATION pi;
> +static HWND hChild, hParent;
> This probably isn't too bad, but pi really is too
> short for a global variable name.
> - Dan
>
> --
> Wine for Windows ISVs: http://kegel.com/wine/isv

programs/winefontcfg: Add winefontcfg

[Nigel Liang]

On 7/12/07, Vijay Kiran Kamuju  wrote:
> Hi,
>
> Is it possible to have a separate tab for fonts in winecfg, rather
> than a separate program.
> Its my opinion of winefontcfg, rather than having a collection of
> configuration tools. Better have one.
>
> Thanks,
> VJ

The main reason I wrote a separate program instead of having it as a
tab in winecfg is because the preview area reflects the changes to the
fontlink registry in real-time. To do this, I had to update the
registry within the main program and have the preview area as a
separate process which is killed and restarted to re-read the registry
entry. Instead of having a seperate binary for the preview area,
winefontcfg calls itself with parameters to specify that it should act
as a child window. It will be hard to do this with winecfg.....
> On 7/12/07, Dan Kegel  wrote:
> > On 7/11/07, Nigel Liang (梁乃強)  wrote:
> > > Sent in try 2: http://www.winehq.org/pipermail/wine-patches/2007-July/041356.html
> >
> > Not all the locals in  FontLinkListBoxWndProc really need to be static.
> > (e.g. 'dst')
> > Those that really need to be static should probably be initialized to
> > zero, and cleared to zero after they're consumed.
> > (I'm thinking of 'src' in particular; it's just scary to
> > have a value hanging around in there after you do the
> > move; what happens if you get a stray WM_BUTTONUP message
> > later?)
> >
> > The code indentation style is nonstandard; I think wine
> > usually uses 4 space, not 2 space.
> > (OK, OK, I probably submitted lots of code that wasn't 4 space
> > indented, but I'm sorry :-)
> >
> > You still have nonportable initializations like
> > - const WCHAR fontlinktermstr[] = {0,0};
> >
> > Wine uses ANSI C.  You cannot initialize nonstatic arrays
> > at compile time.
> >
> > You have some globals with names that aren't obviously global
> > or easy to search for:
> > +static PROCESS_INFORMATION pi;
> > +static HWND hChild, hParent;
> > This probably isn't too bad, but pi really is too
> > short for a global variable name.
> > - Dan

I'll make these changes and submit another try.

Thanks,
-Nigel

programs/winefontcfg: Add winefontcfg

[Dan Kegel]

VJ wrote:
> Is it possible to have a separate tab for fonts in winecfg, rather
> than a separate program.
> Its my opinion of winefontcfg, rather than having a collection of
> configuration tools. Better have one.

That's the wrong direction, I think, and here's why:
Native uses separate tools for *everything*, tied together
in a Control Panel.  I'd like to see us go that route instead.
It would make it easier to support third-party control
panel applets.

Now, Wine does support control panel applets, I think, but
it's a pain in the ass to run them because we don't provide a
menu entry called Control Panel that shows the applets
graphically.   (Presumably this behavior belongs in explorer,
since it's done on native with shell folders.)  How hard would
it be to throw this together?  We could make winecfg show up
as a control panel applet for now, and then slowly start breaking
its tabs out into applets as the whim hits us.
- Dan

programs/winefontcfg: Add winefontcfg

[Stefan Dösinger]

Am Donnerstag, 12. Juli 2007 16:54 schrieb Dan Kegel:
> VJ wrote:
> > Is it possible to have a separate tab for fonts in winecfg, rather
> > than a separate program.
> > Its my opinion of winefontcfg, rather than having a collection of
> > configuration tools. Better have one.
>
> That's the wrong direction, I think, and here's why:
> Native uses separate tools for *everything*, tied together
> in a Control Panel.  I'd like to see us go that route instead.
> It would make it easier to support third-party control
> panel applets.
>
> Now, Wine does support control panel applets, I think, but
> it's a pain in the ass to run them because we don't provide a
> menu entry called Control Panel that shows the applets
> graphically.   (Presumably this behavior belongs in explorer,
> since it's done on native with shell folders.)  How hard would
> it be to throw this together?  We could make winecfg show up
> as a control panel applet for now, and then slowly start breaking
> its tabs out into applets as the whim hits us.

I am not sure if it is a good idea to wrap the winecfg functionality into 
something complex like a control panel applet(which is tied to shell folders 
in some way). A user can easilly break core parts of wine with winecfg. The 
more complex the requirements for winecfg are the more likely it is that a 
user can't get back into winecfg to undo a bad option.

On the other hand Dan makes a valid point here. This is just how windows 
works. And it is already possible to break a wineprefix in a way that is not 
reversible with winecfg(e.g. set ole32 = "native").

The third thing is that in the long run winecfg should vanish. Wine shouldn't 
need any configuration on its own, except maybe drive bindings and windows 
version.

Make winecfg a control panel applet? [was: programs/winefontcfg: Add winefontcfg]

[Frank Richter]

On 12.07.2007 18:29, Stefan Dösinger wrote:
> I am not sure if it is a good idea to wrap the winecfg functionality into 
> something complex like a control panel applet(which is tied to shell folders 
> in some way). A user can easilly break core parts of wine with winecfg. The 
> more complex the requirements for winecfg are the more likely it is that a 
> user can't get back into winecfg to undo a bad option.

Control Panel applets are just DLLs... You could provide an alternative
way to launch the wine configuration in case something is too borked,
like an entry point for RunDll32, or turn winecfg into a simple launcher
for the configuration applet.

-f.r.