re: programs/winefontcfg: Add winefontcfg
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.
"Dan Kegel" <dank(a)kegel.com> 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.
On 7/11/07, Dmitry Timoshkov <dmitry(a)codeweavers.com> wrote:
"Dan Kegel" <dank(a)kegel.com> 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
On 7/11/07, Nigel Liang (梁乃強) <ncliang(a)google.com> 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
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 <dank(a)kegel.com> wrote:
On 7/11/07, Nigel Liang (梁乃強) <ncliang(a)google.com> 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
On 7/12/07, Vijay Kiran Kamuju <infyquest(a)gmail.com> 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 <dank(a)kegel.com> wrote:
On 7/11/07, Nigel Liang (梁乃強) <ncliang(a)google.com> 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
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
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.
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.
participants (7)
-
Dan Kegel -
Dmitry Timoshkov -
Frank Richter -
Nigel Liang -
Nigel Liang (梁乃強) -
Stefan Dösinger -
Vijay Kiran Kamuju