[PATCH] setup: Always draw chooser icons using system colors

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] setup: Always draw chooser icons using system colors

Igor Peshansky
Hi,

As promised in <http://cygwin.com/ml/cygwin/2006-05/msg00612.html>, here's
a patch that makes setup always draw the chooser icons using the system
foreground and background colors (just like it does for text).  I've also
taken the opportunity to do a slight bit of code clean-up.  There's still
a little glitch left -- when doing a lot of fast scrolling back and forth,
the icons sometimes switch from the system foreground color to the
inverted system background color.  I think I know why this is happening
(i.e., the FillRgn() gets ignored for some reason), but this needs further
debugging.  As this is a corner case, I wouldn't worry too much about it.
As always, the ChangeLog is below -- opinions welcome.
        Igor

ChangeLog:
==============================================================================
2006-05-23  Igor Peshansky  <[hidden email]>

        * PickPackageLine.h (PickPackageLine::DrawIcon): Move to PickView.
        * PickView.h (PickView::DrawIcon): Move from PickPackageLine.
        * PickCategoryLine.cc (PickCategoryLine::paint): Use
        PickView::DrawIcon() instead of BitBlt().
        * PickPackageLine.cc (PickPackageLine::DrawIcon): Move to PickView.
        (PickPackageLine::paint): Use PickView::DrawIcon().
        * PickView.cc (PickView::~PickView): Delete GDI objects.
        (PickView::DrawIcon): New function.  Use system default colors to
        draw bitmaps.
        (PickView::paint): Set background color instead of using transparent
        mode.

--
                                http://cs.nyu.edu/~pechtcha/
      |\      _,,,---,,_    [hidden email] | [hidden email]
ZZZzz /,`.-'`'    -.  ;-;;,_ Igor Peshansky, Ph.D. (name changed!)
     |,4-  ) )-,_. ,\ (  `'-' old name: Igor Pechtchanski
    '---''(_/--'  `-'\_) fL a.k.a JaguaR-R-R-r-r-r-.-.-.  Meow!

"Las! je suis sot... -Mais non, tu ne l'es pas, puisque tu t'en rends compte."
"But no -- you are no fool; you call yourself a fool, there's proof enough in
that!" -- Rostand, "Cyrano de Bergerac"

setup-bitmap-colors.patch (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] setup: Always draw chooser icons using system colors

Brian Dessent
Igor Peshansky wrote:

> As promised in <http://cygwin.com/ml/cygwin/2006-05/msg00612.html>, here's
> a patch that makes setup always draw the chooser icons using the system
> foreground and background colors (just like it does for text).  I've also
> taken the opportunity to do a slight bit of code clean-up.  There's still
> a little glitch left -- when doing a lot of fast scrolling back and forth,
> the icons sometimes switch from the system foreground color to the
> inverted system background color.  I think I know why this is happening
> (i.e., the FillRgn() gets ignored for some reason), but this needs further
> debugging.  As this is a corner case, I wouldn't worry too much about it.
> As always, the ChangeLog is below -- opinions welcome.

Thanks for taking a look at this.  I like that it abstracts all the
blitting to one place, although it seems wasteful to me to have to
create and destroy dc_tmp like that every time.  However, whatever
performance hit this creates is probably better than the blatant
incorrectness that is there now, so I guess this is progress.  I'm also
a little hesitant about this glitch you mention, but it sounds like
you're on top of that too so I say go ahead and check this in and we can
smooth out the rough edges later.

Brian
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] setup: Always draw chooser icons using system colors

Igor Peshansky
On Tue, 23 May 2006, Brian Dessent wrote:

> Igor Peshansky wrote:
>
> > As promised in <http://cygwin.com/ml/cygwin/2006-05/msg00612.html>, here's
> > a patch that makes setup always draw the chooser icons using the system
> > foreground and background colors (just like it does for text).  I've also
> > taken the opportunity to do a slight bit of code clean-up.  There's still
> > a little glitch left -- when doing a lot of fast scrolling back and forth,
> > the icons sometimes switch from the system foreground color to the
> > inverted system background color.  I think I know why this is happening
> > (i.e., the FillRgn() gets ignored for some reason), but this needs further
> > debugging.  As this is a corner case, I wouldn't worry too much about it.
> > As always, the ChangeLog is below -- opinions welcome.
>
> Thanks for taking a look at this.  I like that it abstracts all the
> blitting to one place, although it seems wasteful to me to have to
> create and destroy dc_tmp like that every time.  However, whatever
> performance hit this creates is probably better than the blatant
> incorrectness that is there now, so I guess this is progress.  I'm also
> a little hesitant about this glitch you mention, but it sounds like
> you're on top of that too so I say go ahead and check this in and we can
> smooth out the rough edges later.

