RE: [setup PATCH] SetDlgItemFont
> On Sat, 2003-08-02 at 09:12, Max Bowsher wrote: > > > Now, I don't think moving 2 lines of code and 4 lines of comments into a > > seperate function makes this clearer - rather, it obfuscates what is > > happening here. > > > > In case you are not convinced, here is the alternate patch, to avoid another > > round-trip of emails: > > I'm not particularly fond of the name "SetFontPolicy". Better names > > welcomed. > > call it void PropertyPage::setTitleFont(). > > And I'm not convinced - this is appropriate to be a separate function. > > Approved - as a separate function, called setTitleFont. > > Cheers, > Rob > I have to agree with Rob here. What really should be done down the road[1] is to have another class derived from PropertyPage, say SetupWizardPage, which is then the base class for the various pages. The intended-to-be-rather-generic PropertyPage really has no business setting fonts. Keeping this functionality a separate member will help prevent it from becoming entangled in the core PropertyPage logic, and make it easier to move to a new class in the aforementioned future. "And I even like the color" (Sultan of... Whereever... in "Indiana Jones and the Last Crusade"). "setTitleFont" is about as good a name as I can think of for setting title fonts. (I'll fix the capitalization later ;-)). BTW, if I haven't said so already (memory is the second thing to go) I ***REALLY*** appreciate your work Max in shepherding my work though the often-agonizing approval process. [1] Key words: "down the road", in accordance with the "Bird In The Hand Is Worth Two In The Bush Pattern" from my upcoming book, "Don't Start Nothin', Won't Be Nothin': Getting Software From Here To There In Finite Time". -- Gary R. Van Sickle Brewer. Patriot.
Re: [setup PATCH] SetDlgItemFont
On Sat, 2003-08-02 at 09:12, Max Bowsher wrote: > Now, I don't think moving 2 lines of code and 4 lines of comments into a > seperate function makes this clearer - rather, it obfuscates what is > happening here. > > In case you are not convinced, here is the alternate patch, to avoid another > round-trip of emails: > I'm not particularly fond of the name "SetFontPolicy". Better names > welcomed. call it void PropertyPage::setTitleFont(). And I'm not convinced - this is appropriate to be a separate function. Approved - as a separate function, called setTitleFont. Cheers, Rob -- GPG key available at: http://members.aardvark.net.au/lifeless/keys.txt. --- signature.asc Description: This is a digitally signed message part
Re: [setup PATCH] SetDlgItemFont
Robert Collins wrote:
> On Thu, 2003-07-31 at 22:20, Max Bowsher wrote:
>> ChangeLog says it all really - this is just an incremental tweak to make
it
>> easier to create a "Finished" page, and to have all our font stylings in
one
>> place.
>>
>> Note Gary had the SetDlgItemFont calls moved out in a seperate method.
>> Given that they are almost the entire contents of the "case
WM_INITDIALOG",
>> I don't think this is appropriate.
>>
>> Max.
>
> Well, factoring out code for clarity is a good thing - the separate
> function is appropriate.
>
> Can you resubmit with that..
OK, but I actually believe that a seperate function makes things less clear
in this case, so I'm going to attempt to convince you to go with my original
submission.
Here is the block of code with the changes made:
=
switch (message)
{
case WM_INITDIALOG:
{
OnInit ();
// These font settings will just silently fail when the resource id
// is not present on a page.
// Set header title font of each internal page
SetDlgItemFont(IDC_STATIC_HEADER_TITLE, "MS Shell Dlg", 8, FW_BOLD);
// Set the font for the IDC_STATIC_WELCOME_TITLE
SetDlgItemFont(IDC_STATIC_WELCOME_TITLE, "Arial", 12, FW_BOLD);
// TRUE = Set focus to default control (in wParam).
return TRUE;
break;
}
case WM_NOTIFY:
switch (((NMHDR FAR *) lParam)->code)
{
=
Now, I don't think moving 2 lines of code and 4 lines of comments into a
seperate function makes this clearer - rather, it obfuscates what is
happening here.
In case you are not convinced, here is the alternate patch, to avoid another
round-trip of emails:
I'm not particularly fond of the name "SetFontPolicy". Better names
welcomed.
+2003-08-02 Gary R. Van Sickle <[EMAIL PROTECTED]>
+
+ Changes modified by Max Bowsher <[EMAIL PROTECTED]>
+ * splash.cc (Copyright): Update copyright dates.
+ (SplashPage::OnInit): Remove call to SetDlgItemFont(). Now handled in
+ base class.
+ * proppage.h (PropertyPage::SetFontPolicy): Declare.
+ * proppage.cc (Copyright): Update copyright dates.
+ (PropertyPage::DialogProc WM_INITDIALOG): Move all font setting code,
+ including that from splash.cc into new function...
+ (PropertyPage::SetFontPolicy): Create, using moved code. Change font
+ "MS Sans Serif" to "MS Shell Dlg" in line with recent res.rc change.
+ Set font for IDC_STATIC_WELCOME_TITLE here, to allow easy re-use of
+ style for future "Finished" page.
Index: proppage.cc
===
RCS file: /home/max/cvsmirror/cygwin-apps-cvs/setup/proppage.cc,v
retrieving revision 2.8
diff -u -p -r2.8 proppage.cc
--- proppage.cc 1 Aug 2003 22:20:13 - 2.8
+++ proppage.cc 1 Aug 2003 23:03:36 -
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2001, Gary R. Van Sickle.
+ * Copyright (c) 2001, 2002, 2003 Gary R. Van Sickle.
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@@ -116,9 +116,7 @@ PropertyPage::DialogProc (UINT message,
{
OnInit ();
- // Set header title font of each internal page to MS Sans Serif, Bold, 8
Pt.
- // This will just silently fail on the first and last pages.
- SetDlgItemFont(IDC_STATIC_HEADER_TITLE, "MS Sans Serif", 8, FW_BOLD);
+ SetFontPolicy ();
// TRUE = Set focus to default control (in wParam).
return TRUE;
@@ -269,4 +267,15 @@ PropertyPage::DialogProc (UINT message,
// Wasn't handled
return FALSE;
+}
+
+void
+PropertyPage::SetFontPolicy ()
+{
+ // These font settings will just silently fail when the resource id
+ // is not present on a page.
+ // Set header title font of each internal page
+ SetDlgItemFont(IDC_STATIC_HEADER_TITLE, "MS Shell Dlg", 8, FW_BOLD);
+ // Set the font for the IDC_STATIC_WELCOME_TITLE
+ SetDlgItemFont(IDC_STATIC_WELCOME_TITLE, "Arial", 12, FW_BOLD);
}
Index: proppage.h
===
RCS file: /home/max/cvsmirror/cygwin-apps-cvs/setup/proppage.h,v
retrieving revision 2.7
diff -u -p -r2.7 proppage.h
--- proppage.h 1 Aug 2003 22:17:04 - 2.7
+++ proppage.h 1 Aug 2003 23:05:10 -
@@ -45,6 +45,7 @@ class PropertyPage:public Window
LPARAM lParam);
static BOOL CALLBACK DialogProcReflector (HWND hwnd, UINT message,
WPARAM wParam, LPARAM lParam);
+ void SetFontPolicy ();
protected:
virtual BOOL CALLBACK DialogProc (UINT message, WPARAM wParam,
Index: splash.cc
===
RCS file: /home/max/cvsmirror/cygwin-apps-cvs/setup/splash.cc,v
retrieving revision 2.11
diff -u -p -r2.11 splash.cc
--- splash.cc 29 Jul 2003 14:14:06 - 2.11
+++ splash.cc 31 Jul 2003 11:49:30 -
@@ -1,5 +1,5 @@
/*
- * Cop
Re: [setup PATCH] SetDlgItemFont
On Thu, 2003-07-31 at 22:20, Max Bowsher wrote: > ChangeLog says it all really - this is just an incremental tweak to make it > easier to create a "Finished" page, and to have all our font stylings in one > place. > > Note Gary had the SetDlgItemFont calls moved out in a seperate method. > Given that they are almost the entire contents of the "case WM_INITDIALOG", > I don't think this is appropriate. > > Max. Well, factoring out code for clarity is a good thing - the separate function is appropriate. Can you resubmit with that.. Cheers, Rob -- GPG key available at: http://members.aardvark.net.au/lifeless/keys.txt. --- signature.asc Description: This is a digitally signed message part
Re: [setup PATCH] SetDlgItemFont
Andrew M. Inggs wrote: > Max Bowsher wrote: >> Andrew M. Inggs wrote: >> >>> Could you include the ChangeLog on its own rather than using a >>> diff-generated version? >> >> Why? The issue below? That's independent. > It would make it easier to read, in my opinion. I do strip off the diff surroundings, leaving only the + prefixed on each line. > It's also pretty standard policy to make it easier for the person > applying the patch. I guess it's not relevant here since you apply your own. Right. And if I were making patches for someone else to apply, I would include the ChangeLog as a zero-context unidiff (-U0), which would add to the top of ChangeLog regardless of it's contents. Max.
Re: [setup PATCH] SetDlgItemFont
Max Bowsher wrote: Andrew M. Inggs wrote: Could you include the ChangeLog on its own rather than using a diff-generated version? Why? The issue below? That's independent. It would make it easier to read, in my opinion. It's also pretty standard policy to make it easier for the person applying the patch. I guess it's not relevant here since you apply your own. I'll shut up now. -- Andrew
RE: [setup PATCH] SetDlgItemFont
[snip] > > + // Set header title font of each internal page > > + SetDlgItemFont(IDC_STATIC_HEADER_TITLE, "MS Shell Dlg", 8, FW_BOLD); > > + // Set the font for the IDC_STATIC_WELCOME_TITLE > > + SetDlgItemFont(IDC_STATIC_WELCOME_TITLE, "Ariel", 12, FW_BOLD); > > > > Shouldn't that be Arial rather than Ariel? > No. Microsoft's official GUI guidelines specify that The Littlest Mermaid be used for the "Welcome" and "Finished" text. ;-) -- Gary R. Van Sickle Brewer. Patriot.
Re: [setup PATCH] SetDlgItemFont
On Thu, 31 Jul 2003, Max Bowsher wrote: > Andrew M. Inggs wrote: > > Max Bowsher wrote: > > >> - // This will just silently fail on the first and last pages. > >> - SetDlgItemFont(IDC_STATIC_HEADER_TITLE, "MS Sans Serif", 8, FW_BOLD); > >> + // These font settings will just silently fail when the resource id > >> + // is not present on a page. > >> + // Set header title font of each internal page > >> + SetDlgItemFont(IDC_STATIC_HEADER_TITLE, "MS Shell Dlg", 8, FW_BOLD); > >> + // Set the font for the IDC_STATIC_WELCOME_TITLE > >> + SetDlgItemFont(IDC_STATIC_WELCOME_TITLE, "Ariel", 12, FW_BOLD); > > > > Shouldn't that be Arial rather than Ariel? > > Maybe. Curiously, it doesn't seem to make a difference! > > Max. If it doesn't recognize the font name (or the font is not installed), Windows reverts to the default font. Apparently, Arial is the default font for dialog titles... You can check that by modifying the Appearance tab of the Display properties and seeing if it makes a difference then. Igor -- http://cs.nyu.edu/~pechtcha/ |\ _,,,---,,_[EMAIL PROTECTED] ZZZzz /,`.-'`'-. ;-;;,_[EMAIL PROTECTED] |,4- ) )-,_. ,\ ( `'-' Igor Pechtchanski, Ph.D. '---''(_/--' `-'\_) fL a.k.a JaguaR-R-R-r-r-r-.-.-. Meow! "I have since come to realize that being between your mentor and his route to the bathroom is a major career booster." -- Patrick Naughton
Re: [setup PATCH] SetDlgItemFont
Andrew M. Inggs wrote: > Max Bowsher wrote: > >> >> +2003-07-31 Gary R. Van Sickle >> + >> + Changes modified by Max Bowsher >> + * splash.cc (Copyright): Update copyright dates. >> + (SplashPage::OnInit): Remove call to SetDlgItemFont(). Now handled in >> + base class. > > Could you include the ChangeLog on its own rather than using a > diff-generated version? Why? The issue below? That's independent. > Is it just the way my email client is displaying tabs, or are tabs being > converted into single spaces somewhere along the way? Yes, Outlook Express eats tabs. Suggestions (off list) for a better, GUI, non-ugly MUA welcomed. >> Index: proppage.cc > ... >> - // Set header title font of each internal page to MS Sans Serif, Bold, 8 >> Pt. >> - // This will just silently fail on the first and last pages. >> - SetDlgItemFont(IDC_STATIC_HEADER_TITLE, "MS Sans Serif", 8, FW_BOLD); >> + // These font settings will just silently fail when the resource id >> + // is not present on a page. >> + // Set header title font of each internal page >> + SetDlgItemFont(IDC_STATIC_HEADER_TITLE, "MS Shell Dlg", 8, FW_BOLD); >> + // Set the font for the IDC_STATIC_WELCOME_TITLE >> + SetDlgItemFont(IDC_STATIC_WELCOME_TITLE, "Ariel", 12, FW_BOLD); > > Shouldn't that be Arial rather than Ariel? Maybe. Curiously, it doesn't seem to make a difference! Max.
Re: [setup PATCH] SetDlgItemFont
Max Bowsher wrote: +2003-07-31 Gary R. Van Sickle + + Changes modified by Max Bowsher + * splash.cc (Copyright): Update copyright dates. + (SplashPage::OnInit): Remove call to SetDlgItemFont(). Now handled in + base class. Could you include the ChangeLog on its own rather than using a diff-generated version? Is it just the way my email client is displaying tabs, or are tabs being converted into single spaces somewhere along the way? Index: proppage.cc ... - // Set header title font of each internal page to MS Sans Serif, Bold, 8 Pt. - // This will just silently fail on the first and last pages. - SetDlgItemFont(IDC_STATIC_HEADER_TITLE, "MS Sans Serif", 8, FW_BOLD); + // These font settings will just silently fail when the resource id + // is not present on a page. + // Set header title font of each internal page + SetDlgItemFont(IDC_STATIC_HEADER_TITLE, "MS Shell Dlg", 8, FW_BOLD); + // Set the font for the IDC_STATIC_WELCOME_TITLE + SetDlgItemFont(IDC_STATIC_WELCOME_TITLE, "Ariel", 12, FW_BOLD); Shouldn't that be Arial rather than Ariel? -- Andrew
