|
Joel,
A seventh option that would work with "legacy" applications and ".NET" applications alike, is to use DCOM. From the ".NET" perspective, this is virtually identical to use remoting.
--
Paul
"I drank... WHAT?"
|
|
|
|
|
1) the uint( UInt32 ) data type is not CLS compliant. I suggest you use Int64 or Int32 instead in the public ShowBallon method, and then convert it to an uint internally.
2) I believe the Dispose override should call base.Dispose() after Remove()
3) Make more virtual methods, so we can subclass this effectively.
4) I suggest you take a look at the standard pattern for event raising across the BCL. things you should consider doing along this line:
4a) make the NotifyIconHandler delegate a void NotifyIconTargetEventHandler( Object sender, NotifyIconTargetEventArgs e )
4b) subclass EventArgs to make NotifyIconTargetEventArgs with the Id as a get-only property.
4c) make the OnEVENT methods protected, virtual, and only receive the NotifyIconTargetEventArgs
4d) use the onEVENT methods to determine wether the event is null, and raise it with "this"
4e) use EventArgs.Empty instead of new EventArgs()
5) maybe change ClickBalloon event to BalloonClick, the former sounds more like a method that _does_ the clicking.
overall, very usefull code. good job.
|
|
|
|
|
In answer to your points:
1) the uint( UInt32 ) data type is not CLS compliant. I suggest you use Int64 or Int32 instead in the public ShowBallon method, and then convert it to an uint internally.
- Picky but point taken.
2) I believe the Dispose override should call base.Dispose() after Remove()
- I will fix this.
3) Make more virtual methods, so we can subclass this effectively.
- I am married with 4 kids so you do it
4) I suggest you take a look at the standard pattern for event raising across the BCL. things you should consider doing along this line:
4a) make the NotifyIconHandler delegate a void NotifyIconTargetEventHandler( Object sender, NotifyIconTargetEventArgs e )
4b) subclass EventArgs to make NotifyIconTargetEventArgs with the Id as a get-only property.
- Somebody needs to explain why I should do this since it is just more work than passing the id. Maybe I am missing something. Why bother creating a new class just to pass an id. Just because that's what the .NET library does?
4c) make the OnEVENT methods protected, virtual, and only receive the NotifyIconTargetEventArgs
4d) use the onEVENT methods to determine wether the event is null, and raise it with "this"
- That would be just the way you would do it maybe. You need to give good reasons if you are going to make this kind of suggestion.
4e) use EventArgs.Empty instead of new EventArgs()
- Why? To save memory on the heap? The docs don't say this but I assume it does otherwise what's the point?
Wow, you are pretty picky dude
|
|
|
|
|
Joel Matthias wrote:
4a) make the NotifyIconHandler delegate a void NotifyIconTargetEventHandler( Object sender, NotifyIconTargetEventArgs e )
4b) subclass EventArgs to make NotifyIconTargetEventArgs with the Id as a get-only property.
- Somebody needs to explain why I should do this since it is just more work than passing the id. Maybe I am missing something. Why bother creating a new class just to pass an id. Just because that's what the .NET library does?
Well, it's more than just what the library does.
A) It's promotes consistency. which helps people reading your code
B) Following this allows any method to handle any event with the base "void ( object, eventargs )" signature.
Joel Matthias wrote:
4c) make the OnEVENT methods protected, virtual, and only receive the NotifyIconTargetEventArgs
4d) use the onEVENT methods to determine wether the event is null, and raise it with "this"
- That would be just the way you would do it maybe. You need to give good reasons if you are going to make this kind of suggestion.
The reason for the method attribute changes is so that subclassers of this class can use the standard and good practice of overriding the OnEVENT method to extend behavior. The purpose for the changes in parameters, is, yes, to be consistent with the all the other hundreds of event raising methods.
If you are a coder of this level that doesn't believe in the virtues of having a consistent interface... then I doubt some comment in the forums is going to change that. take what you will from this.
Joel Matthias wrote:
4e) use EventArgs.Empty instead of new EventArgs()
- Why? To save memory on the heap? The docs don't say this but I assume it does otherwise what's the point?
EventArgs.Empty is a static instance of the EventArgs class. basically, every EventHandler in the BCL uses this same instance of EventArgs for an empty args. Using EventArgs.Empty not only stops the creation of a new instance needlessly, it also is one less object that needs to be GC'd.
yes, I know I'm picky... but I don't mean to say your code is bad. I just think that the more programmers using Best Practices, the better
|
|
|
|
|
Andy Smith wrote:
Joel Matthias wrote:
4a) make the NotifyIconHandler delegate a void NotifyIconTargetEventHandler( Object sender, NotifyIconTargetEventArgs e )
4b) subclass EventArgs to make NotifyIconTargetEventArgs with the Id as a get-only property.
- Somebody needs to explain why I should do this since it is just more work than passing the id. Maybe I am missing something. Why bother creating a new class just to pass an id. Just because that's what the .NET library does?
Well, it's more than just what the library does.
A) It's promotes consistency. which helps people reading your code
B) Following this allows any method to handle any event with the base "void ( object, eventargs )" signature.
I have to question the usefulness of B) being able to handle an event with a generic method that does not need know what the actual event argument is. I just don't see that as being useful and have not seen an example of this type of code. I think the trade off of making the code simpler is worth it. I think there is a school of thought that says don't make things complicated just because someone someday may want to use your class in a certain way.
Andy Smith wrote:
Joel Matthias wrote:
4c) make the OnEVENT methods protected, virtual, and only receive the NotifyIconTargetEventArgs
4d) use the onEVENT methods to determine wether the event is null, and raise it with "this"
- That would be just the way you would do it maybe. You need to give good reasons if you are going to make this kind of suggestion.
The reason for the method attribute changes is so that subclassers of this class can use the standard and good practice of overriding the OnEVENT method to extend behavior. The purpose for the changes in parameters, is, yes, to be consistent with the all the other hundreds of event raising methods.
If you are a coder of this level that doesn't believe in the virtues of having a consistent interface... then I doubt some comment in the forums is going to change that. take what you will from this.
I'm not sure I am familiar with the practice of overriding 'OnEvent'. Is this a .NET practice because I couldn't find OnEvent in the .NET help files? Again I think you are just trying to make the class as flexable as possible which is fine but you are going to sacrifice simplicity with this approach.
Don't get me wrong you made some valid points which I have noted and I will update my code. I really did not intend to make this class generic and extendable. For one I wanted to keep is simple and my free time is somewhat limited.
Cheers - Joel
|
|
|
|
|
Joel Matthias wrote:
I think the trade off of making the code simpler is worth it. I think there is a school of thought that says don't make things complicated just because someone someday may want to use your class in a certain way.
yes, that school of thought is the classic Worse it better approach. .Net is is designed to be The Right Thing© instead.
Joel Matthias wrote:
I'm not sure I am familiar with the practice of overriding 'OnEvent'.
I mean "Event" in the generic sense. Look thru the BCL. For every single ( I believe ) public event Foo, you will see a protected virtual OnEvent method. The function of this method is to check the event for null, and raise it if so. the reason for the accessability is for subclassers.
|
|
|
|
|
Andy Smith wrote:
I mean "Event" in the generic sense. Look thru the BCL. For every single ( I believe ) public event Foo, you will see a protected virtual OnEvent method. The function of this method is to check the event for null, and raise it if so. the reason for the accessability is for subclassers.
I looked at the original .NET NotifyIcon and none of the events have a virtual OnEVENT finction associated with them.
And one more thing (in my defense). The hidden window class is a completely private internal class that is not designed to be derived from so I don't feel too bad about not making it easy to do so.
Anyway I have updated the code with some of your suggestions.
Regards - Joel
|
|
|
|
|
Joel Matthias wrote:
I looked at the original .NET NotifyIcon and none of the events have a virtual OnEVENT finction associated with them.
You're right, the NotifyIcon class doesn't have the protected virtual OnEVENT methods because it is sealed, thus a protected method isn't going to do anything for it :-P
For non-sealed classes though, that is the standard and like it or not; if you deviate from the standard you increase the chance people are going to have a problem working with your code then they'll blame your code for not working.
James
Simplicity Rules!
|
|
|
|
|
So I'll just make the class sealed, ha! But seriously how DO you decide whether to seal a class or not.
Joel
|
|
|
|
|
From My Experience©
You seal a class when:
It doesn't make any sense to derive from it ( like most subclasses of EventArgs )
When you've made the class part of a pattern where subclassing it has a high likelyhood of breaking the pattern in subtle ways.
You've made assumptions about the internal layout of the resulting IL, and subclassing it would break those assumptions.
When you feel like being a jerk.
|
|
|
|
|
Andy Smith wrote:
When you feel like being a jerk.
Ouch! Ohhhh, you meant the guy at Microsoft who made their NotifyIcon class sealed is a jerk. For a minute.....
Joel
|
|
|
|
|
Joel Matthias wrote:
1) the uint( UInt32 ) data type is not CLS compliant. I suggest you use Int64 or Int32 instead in the public ShowBallon method, and then convert it to an uint internally.
- Picky but point taken.
IIRC VB.NET doesn't support unsigned types.
James
Simplicity Rules!
|
|
|
|
|
what happens with the balloon when people don't have the newer version of windows/ie/whatever that enables the balloon?
if you haven't addressed that, maybe you should just make it a dialogbox with only an OK button.
|
|
|
|
|
I could do but in a commercial app I would probably ship the redistributable which would update their system to be able to use this feature. (I think it may be in ComCtl32 which MS provides a redist for.
Joel
|
|
|
|
|
I have nt4, with all the latest "other crap", like ie and whatnot, that I know of, and I cannot get any balloons to show up.
From what I can tell, it's just flat out not possible to do in NT4. I know it works in XP as I've seen it. And pages I've scanned seemed to indicated that the balloon was available starting with W2K. Can anybody with Win98/ME test it as well?
Are there any other kinds of fire-and-forget style notification options available for older systems?
|
|
|
|
|
So you have IE6? If so then the comments in the header file that check for IE versions are a bit misleading. Have you tried getting the latest COMCTL32 redistributable?
I have tested it on WinME and it works (except for the x button on the balloon). I will try to test it on Win98.
Joel
|
|
|
|
|
this page claims that the latest version always comes with IE. And I couldn't find a newer redist than v5.
|
|
|
|
|
Have you looked at ms-help://MS.VSCC/MS.MSDNVS/shellcc/platform/Shell/Versions.htm (try pasting the link into the VS.NET help address box.
Joel
|
|
|
|
|
sorry, don't have visual studio.
|
|
|
|
|
|
and I quoteth from that page:
Downloading the Latest Version
To get the most recent version of the Shell and common controls DLLs, download and install Internet Explorer.
so it looks like it's just not gonna be possible to show a balloon on nt4.
which tells me why they chose not to include it in the standard notifyicon.
|
|
|
|
|
Andy Smith wrote:
so it looks like it's just not gonna be possible to show a balloon on nt4.
Looks like it - bummer. Let me know when you have coded up a substitute for use on NT 4 . The first problem I ran into was determining the screen coords of the target notify icon. Of course you could just show a popup window in the lower right corner, just above the task bar and not worry about the exact position of the notify icon (I think the Yahoo messenger does this).
Nothing's ever simple is it. Of course you could take the approach of saying the balloon is not a feature of the OS and as such you shouldn't try to use it. But what fun would that be.
Regards - Joel
|
|
|
|
|
Joel Matthias wrote:
Let me know when you have coded up a substitute for use on NT 4
actually, I spent some time today, and created a NotifyPopup component which acts like the little square popups that MSN Messenger has.
The only problem I have left is figuring out what side of the screen the taskbar is on. I could probably do it with a simple DLLImport or something, but i'm trying to figure out how to do it without that, because so far I haven't had to.
|
|
|
|
|
Andy Smith wrote:
actually, I spent some time today, and created a NotifyPopup component which acts like the little square popups that MSN Messenger has.
Cool.
Andy Smith wrote:
The only problem I have left is figuring out what side of the screen the taskbar is on. I could probably do it with a simple DLLImport or something, but i'm trying to figure out how to do it without that, because so far I haven't had to.
Yes, It looks like if you need to do anything very powerful/low level you have to fall back on the Win32 API.
The problem I face is finding whether a particular piece of information is available from the .NET class library and if so where the heck it is located.
We need a book that will make finding these classes/functions easy.
Joel
|
|
|
|
|
How's this for a delayed response to your post of April 16 on CP...
Andy Smith wrote:
actually, I spent some time today, and created a NotifyPopup component which acts like the little square popups that MSN Messenger has.
Any chance I could get you to share this?
Andrew Connell
IM on MSN
andrew@aconnell.com
|
|
|
|