[Bugfix] Simple code for trayicons, please check...

Hi, Experts.

I created a nice and very simple piece of code for a trayicon interface to be used in very small applications. (It still depends on some VCL units but hey, a console application with this unit will still be smaller than 60 KB.)

I fear, however, that there are quite a few minor flaws in this code, so I need volunteers to play around with it, to spot errors and provide solutions. The sourcecode is at http://www.workshop-alex.org/Sources/TrayIcon/untTrayIconThread.html if you're interested.

There will be 100 points for a real major bug, 50 points for minor bugs and 20 points for nice suggestions. No more than 500 points will be given away, though, and those who spot bugs first will be the first to receive points. (But hey, feel free to continue making suggestions.) You yourself should describe if it's a major bug, minor bug or just a suggestion and I will check if you're right.

Oh, I know this trayicon object doesn't have much functionality. This is on purpose, since it's supposed to be used for debugging purposes only for other application. I would enable the debug mode in some way and the trayicon would provide me a way to make dumps of the internal process whenever I want to dump them. Of course, it could be made into a much more interesting thing or even a component, but for me it's extremely important that it doesn't need too much additional overhead.

Usage of this thing in e.g. a console application:

program Project1;

{$APPTYPE CONSOLE}

uses
  SysUtils,
  untTrayIconThread in 'untTrayIconThread.pas',
  untExtraInterfaces in 'untExtraInterfaces.pas';

type
  TDummy = class
    procedure SayHello;
    procedure SayBye;
  end;

var
  TrayIcon: ITrayIcon;
  Dummy: TDummy;

procedure TDummy.SayBye;
begin
  WriteLn('Bye');
end;

procedure TDummy.SayHello;
begin
  WriteLn('Hello');
end;

begin
  Dummy := TDummy.Create;
  TrayIcon := NewTrayIcon( 'TestAppA', 'Just a simple test.' );
  TrayIcon.AddTrayAction( 'Hello', Dummy.SayHello );
  TrayIcon.AddTrayAction( 'Bye', Dummy.SayBye );
  TrayIcon.Suspended := False;
  WriteLn( 'Press <Enter> to quit. ' );
  ReadLn;
  TrayIcon := nil;
  Dummy.Free;
end.

Feel free to give any comments about it that you like.
And if you feel this code has been useful for you, don't hesitate to provide positive feedback in my member profile. ;-)
LVL 17
Wim ten BrinkSelf-employed developerAsked:
Who is Participating?
I wear a lot of hats...

"The solutions and answers provided on Experts Exchange have been extremely helpful to me over the last few years. I wear a lot of hats - Developer, Database Administrator, Help Desk, etc., so I know a lot of things but not a lot about one thing. Experts Exchange gives me answers from people who do know a lot about one thing, in a easy to use platform." -Todd S.

kretzschmarCommented:
looks good, alex, but by my lack of time, i can't help,
therefore following this thread
0
Lee_NoverCommented:
cool idea about the links for units in the uses clause .. can I use that in my pas2html.dll ? :)
I'll check the TI out ..
0
Wim ten BrinkSelf-employed developerAuthor Commented:
@Meikl, No problem. I'm not in a hurry. I haven't noticed anything wrong with it yet but just doesn't have enough time to text this thoroughly especially since I will only be using this code for debugging purposes of some other projects. (The trayicon allows me to dump debug information while the application just keeps running.)

@Lee, feel free to use this code for anything you like. I used GExperts to generate the HTML output, btw. Just edited the page to add a link to the second unit that it needs.

I don't expect any real nasty issues with this code and think it should run anywhere. If I had more time available I would remove the SysUtils unit from the code too. But am using that unit for some exception handling... The "TrayAction" method executes some arbitrary code defined elsewhere and theoretically this code could raise an exception. If I don't catch it then it crashes the thread. And if I do catch it, I should display it since there's nothing else that will be handling this error. Not using SysUtils would make the code a bit more unstable.
0
Cloud Class® Course: Certified Penetration Testing

