This is the mail archive of the cygwin-apps mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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  <pechtcha@cs.nyu.edu>

	* 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/
      |\      _,,,---,,_	    pechtcha@cs.nyu.edu | igor@watson.ibm.com
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"

Attachment: setup-bitmap-colors.patch
Description: Text document


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]