Right.  I was a bit concerned about the performance hit, but I haven't
noticed any slowdown in drawing the chooser.  Also, I couldn't factor out
(and cache) the creation of those resources, and figured a timely patch
was better than wasting time exploring it.  We can always do this in a
later refactoring.

As for the glitch, I've only noticed this after very intensive fast
scrolling back-and-forth, using the scroll buttons instead of the scroll
bars...  It's annoying, but I wouldn't worry about it.  What I do worry
about is that my patch doesn't check the return values of any functions.
I need to fix that before I check it in.  Maybe tonight.
        Igor
--
                                http://cs.nyu.edu/~pechtcha/
      |\      _,,,---,,_    [hidden email] | [hidden email]
ZZZzz /,`.-'`'    -.  ;-;;,_ Igor Peshansky, Ph.D. (name changed!)
     |,4-  ) )-,_. ,\ (  `'-' old name: Igor Pechtchanski
    '---''(_/--'  `-'\_) fL a.k.a JaguaR-R-R-r-r-r-.-.-.  Meow!

"Las! je suis sot... -Mais non, tu ne l'es pas, puisque tu t'en rends compte."
"But no -- you are no fool; you call yourself a fool, there's proof enough in
that!" -- Rostand, "Cyrano de Bergerac"
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] setup: Always draw chooser icons using system colors

Igor Peshansky
On Tue, 23 May 2006, Igor Peshansky wrote:

> On Tue, 23 May 2006, Brian Dessent wrote:
>
> > Igor Peshansky wrote:
> >
> > > As promised in <http://cygwin.com/ml/cygwin/2006-05/msg00612.html>,
> > > here's a patch that makes setup always draw the chooser icons using
> > > the system foreground and background colors (just like it does for
> > > text).  I've also taken the opportunity to do a slight bit of code
> > > clean-up.  There's still a little glitch left -- when doing a lot of
> > > fast scrolling back and forth, the icons sometimes switch from the
> > > system foreground color to the inverted system background color.  I
> > > think I know why this is happening (i.e., the FillRgn() gets ignored
> > > for some reason), but this needs further debugging.  As this is a
> > > corner case, I wouldn't worry too much about it. As always, the
> > > ChangeLog is below -- opinions welcome.
> >
> > Thanks for taking a look at this.  I like that it abstracts all the
> > blitting to one place, although it seems wasteful to me to have to
> > create and destroy dc_tmp like that every time.  However, whatever
> > performance hit this creates is probably better than the blatant
> > incorrectness that is there now, so I guess this is progress.  I'm
> > also a little hesitant about this glitch you mention, but it sounds
> > like you're on top of that too so I say go ahead and check this in and
> > we can smooth out the rough edges later.
>
> Right.  I was a bit concerned about the performance hit, but I haven't
> noticed any slowdown in drawing the chooser.  Also, I couldn't factor out
> (and cache) the creation of those resources, and figured a timely patch
> was better than wasting time exploring it.  We can always do this in a
> later refactoring.
I was wrong.  I could (and did) factor out the creation of the resources.
My previous approach was trying to create the brush upon initialization,
instead of in the paint() function.  There is still no perceptible
performance difference, but there *is* a warm fuzzy feeling.

> As for the glitch, I've only noticed this after very intensive fast
> scrolling back-and-forth, using the scroll buttons instead of the scroll
> bars...  It's annoying, but I wouldn't worry about it.  What I do worry
> about is that my patch doesn't check the return values of any functions.
> I need to fix that before I check it in.  Maybe tonight.

As a bonus, I've also tracked down the glitch, which turned out to be
related.  Contrary to the MSDN example code, you *are* supposed to delete
any region you allocate using CreateRectRgn() -- otherwise the system runs
out of space in the region handle table, and refuses to create any more of
them.  As I wasn't deleting the regions, eventually new regions didn't get
created, and thus didn't get filled, and thus I lost the neat color combo
that effectively turned the icon into a mask.  Adding the DeleteObject()
call fixed my original patch, but now I've factored this out anyway (with
the appropriate deletions for all objects, I think).  FWIW, the MSDN entry
on CreateRectRegion is also silent on whether they should be deleted.  Oh,
well...