This CPTE Certified Penetration Testing Engineer course covers everything you need to know about becoming a Certified Penetration Testing Engineer. Career Path: Professional roles include Ethical Hackers, Security Consultants, System Administrators, and Chief Security Officers.

HypoviaxCommented:
Hmmm....

What's this code meant to do? Because when i run your sample app (having downloaded the needed units etc) nothing really happens. All i get is the <Press Enter to Exit> line.

Regards,

Hypoviax
0
Wim ten BrinkSelf-employed developerAuthor Commented:
@Hypoviax, you should see a trayicon next to all other trayicons. However, since a console application doesn't have an icon, it's actually a simple gray block. You should add an icon to the console application by modifying the resource file. :-)
The trayicon is there, however. If you hold your mouse next to the right-most trayicon on the gray area, you probably would see a hint window pop up. If you click on it, a menuoption would have to be visible, showing two options. In the meantime, the console itself is just waiting for you to terminate the application again by pressing <Enter> which would kill the trayicon again.
If you would create a GUI application, the trayicon should be visible.
0
HypoviaxCommented:
It works well GUI. No problemo.

Comparitively this may interest you. It's written for GUI apps but it may be useful comparing to what you have:

uses ..., ShellApi;

type
  TForm1 = class(TForm)
    PopupMenu1: TPopupMenu; // popup menu for the tray with two options: show and exit
    mnuTrayShow: TMenuItem;
    mnuTrayExit: TMenuItem;
     procedure ToTray;
     procedure FormCreate(Sender: TObject);
    procedure FormActivate(Sender: TObject);
    procedure FormClose(Sender: TObject; var Action: TCloseAction);
    procedure mnuTrayShowClick(Sender: TObject);
    procedure mnuTrayExitClick(Sender: TObject);
  private
    { Private declarations }
  protected
    procedure WndProc(var Msg : TMessage); override;
  public
    { Public declarations }
    IconNotifyData : TNotifyIconData;
  end;

var
  Form1: TForm1;

implementation

{$R *.DFM}

uses unit2, Tools;

procedure TForm1.ToTray;
begin
  Form1.Hide;
end;

procedure TForm1.WndProc(var Msg : TMessage);
var
   p : TPoint;
begin
  case Msg.Msg of
    WM_USER+1:
    case Msg.lParam of
      WM_RBUTTONUP: begin // show popup menu with two options: show and exit
        GetCursorPos(p);
        PopupMenu1.Popup(p.x, p.y);
      end;
      WM_LBUTTONUP: begin
         mnuTrayShowClick(Self);
      end;
    end;
  end;
  if (Msg.Msg = WM_SYSCOMMAND) and (Msg.wParam = SC_MINIMIZE) then begin
    ToTray;
    Msg.wParam := 0;
    Msg.lParam :=0;
  end;
  inherited;
end;

procedure TForm1.FormCreate(Sender: TObject);
begin
  with IconNotifyData do begin
    hIcon :=Application.Icon.Handle;
    uCallbackMessage := WM_USER + 1;
    cbSize := sizeof(IconNotifyData);
    Wnd := Handle;
    uID := 100;
    uFlags := NIF_MESSAGE + NIF_ICON + NIF_TIP;
  end;
  StrPCopy(IconNotifyData.szTip, 'CD FTP');
  Shell_NotifyIcon(NIM_ADD, @IconNotifyData);
end;

procedure TForm1.FormActivate(Sender: TObject);
begin
  ShowWindow(Application.Handle, SW_HIDE);
  SetWindowLong(Application.Handle, GWL_EXSTYLE, WS_EX_TOOLWINDOW);
end;

procedure TForm1.FormClose(Sender: TObject; var Action: TCloseAction);
begin
  Shell_NotifyIcon(NIM_DELETE, @IconNotifyData);
  Application.ProcessMessages;
end;

