current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 5 down vote favorite
1

I wrote this script for a user who wants to toggle the minimized state of a window that minimizes to the tray. The user required a .bat script, so I wrote a bat + PowerShell hybrid script in order to import functions from user32.dll.

<# : minimize.bat
:: toggles minimized state of a window by its filename
:: minimize.bat /? for usage

@echo off & setlocal

if "%~1"=="" goto usage
set "prog=%~n1"
tasklist | findstr /i "\<%prog%\>" >NUL || goto usage

set /P "=Toggling the minimized state of %prog%... " <NUL
powershell -noprofile -noninteractive "iex ((gc \"%~f0\") -join \"`n\")"

goto :EOF

:usage
echo syntax: %~nx0 progname[.exe]
echo;
echo If the program is visible, minimize it.  If minimized, restore it.
goto :EOF

End batch / begin PowerShell hybrid chimera #>

Add-Type user32_dll @'
    [DllImport("user32.dll")]
    public static extern bool ShowWindowAsync(IntPtr hWnd, int nCmdShow);

    [DllImport("user32.dll")]
    public static extern int GetWindowLong(IntPtr hWnd, int nIndex);

    [DllImport("user32.dll")]
    public static extern IntPtr FindWindowEx(IntPtr parentHandle, IntPtr childAfter,
        IntPtr lclassName, string windowTitle);
'@ -namespace System

$hwnd = (ps $env:prog)[0].MainWindowHandle

if ($hwnd -eq 0) {
    tasklist /v /fi "imagename eq $env:prog*" /fo list | %{
        $title = $_ -replace '^[^:]+:\s+'
    }
    $zero = [IntPtr]::Zero
    $hwnd = [user32_dll]::FindWindowEx($zero, $zero, $zero, $title)
}

$state = [user32_dll]::GetWindowLong($hwnd, -16)

# mask of 0x20000000 = minimized; 2 = minimize; 4 = restore
if ($state -band 0x20000000) { $action = 4 } else { $action = 2 }

if ([user32_dll]::ShowWindowAsync($hwnd, $action)) {
    write-host "Success" -f green
} else {
    write-host "Fail" -f red
}

Questions:

  1. Neither the get-process cmdlet nor gwmi win32_process nor [diagnostics.process]::getProcessByName() would show me either the window title or the HWND of a window that's minimized to the tray (uTorrent, for example). Is there an API method I missed, or is tasklist.exe the correct hack here?

  2. Is there any situation where FindWindowEx() would not be able to find a window that could be found with FindWindow()? Should I import and use FindWindow() as well?

Suggested improvements and other comments (not including "Why not just write the whole thing in PowerShell?") are welcome, of course!

share|improve this question
up vote 2 down vote accepted

It took a bit to wrap me head around what you have here. It certainly is a piece of work. Rather clever what you have done in order to have a file that runs batch and PowerShell. Bravo sir! It also seems you like doing this with other languages as well!

I will first try to address your questions first and then some other points I feel worth mentioning.

  1. In practice there are of course better options until you put in the minimized exception. In my minimal practice of you code and the suggestions you tried I think, in general, your approach is required. This is the weakest part of the review as I do not have much to offer here.

  2. Is there any situation where FindWindowEx() would not be able to find a window that could be found with FindWindow()? Refering to the documentation for FindWindow() I did find some evidence which could convince you to stick with FindWindowEx()

    Retrieves a handle to the top-level window whose class name and window name match the specified strings. This function does not search child windows. This function does not perform a case-sensitive search.

That might not change anything but depending on the target of the script it might not find the window you are looking for if you use FindWindow().

There is a few things to touch on. Most are just FYI. For the ones that are more than that I will try and draw a little more attention to them.

Invoke-Expression

The hybrid-ness of your script relies on this. I am sure that you do it so that it is self contained. However, in case no one mentioned it: Invoke-Expression can be evil. While I don't take that to heart it is potentially dangerous to use as someone can inject code into that file and it will execute blindly.

If you are willing to have a .ps1 file that gets called it would be safer. That is my tactful way of addressing "Why not just write the whole thing in PowerShell?"

Don't really expect you to adhere to the advice. Just wanted you to be aware.

Comments

This is a big one since you are giving this to someone else. This script is lacking of comments. While I do not code much in batch the PowerShell is lacking and some of the idiosyncrasies of this script were lost on me until I looked up the documentation. While you might know what is going on now you might not months/years from now.

For instance I had to look up to documentation for the line $hwnd = (ps $env:prog)[0].MainWindowHandle. Something simple like 

# MainWindowHandle will return 0 if the process in hidden/minimized.

Again that could just be me but in general this is lacking context comments.

Get-Content

I see that you are reading the file in as one string using a -join. I would think that you have at least PowerShell v3.0. Either way there are cleaner options for reading the file in as one string.

# PowerShell v3.0+
"iex (gc \"%~f0\" -Raw)"
# PowerShell 2.0
"iex (gc \"%~f0\" | Out-String)"

Determining Title

Parsing cmd output is a specialty of mine. From the looks of it you are banking on Window Title being the last line of the output. While I doubt there will ever be a problem I can not say that nothing would go wrong with that logic.

With what little it would take to get more reliable results I would go so far as to change that.

$titles = tasklist /v /fi "imagename eq Keepass*" /fo list | 
        Where-Object{$_ -match "^Window Title:\s+?(.*)"} | 
        ForEach-Object{$Matches[1]}

Notice I also used $titles since you could have multiple matches. The way you had it before would just take the last one. Could do just the same with a Select -First 1

Again, I am impressed with the script you have set up here.

share|improve this answer
    
Thanks for the feedback! Re: Out-String, I like it! Thanks! Re: Invoke-Expression, I have the same hesitation, reminiscent of avoiding eval() in JavaScript and elsewhere. In this case, the only potential harm is the extra thread needed to eval the script I think. Re: $titles, I considered that, and it would certainly be more efficient than performing a regex replace 10 times only to trash the first 9 as happens now. (I might add Select -Last 1.) Regardless, this way should be locale agnostic, whereas your suggestion will require English Windows. Re: comments, you're right, of course. – rojo Jan 19 '16 at 2:56
    
@rojo I had though about ConvertFrom-StringData but you do little with the object after the fact. That and it would still suffer from the language issue. If nothing else Select -Last 1 would address the problem I was brining to light. – Matt Jan 19 '16 at 3:00
    
Interesting! I hadn't seen that cmdlet before. I could avoid the locale issue with $task = tasklist args... | %{ $_ -replace '^([^:]+):', '$1=' } | ConvertFrom-StringData then $title = $task.Values | Select -Last 1. That'd still perform a regex replace on all lines, in addition to being wordier than my current solution. It was a fun exercise, though. :) For what it's worth, I rewrote the script in C#, just to teach myself a little C#. The compiled and linked exe is much faster. I'm trying to rewrite it in Jscript.Net, but not having much luck importing user32.dll yet. I'm not smart. – rojo Jan 19 '16 at 3:33
    
FWIW I don't know much about c# so you are 1 up on me. hopefully powershell is related enough to be of use. – Matt Jan 19 '16 at 3:50

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.