New iteration of the patch attached.  The ChangeLog is slightly different,
and is included below.  Again, comments welcome, but IMO this is ready to
be checked in (as soon as Brian looks it over and gives the go-ahead).  I
would also guess this is a good excuse for generating a new setup
snapshot.
        Igor
==============================================================================
ChangeLog:
2006-05-23  Igor Peshansky  <[hidden email]>

        * PickPackageLine.h (PickPackageLine::DrawIcon): Move to PickView.
        * PickView.h (PickView::DrawIcon): Move from PickPackageLine.
        (PickView::icon_dc,PickView::bm_icon): New instance field.
        (PickView::rect_icon,PickView::bg_fg_brush): Ditto.
        * PickCategoryLine.cc (PickCategoryLine::paint): Use
        PickView::DrawIcon() instead of BitBlt().
        * PickPackageLine.cc (PickPackageLine::DrawIcon): Move to PickView.
        (PickPackageLine::paint): Use PickView::DrawIcon().
        * PickView.cc (PickView::~PickView): Delete GDI objects.
        (PickView::init): Create icon drawing context.
        (PickView::DrawIcon): New function.  Use system default colors to
        draw bitmaps.
        (PickView::paint): Set background color instead of using transparent
        mode.  Create system-colored brush.

--
                                http://cs.nyu.edu/~pechtcha/
      |\      _,,,---,,_    [hidden email] | [hidden email]
ZZZzz /,`.-'`'    -.  ;-;;,_ Igor Peshansky, Ph.D. (name changed!)
     |,4-  ) )-,_. ,\ (  `'-' old name: Igor Pechtchanski
    '---''(_/--'  `-'\_) fL a.k.a JaguaR-R-R-r-r-r-.-.-.  Meow!

"Las! je suis sot... -Mais non, tu ne l'es pas, puisque tu t'en rends compte."
"But no -- you are no fool; you call yourself a fool, there's proof enough in
that!" -- Rostand, "Cyrano de Bergerac"

setup-bitmap-colors.patch (12K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] setup: Always draw chooser icons using system colors

Brian Dessent
Igor Peshansky wrote:

> New iteration of the patch attached.  The ChangeLog is slightly different,
> and is included below.  Again, comments welcome, but IMO this is ready to
> be checked in (as soon as Brian looks it over and gives the go-ahead).  I
> would also guess this is a good excuse for generating a new setup
> snapshot.

Warm fuzzies indeed.  Go ahead.

Brian
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] setup: Always draw chooser icons using system colors

Igor Peshansky
On Wed, 24 May 2006, Brian Dessent wrote:

> Igor Peshansky wrote:
>
> > New iteration of the patch attached.  The ChangeLog is slightly different,
> > and is included below.  Again, comments welcome, but IMO this is ready to
> > be checked in (as soon as Brian looks it over and gives the go-ahead).  I
> > would also guess this is a good excuse for generating a new setup
> > snapshot.
>
> Warm fuzzies indeed.  Go ahead.

Thanks, done.  There are some pending patches/discussions, though, that it
might be a good idea to commit/resolve before firing off a snapshot build:

http://cygwin.com/ml/cygwin-apps/2006-03/msg00060.html
http://cygwin.com/ml/cygwin-apps/2006-03/msg00063.html (2 issues)
http://cygwin.com/ml/cygwin-apps/2006-03/msg00061.html
http://cygwin.com/ml/cygwin-apps/2006-03/msg00070.html
http://cygwin.com/ml/cygwin-apps/2006-02/msg00124.html

Those are the ones I've found.
        Igor
--
                                http://cs.nyu.edu/~pechtcha/
      |\      _,,,---,,_    [hidden email] | [hidden email]
ZZZzz /,`.-'`'    -.  ;-;;,_ Igor Peshansky, Ph.D. (name changed!)
     |,4-  ) )-,_. ,\ (  `'-' old name: Igor Pechtchanski
    '---''(_/--'  `-'\_) fL a.k.a JaguaR-R-R-r-r-r-.-.-.  Meow!

"Las! je suis sot... -Mais non, tu ne l'es pas, puisque tu t'en rends compte."
"But no -- you are no fool; you call yourself a fool, there's proof enough in
that!" -- Rostand, "Cyrano de Bergerac"