procedure TForm1.mnuTrayShowClick(Sender: TObject);
begin
  Form1.Show;
  Application.BringToFront;
end;

procedure TForm1.mnuTrayExitClick(Sender: TObject);
begin
  Close;
end;

end.

Regards,

Hypoviax

0
Wim ten BrinkSelf-employed developerAuthor Commented:
Interesting, yes. But I wrote that other code myself to be part of some project I'm working on. Took me 4 hour to write and test it but I'm just not sure if it's really bugfree, which is why I posted this question. ;-)
I am quite familiar with trayicons, though. It's just that I want to have a non-intruisive way to use a trayicon for debugging purposes, which above unit archieves since it uses a separate thread and messageloop for each trayicon it generates. Your solution just hooks into the messageloop of an existing form. My trayicon could technically be used in a DLL or a console application and probably in a few other project types too.

My fear is that it just seems to be to easy to be bugfree. See, This unit combines several advanced Windows techniques. Multi-threading, trayicons, messageloops and critical sections. But it's less than 300 lines and it seems a bit too simple for me to be 100% confident about it. Still, I found no flaws with it. (except for invisible icons if the application has no icon.) Since I ran out of time to test this code, I just thought, let's share it and see if someone else can find any flaws in it. (Which is why I give it away too, free of charge.)

And hey, perhaps it is flawless and maybe someone else has a good use for it too. :-)

Oh, for a GUI application you could just do this:

uses ..., untTrayIconThread;

type
  TForm1 = class(TForm)
    procedure FormCreate(Sender: TObject);
    procedure FormClose(Sender: TObject; var Action: TCloseAction);
  private
    { Private declarations }
    TrayIcon: ITrayIcon;
  protected
    procedure SayHello;
    procedure SayGoodbye;
  public
    { Public declarations }
  end;

var
  Form1: TForm1;

implementation

{$R *.DFM}

procedure TForm1.FormCreate(Sender: TObject);
begin
  TrayIcon := NewTrayIcon( 'TestAppA', 'Just a simple test.' );
  TrayIcon.AddTrayAction( 'Hello', SayHello );
  TrayIcon.AddTrayAction( 'Bye', SayGoodbye );
  TrayIcon.AddTrayAction( 'Show', Show );
  TrayIcon.AddTrayAction( 'Hide', Hide );
  TrayIcon.Suspended := False;
end;

procedure TForm1.FormClose(Sender: TObject; var Action: TCloseAction);
begin
  TrayIcon := nil;
end;

procedure TForm1.SayHello;
begin
  ShowMessage('Hello');
end;

procedure TForm1.SayGoodbye;
begin
  ShowMessage('Goodbye');
end;

end.

Be aware however that SayHello and SayGoodbye will NOT be executed in the main thread but in the thread related to the trayicon. Thus you can execute very long actions here, as long as you keep it thread-safe. Of course, the trayicon could also show and hide the application. You don't even need to add special functions in those cases since the methods Show and Hide are already of the correct type. Of course, I could overload the AddTrayAction method to accept some other kinds of callback functions too. (In which case I will need to remember the callback function type too and use the correct callback technique. But Hey, I don't have time for this.)
I did discover one flaw in a GUI environment, though. After:
  TrayIcon.AddTrayAction( 'Close', Close );
I get a nice Close action in my trayicon but since the Close is called from a different thread than the thread where the form was created, this will result in a nasty exception. Or actually 2:
> Exception EExternalException in module ntdll.dll at 0001FFE4.
> External exception C0000008.
and
> Exception EWin32Error in module SMSMessenger.exe at 0000A76C.
> Win32 Error.  Code: 5.
> Access is denied.
I've seen similar exceptions in other applications. Now I know why I might get them in my multi-threaded apps.
0
geobulCommented:
Hi Workshop_Alex,

Looks very good indeed. Somehow I've missed this question and saw your last one first.

