From: Morbus I. <mo...@di...> - 2001-07-02 14:14:13
|
I'm looking for a code review of the below - mainly in the realms of Windows correctness, and what I'm missing, and so forth. The code below is a drop-able library into my AmphetaDesk (http://www.disobey.com/amphetadesk/), but could work for some other programs as well. As it stands right now, I'm getting some minor reports of weirdness in Window minimization - when people click the taskbar button, nothing happens (only sometimes), when people click it again, it minimizes, and so forth. In some cases, I think I'm not properly "bringing Window to front". Anyways, please look at the code below and let me know where it can be improved (note if your email window is set for anything less than 80 characters, then you're gonna see some ugly wrapping): ############################################################################### # LIST OF ROUTINES BELOW: # ############################################################################### # gui_init() - start the gui and display the window # # gui_listen() - listen for window events for our gui # # gui_note() - send a note to our gui window # # open_url() - open a url in the system's default browser # # _things() - various routines that control the gui and should be private # ############################################################################### # these variables are global for # the Windows gui - they just keep # track of various things that listen # and notes need to know about. my ($hwnd, $icon, $hwnd_class, $window, $menu_bar); my ($img, $systray_icon, $systray_menu, $font, $logbox); # used to circumvent a bug that the popup menu functions # get called twice. A temp fix, while I look for the problem. my $already_called = 0; # remember the process id. my $fork_id_path; ############################################################################### # gui_init() - start the gui and display the window # ############################################################################### # USAGE: # # &gui_init; # # # # NOTES: # # This routine loads specific libraries specific to the OS and attempts to # # do everything necessary to start up a GUI window and start listening for # # events. Further down in this file, you should see supplementary GUI # # routines (starting with _) that are part of the GUI happenings this # # routine inits. # # # # Most of the code within this routine is thanks to David Berube. # # # # RETURNS: # # 1; this routine always returns happily. # ############################################################################### sub gui_init { use Win32::GUI; # hwnd is a handle to a window - basically, window's # way of keeping track of it's program windows... $hwnd = GUI::GetPerlWindow(); # comment this to see error messages in a dos window # otherwise, this will hide the blasted thing... GUI::Hide($hwnd); # get the width and height of the user's system. my $screen_width = Win32::GUI::GetSystemMetrics(0); my $screen_height = Win32::GUI::GetSystemMetrics(1); # create the icon handler. $icon = new Win32::GUI::Icon($SETTINGS->{files}->{gui_win_icon}); # create a window class for our window. $hwnd_class = new Win32::GUI::Class( -name => "$SETTINGS->{app}->{name} Class", -icon => $icon ); # set up our menu bar. these point to various # routines defined later on in this file, as # well as set up key commands for the items. $menu_bar = new Win32::GUI::Menu( "&File" => "File", " > E&xit" => "_FileExit", "&Edit" => "Edit", " > &Copy Ctrl+C" => "_EditCopy", " > Select &All Ctrl+A" => "_EditSelectAll" ); # creates the main window. notice that we use a generic # "Name" parameter, which is used for events, which must # have the format Name_EventName. $window = new Win32::GUI::Window( -name => '_Window', -text => $SETTINGS->{app}->{name}, -left => ($screen_width - 600)/2, -top => ($screen_height - 400)/2, -width => 480, -height => 400, -maxsize => [480, 400], -minsize => [480, 400], -menu => $menu_bar, -class => $hwnd_class, -icon => $icon, -maximizebox => 0 ); # create the systray icon. $systray_icon = $window->AddNotifyIcon( -name => "_Systray", -id => 1, -icon => $icon, -tip => $SETTINGS->{app}->{name} ); # Create the popup menu $systray_menu = new Win32::GUI::Menu( "SystrayMenu Functions" => "SystrayMenu", "> Open Window" => "_OpenWindow", "> Refresh Channels" => "_RefreshChannels", "> Exit" => "_SystrayExit" ); # create our pretty logo - we need to figure out # a good window size and height and then fix this # stuff (and the logbox) up... $window->AddLabel( -text => "", -name => "Bitmap", -left => -34, -top => -3, -width => 505, -height => 116, -style => 14, -visible => 1, ); # actually display the image... $img = new Win32::GUI::Bitmap($SETTINGS->{files}->{gui_win_logo}); $window->Bitmap->SetImage($img); $window->Bitmap->Resize(505, 116); # set the font of our log box below. $font = Win32::GUI::Font->new( -name => "Verdana", -size => 12 ); # create the log box which is gonna hold all our info. $logbox = $window->AddRichEdit( -name => "_RichEdit", -font => $font, -top => 116, -left => 0, -width => 505, -height => 240, -tabstop => 1, -style => WS_CHILD | WS_VISIBLE | ES_LEFT | ES_MULTILINE | ES_AUTOVSCROLL | WS_VSCROLL | ES_READONLY, -exstyle => WS_EX_CLIENTEDGE ); # and make it a little smaller than our window size. $logbox->Resize( $window->ScaleWidth, $window->ScaleHeight - 118); # finally, show all the junk we just did. $window->Show(); return 1; } 1; ############################################################################### # gui_listen() - listen for window events for our gui # ############################################################################### # USAGE: # # &gui_listen; # # # # NOTES: # # This routine checks the event queue and sees if there is anything that # # needs to be done. It's called from the main loop of our program. # # # # RETURNS: # # 1; this routine always returns happily. # ############################################################################### sub gui_listen { # anyone there? Win32::GUI::PeekMessage(0,0,0); Win32::GUI::DoEvents(); return 1; } 1; ############################################################################### # gui_note() - send a note to our gui window # ############################################################################### # USAGE: # # &gui_note("This is a gui window line. Yup."); # # # # NOTES: # # Much like ¬e, only we send our message to our os specific gui window. # # # # RETURNS: # # 1; this routine always returns happily. # ############################################################################### sub gui_note { my ($message) = @_; # select our last line. $logbox->Select(999999,999999); $logbox->ReplaceSel("$message\n", 1); select(undef, undef, undef, 0.25); # autoscroll the log box. $logbox->SendMessage (0x115, 1, 0) while $message =~ /\n|$/g; # listen for good measure. Win32::GUI::PeekMessage(0,0,0); Win32::GUI::DoEvents(); return 1; } 1; ############################################################################### # open_url() - open a url in the system's default browser # ############################################################################### # USAGE: # # &open_url( "http://127.0.0.1:8888/" ); # # # # OS SPECIFIC NOTES: # # This routine loads the Win32::Shell module to execute the "open" # # command which will open the browser and load the URL. However, if the # # user has defined a local path to a browser, we try to open that instead. # # # # RETURNS: # # 1; we instruct the user to open their browser if we can't. # ############################################################################### sub open_url { my ($url) = @_; # we spit out our suggestion just to catch all instances. ¬e("If your browser doesn't load, go to <$url>", 1); # find out what browser we're using. use Win32::TieRegistry; my $browser = $Registry->{"Classes\\http\\shell\\open\\command"}->{'\\'}; ¬e("Your registry states that $browser is your default program."); # if a browser_path hasn't been set, try # to open the default browser using the native API. # we do an API call instead of a 'start $url' sort # of thing, because the API call seems far more stable - # we've experienced slowness with 'start $url' # and random crashes on some machines. if ( $SETTINGS->{user}->{browser_path} eq "default" ) { use Win32::API; my $ShellExecute = new Win32::API("shell32", "ShellExecuteA", ['N','P', 'P', 'P', 'P', 'I'], 'N'); $ShellExecute->Call(0, "open", $url, 0, 0, 1); } # if a browser_path has been defined, try passing # the $url to the .exe and hope it understands. else { ¬e("But instead, we'll try loading $SETTINGS->{user}->{browser_path}."); unless ( $fork_id_path = fork ) { system( qq|"$SETTINGS->{user}->{browser_path} " $url| ); } } return 1; } 1; ############################################################################### # _things() - various routines that control the gui and should be private # ############################################################################### # USAGE: # # These are internally called by the GUI and shouldn't be publically # # used. So stop poking around here. Sheesh. Flippin' nosy people. Sigh. # ############################################################################### # various menu commands. sub _EditCopy_Click { ¬e("Received 'Copy' request."); $logbox->SendMessage(0x301,0,0); } sub _EditSelectAll_Click { ¬e("Received 'Select All' request."); $logbox->Select(0,999999);} sub _FileExit_Click { ¬e("Quitting $SETTINGS->{app}->{name} due to File > Exit."); &exit_program; } # re-open the browser window. sub _OpenWindow_Click { unless ( $already_called ) { # helloooooooo, nurse! ¬e("Received 'Open Window' request from Systray."); # do the dirty deed. &open_url( $SETTINGS->{urls}->{channels_home} ); # for some reason, we get two clicks every # time someone clicks a menu item. we check # for this here. $already_called = 1; } else { $already_called = 0; } } # Respond to the menu choice for # channel refreshing and browser opening. sub _RefreshChannels_Click { unless( $already_called ) { # helloooooooo, nurse! ¬e("Received 'Refresh Channels' request from Systray."); ¬e("--------------------------------------------------------------------------------", 1); ¬e("Refreshing the channel list and opening browser window...", 1); ¬e("--------------------------------------------------------------------------------", 1); # do the dirty deed. &download_my_channels(); &open_url( $SETTINGS->{urls}->{channels_home} ); # for some reason, we get two clicks every # time someone clicks a menu item. we check # for this here. $already_called = 1; } else { $already_called = 0; } } # generic code to force removal of systray icon. sub _Remove_Systray_Icon { Win32::GUI::NotifyIcon::Delete( $systray_icon->{-parent}, -id => $systray_icon->{-id} ); } # if the systray icon is clicked, re-enable the window. sub _Systray_Click { ¬e("Received Systray left click."); $window->Enable(); $window->Show; } # If someone right clicks the systray icon, show a menu. sub _Systray_RightClick { ¬e("Received Systray right click."); # get the x and y coords of the mouse to display the menu at. my($x, $y) = Win32::GUI::GetCursorPos(); # make the popup menu visible at the cursor $window->TrackPopupMenu($systray_menu->{SystrayMenu}, $x, $y-50); } # quit the program. we only do it this way so we can track what was clicked. sub _SystrayExit_Click { ¬e("Quitting $SETTINGS->{app}->{name} due to Systray > Exit."); &exit_program; } # hide the window if it's been minimized. the only # way to get it back would be from the systray icon. # we need to figure out what to do when people click # the "X' on the window. minimize? or close the app? sub _Window_Minimize { ¬e("Minimizing window."); $window->Hide(); } sub _Window_Terminate { ¬e("Quitting $SETTINGS->{app}->{name} due to window termination."); &exit_program; } # our exit code - called from various methods in the Windows # GUI (systray quit, window termination, file->exit, etc.) sub exit_program { # kill our children if they are there. if ( $fork_id_path ) { ¬e("Attempting to manually kill path process id: $fork_id_path."); kill( "TERM", $fork_id_path ); } # remove the icon. &_Remove_Systray_Icon; # ciao. exit; } 1; -- ICQ: 2927491 / AOL: akaMorbus Yahoo: morbus_iff / Jabber: mo...@ja... mo...@di... / http://www.disobey.com/ |
From: Johan L. <jo...@ba...> - 2001-07-02 21:00:16
|
Morbus wrote: >I'm looking for a code review of the below - mainly in the realms of >Windows correctness, and what I'm missing, and so forth. The code below is >a drop-able library into my AmphetaDesk >(http://www.disobey.com/amphetadesk/), but could work for some other >programs as well. Basically I would say it looks fine. I'm a bit curious about that double-event-thingy though, sounds a little odd. Ok, the code review, here goes : * The $SETTINGS global hash ref isn't documented anywhere and could probably be passed to the various subs instead. Globals config variables has always bitten me sooner or later in the past :/ * In open_url: use Win32::API; my $ShellExecute = new Win32::API("shell32", "ShellExecuteA", ['N','P', 'P', 'P', 'P', 'I'], 'N'); I would create the API object once at compile time (outside the sub) and check for success (or die("something")). That way you will have a safe state when the application starts--no surprises in the middle of everything you have to recover from. * What does $logbox->SendMessage(0x301,0,0); mean? You won't remember in two weeks either. Encapsulate in a generic and well-named sub that gives away what it is doing. Beats a comment any day (not that you shouldn't document the sub :) * sub _OpenWindow_Click { What happens if open_url takes a long time and the user double clicks? Set the "$already_called = 1;" immediately after checking it, making it as atomic as possible. As a personaly coding style, when I have a global variable ($already_called) which is in effect only a static variable (only used in that sub), I declare it in close proximity to the sub, i.e. just before the sub, and make sure it gets initialized with something useful. Readability and maintainability reasons. [oops, saw that you used it in many subs now :) the point is still valid in that context though] /J ------ ---- --- -- -- -- - - - - - Johan Lindström Boss Casinos Sourcerer jo...@ba... http://www.bahnhof.se/~johanl/ If the only tool you have is a hammer, everything tends to look like a nail |
From: Morbus I. <mo...@di...> - 2001-07-02 21:20:27
|
>* The $SETTINGS global hash ref isn't documented anywhere and could >probably be passed to the various subs instead. Globals config variables >has always bitten me sooner or later in the past :/ Point taken - in this case, it's part of the much larger AmphetaDesk project (at http://www.disobey.com/amphetadesk/), where it's documented in the main program file (amphetadesk.pl). Generically speaking, in my scripts, I use $CAPITALS for variables that are global to the whole program, and then small's for variables that should remain in the library or package. >* In open_url: > use Win32::API; > my $ShellExecute = new Win32::API("shell32", "ShellExecuteA", >['N','P', 'P', 'P', 'P', 'I'], 'N'); > >I would create the API object once at compile time (outside the sub) and >check for success (or die("something")). That way you will have a safe >state when the application starts--no surprises in the middle of everything >you have to recover from. Ah. Point taken, and a good one. I'll fix this in a later release. >* What does > > $logbox->SendMessage(0x301,0,0); > >mean? You won't remember in two weeks either. Encapsulate in a generic and >well-named sub that gives away what it is doing. Beats a comment any day >(not that you shouldn't document the sub :) In this case, I'm thinking that the sub name is fine: sub _EditCopy_Click The fact that it's the Copy command from the Edit menu, and that the user has "Click"ed on it, and that's the only line in the routine works good enough to tell me that "this esoteric command means to copy whatever's selected). >* sub _OpenWindow_Click { > >What happens if open_url takes a long time and the user double clicks? Set >the "$already_called = 1;" immediately after checking it, making it as >atomic as possible. Hmm. I wonder if that had anything to do with some of the slowdown I experienced before. I'll be sure to check into that. More questions: - do you see any Window related handles that I've missed? I'd much rather define every single possible event than miss some and hope that the code figures it out. Does my code always bring the Window to the top (in practice, sometimes it does, sometimes it doesn't)? -- Morbus Iff ( .sig on other machine. ) http://www.disobey.com/ && http://www.gamegrene.com/ "where's there's a will, there's a morbus ready to collect!" |
From: Johan L. <jo...@ba...> - 2001-07-03 06:47:51
|
Morbus wrote: > - do you see any Window related handles that I've missed? > I'd much rather define every single possible event than miss > some and hope that the code figures it out. Does my code > always bring the Window to the top (in practice, sometimes > it does, sometimes it doesn't)? Just looking at the code, I would make sure the event handlers always return exactly what I think they should: >sub _Window_Minimize { ¬e("Minimizing window."); $window->Hide(); } Return 1 explicitly to avoid surprises. /J ------ ---- --- -- -- -- - - - - - Johan Lindström Boss Casinos Sourcerer jo...@ba... http://www.bahnhof.se/~johanl/ If the only tool you have is a hammer, everything tends to look like a nail |