bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

AeroPeek Thumbbar buttons - set mImage and flags

NEW
Unassigned

Status

()

Firefox
Shell Integration
8 years ago
8 years ago

People

(Reporter: Felipe, Unassigned)

Tracking

Trunk
x86
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Created attachment 447923 [details] [diff] [review]
Patch

Hey Rob, this is the patch that we talked about earlier, setting the flags of THB_ICON in place. Thing is, I haven't been able to reproduce the disappearing icons anymore, so I don't know if this change is useful or totally unnecessary.

I'm posting the patch anyway as it's already written, so you can take a look. Not requesting review on it since I want to hear your opinion first.

(Also included a new attribute for the non-interactive flag. I don't know if you left it out on purpose or if it didn't exist back on RC days)

The one odd thing that I see, and I thought this patch would solve it but doesn't, is that setting the icon to null has no effect. I think it's a Windows thing of not letting the icon be cleared, as it appears that the DestroyIcon call has no effect. Setting it to a new, different icon works as expected.

There's also one way to make the buttons go crazy for a while, which is to make changes to the buttons while the flyout is open. I don't think it's a problem though because one would hardly be making lots of changes to the buttons anyway.
(In reply to comment #0)
> (Also included a new attribute for the non-interactive flag. I don't know if
> you left it out on purpose or if it didn't exist back on RC days)

They did not. Are there any existing apps that make use of them?

> The one odd thing that I see, and I thought this patch would solve it but
> doesn't, is that setting the icon to null has no effect. I think it's a Windows
> thing of not letting the icon be cleared, as it appears that the DestroyIcon
> call has no effect. Setting it to a new, different icon works as expected.

When we NULL out the icon, we also remove the flag. Also note that windows makes a copy of the icon to keep for itself so our DestroyIcon call has no effect on that. Since we clear the icon flags when we null out the .hIcon member, perhaps that is confusing Windows into thinking we're not setting an icon (as opposed to setting a NULL one). The API allows you to use an image list to set the icon for the buttons so maybe some code to handle that is getting invoked?

> There's also one way to make the buttons go crazy for a while, which is to make
> changes to the buttons while the flyout is open. I don't think it's a problem
> though because one would hardly be making lots of changes to the buttons
> anyway.

What sorts of changes cause them to go crazy? (not that we can do anything about it).
> (In reply to comment #0)
> > (Also included a new attribute for the non-interactive flag. I don't know if
> > you left it out on purpose or if it didn't exist back on RC days)
> 
> They did not. Are there any existing apps that make use of them?

Haven't seen any app using them yet. By tiling 2 or 3 non-interactive buttons side-by-side, and maybe rendering a msg on a canvas and creating an icon out of it, they could be kinda useful.

At the moment, in practice the same thing can be achieved by setting the button to disabled and no borders, but in theory Windows could style the disabled button differently (and it does that if we provided an image list)

>
> > The one odd thing that I see, and I thought this patch would solve it but
> > doesn't, is that setting the icon to null has no effect. I think it's a Windows
> > thing of not letting the icon be cleared, as it appears that the DestroyIcon
> > call has no effect. Setting it to a new, different icon works as expected.
> 
> When we NULL out the icon, we also remove the flag. Also note that windows
> makes a copy of the icon to keep for itself so our DestroyIcon call has no
> effect on that. Since we clear the icon flags when we null out the .hIcon
> member, perhaps that is confusing Windows into thinking we're not setting an
> icon (as opposed to setting a NULL one). The API allows you to use an image
> list to set the icon for the buttons so maybe some code to handle that is
> getting invoked?

Yeah, I thought about this flag thing, but even if I don't remove the flag (as it is without the patch), the same thing happens. I don't think the image list API is involved, but I'll take a look. Not a big deal since an image-less button is basically pointless.

Hmm right, Windows create its copy so DestroyIcon destroys our copy, not Windows'. Thinking about it, shouldn't we just instantly call DestroyIcon and free our copy (after Update(), of course), instead of just when the icon is switched?
 
> > There's also one way to make the buttons go crazy for a while, which is to make
> > changes to the buttons while the flyout is open. I don't think it's a problem
> > though because one would hardly be making lots of changes to the buttons
> > anyway.
> 
> What sorts of changes cause them to go crazy? (not that we can do anything
> about it).

Mostly when we hide/unhide a button while it's displayed (didn't try many other modifications). The buttons' layout temporarily cuts a button in half or join 2 of them. crazy
(In reply to comment #2)
> > (In reply to comment #0)
> > > (Also included a new attribute for the non-interactive flag. I don't know if
> > > you left it out on purpose or if it didn't exist back on RC days)
> > 
> > They did not. Are there any existing apps that make use of them?
> 
> Haven't seen any app using them yet. By tiling 2 or 3 non-interactive buttons
> side-by-side, and maybe rendering a msg on a canvas and creating an icon out of
> it, they could be kinda useful.
> 
> At the moment, in practice the same thing can be achieved by setting the button
> to disabled and no borders, but in theory Windows could style the disabled
> button differently (and it does that if we provided an image list)
> 
> >
> > > The one odd thing that I see, and I thought this patch would solve it but
> > > doesn't, is that setting the icon to null has no effect. I think it's a Windows
> > > thing of not letting the icon be cleared, as it appears that the DestroyIcon
> > > call has no effect. Setting it to a new, different icon works as expected.
> > 
> > When we NULL out the icon, we also remove the flag. Also note that windows
> > makes a copy of the icon to keep for itself so our DestroyIcon call has no
> > effect on that. Since we clear the icon flags when we null out the .hIcon
> > member, perhaps that is confusing Windows into thinking we're not setting an
> > icon (as opposed to setting a NULL one). The API allows you to use an image
> > list to set the icon for the buttons so maybe some code to handle that is
> > getting invoked?
> 
> Yeah, I thought about this flag thing, but even if I don't remove the flag (as
> it is without the patch), the same thing happens. I don't think the image list
> API is involved, but I'll take a look. Not a big deal since an image-less
> button is basically pointless.
> 
> Hmm right, Windows create its copy so DestroyIcon destroys our copy, not
> Windows'. Thinking about it, shouldn't we just instantly call DestroyIcon and
> free our copy (after Update(), of course), instead of just when the icon is
> switched?

Yeah, it looks that way. Technically we're supposed to wait until we get the "taskbariconcreated" message from Windows before we can make taskbar calls so actually the button code as is violates the spec in Microsoft's docs. Fixing it would mean we'd need to delay the API call to update the buttons until after we got the message which means we need to hold on to the HICON in that case.
 
> Mostly when we hide/unhide a button while it's displayed (didn't try many other
> modifications). The buttons' layout temporarily cuts a button in half or join 2
> of them. crazy

Weird. Well hopefully we can avoid modifying it in that way. Your PoC doesn't suffer from this problem, right?
You need to log in before you can comment on or make changes to this bug.