Just looking at the source code without testing anything and going deeper in understanding it well, there is something that perhaps could become a problem (it is very unlikely to happen or even impossible but anyway): You use CriticalSection.TryEnter (5 attempts) and don't handle situations if it fails. TTrayIcon.AddTrayAction is a good example: If TryEnter fails no action will be added and there is no negative feedback pointing that.

If the above is nonsense, please ignore my comment.

Regards, Geo
0
Wim ten BrinkSelf-employed developerAuthor Commented:
Good comment, Geo. But it's actually done on purpose. :-)
TryEnter does allow you to specify the number of tries but by default it tries 5 times. If it cannot enter the critical section by that time, then I have to assume something is a bit too busy at that moment. The problem however is that the TrayIcon has it's own thread and raising exceptions within it will crash and terminate the thread if unhandled.
I've looked at the code after the TryEnter calls, btw. TryEnter will call a Sleep(0) if it can't enter the CS, so the other thread that keeps it locked gets time to continue processing. I don't think it's still locked after 5 tries.

However, in AddTrayAction I am in the main thread or whatever other thread is running at that moment so I could raise an exception at that location, which would have to be caught somewhere else.

I am evaluating now if I also need to check this critical section in the DeleteTrayAction method. I think I do have to add it there too, so you've pointed out a possible bug. (One which I never detected because I never deleted actions.)

One thing I fortunately noticed is that you can add actions from within an action. I don't execute the action from within the critical section, but just outside it.

Your comments make sense, though. One minor bugfix coming up soon. :-)
0
geobulCommented:
My thoughts especially about AddTrayAction were that you could either use CriticalSection.Enter or make it a function returning a boolean result (the latter is not a standard for AddSomething methods, though), without raising an exception. I see what your point for using TryEnter is. On the other hand I have no information how EnterCriticalSection is implemented actually. Perhaps it also does something like you're alter.

My understanding of critical sections is that each provides access to a resource (or set of resources). If in that particular case the CS protects TrayActionList property then you have to use it everywhere you're accessing that property. Including DeleteTrayAction.

One another thing: in WindowProc function there is:
if TrayIcon.CriticalSection.TryEnter and ( Length( TrayIcon.TrayActionList ) > 0 ) then begin
Perhaps if you switch the conditions it'll be more correctly. The reason is that you may enter the CS but if there are no actions in the list and CS.Leave won't be executed.

Regards, Geo
0

Experts Exchange Solution brought to you by

Your issues matter to us.

Facing a tech roadblock? Get the help and guidance you need from experienced professionals who care. Ask your question anytime, anywhere, with no hassle.

Start your 7-day free trial
geobulCommented:
The last one is valid if {$B-} option is used. Otherwise splitting that IF into to would be better:

    if ( Length( TrayIcon.TrayActionList ) > 0 ) then begin
      if TrayIcon.CriticalSection.TryEnter then begin
        for Index := Low( TrayIcon.TrayActionList ) to High( TrayIcon.TrayActionList ) do begin
          AppendMenu( Menu, MFT_STRING, MENUITEM_ACTIONS + Index, PChar( TrayIcon.TrayActionList[ Index ].Name ) );
        end;
        TrayIcon.CriticalSection.Leave;
      end;
    end;
0
Wim ten BrinkSelf-employed developerAuthor Commented:
Hmmm. I will have to enter the CS before I check the length of TrayActionList because an AddAction or DeleteAction could be changing the length at the same time. TrayActionList is the resource that needs to be protected here. I am not too worried about the items in this list since these are simple records that cannot be modified. They are either added or removed from the list. And when I use one in the WindowProc, I just make a copy of this record before using it, thus if the action is deleted from the list before I call it, I still have valid data.
I've modified AddTrayAction and DeleteTrayAction to just Enter the CS, without trying. But in the WindowProc I keep using TryEnter because this code is less critical in my opinion. If it fails after 5 attempts then too bad... I just don't want to block this thread too long. Also made sure to use a try-finally to make sure the Leave is always called.

Okay, so far Geo's comments resulted into the discovery of two minor bugs: the DeleteAction which was missing CS support and the {$B+} flaw. (An option which I NEVER use anyway.) And a good suggestion about the CS Enter/TryEnter code...
Still 380 points to divide left. ;-)
0
geobulCommented:
Yep, I agree that entering the CS should be first. So, swapping the conditions in one if statement is not a valid option. The only option that remains is:

    if TrayIcon.CriticalSection.TryEnter then begin
      if ( Length( TrayIcon.TrayActionList ) > 0 ) then begin
        for Index := Low( TrayIcon.TrayActionList ) to High( TrayIcon.TrayActionList ) do begin
          AppendMenu( Menu, MFT_STRING, MENUITEM_ACTIONS + Index, PChar( TrayIcon.TrayActionList[ Index ].Name ) );
        end;
      end;
      TrayIcon.CriticalSection.Leave;
    end;
0
Wim ten BrinkSelf-employed developerAuthor Commented:
Yep. But if you look at the source that I've linked to, you'll notice that I've already updated it to:

      if TrayIcon.CriticalSection.TryEnter then try
        if ( Length( TrayIcon.TrayActionList ) > 0 ) then begin
          for Index := Low( TrayIcon.TrayActionList ) to High( TrayIcon.TrayActionList ) do begin
            AppendMenu( Menu, MFT_STRING, MENUITEM_ACTIONS + Index, PChar( TrayIcon.TrayActionList[ Index ].Name ) );
          end;
        end;
      finally
        TrayIcon.CriticalSection.Leave;
      end;

The additional try-except should make it a bit more safe. There is a very slim chance that accessing TrayActionList results in an exception.

I don't like it though, that I cannot report any exceptions to the application but I need to keep this code multi-functional. I would hate to see an ISAPI dll that displays a messagebox because this code generated one... :-) Who is going to click it away? ;-)
0
geobulCommented:
Try..finally is better because it's more safe. I have to refresh my browser from time to time ;-)
0
Wim ten BrinkSelf-employed developerAuthor Commented:
Okay, closing up...

Bugs found:
kretzschmar: Thanks for the comment. :-)
Lee_Nover: You should award me points for giving you such an interesting suggestion for your pas2html.dll. ;-) But okay, this one is free.
Hypoviax: Do you know now what it's supposed to do? ;-) Oh, well... You're right. I need some example code for this unit sooner or later... And some help system, perhaps.
geobul: Yep, it looks good. :-) The part about the CriticalSection.TryEnter is a good suggestion, though. One minor bug...
geobul: Good comment about the CS that in some rare occasions might be entered but never leaves. Second minor bug.

Okay, Geo seems to be the only one who found some problems. Hypoviax provided some interesting piece of code that might be useful for some. All others, thanks for participating.

Dividing:
geobul: 420 points for fuinding the minor bugs
Hypoviax: 40 points for participating and the code snippet.
kretzschmar: 20 points for participating. :-)
Lee_Nover: 20 points for participating. :-)

I now know that 4 experts have at least looked at it and found it okay. Very reassuring. :-)
0
Lee_NoverCommented:
:D tnx
0
Wim ten BrinkSelf-employed developerAuthor Commented:
You're welcome... :)
0
HypoviaxCommented:
Thanks,

You've become a bit generous of late.

Number 2 from the Wizard

Regards,

Hypoviax
0
It's more than this solution.Get answers and train to solve all your tech problems - anytime, anywhere.Try it for free Edge Out The Competitionfor your dream job with proven skills and certifications.Get started today Stand Outas the employee with proven skills.Start learning today for free Move Your Career Forwardwith certification training in the latest technologies.Start your trial today
Delphi

From novice to tech pro — start learning today.

Question has a verified solution.

Are you are experiencing a similar issue? Get a personalized answer when you ask a related question.

Have a better answer? Share it in a comment.