Possible ShellExecuteEx implementation

[binChris]

Analysis of the original Windows API

https://learn.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-shellexecuteexw https://learn.microsoft.com/en-us/windows/win32/api/shellapi/ns-shellapi-shellexecuteinfow

typedef struct SHELLEXECUTEINFOW {
// size of this struct, to be handled internally
DWORD     cbSize;
// in; combination of different flags, see MS docs
ULONG     fMask;
// opt in; owner hwnd
HWND      hwnd;
// opt in
// if not set, the default operation is used (not necessarily "open")
LPCWSTR   lpVerb;
// in; non-opt unless SEE_MASK_INVOKEIDLIST is used
LPCWSTR   lpFile;
// opt in
LPCWSTR   lpParameters;
// opt in
LPCWSTR   lpDirectory;
// in, co::SW
int       nShow;
// opt out, SE_ERR or HINSTANCE (when > 32)
HINSTANCE hInstApp;
// opt in
// pointer to ITEMIDLIST (SEE_MASK_IDLIST or SEE_MASK_INVOKEIDLIST set)
// https://learn.microsoft.com/en-us/windows/win32/api/shtypes/ns-shtypes-itemidlist
void      *lpIDList;
// opt in if SEE_MASK_CLASSNAME set
LPCWSTR   lpClass;
// opt in if SEE_MASK_CLASSKEY set
HKEY      hkeyClass;
// opt in if SEE_MASK_HOTKEY set
DWORD     dwHotKey;
union {
  // opt in if SEE_MASK_ICON set; ignored as of Windows Vista; drop from winsafe API?
  HANDLE hIcon;
  // opt in if SEE_MASK_HMONITOR set; handle to monitor
  HANDLE hMonitor;
} DUMMYUNIONNAME;
// opt out, handle to newly started application
HANDLE    hProcess;
}

The fields 'hwnd', 'lpVerb', 'lpFile', 'lpParameters', 'lpDirectory' and 'nShow' are the same as on the ShellExecuteW function.

Thoughts on the API design

Executing ShellExecute in winsafe does not allow hwnd to be zero, but I do not see that as a problem (using GetDesktopWindow). The hwnd parameter can be set to NULL using w::HWND::NULL.ShellExecute(...). ShellExecuteEx can be implemented next to ShellExecute in the same trait.

Output interface

What bothers me is most is the mix of input and output fields in the Windows API implementation. Struct field count by direction, cbSize and hwnd not counted:

• in: 3
• optional in: 8
• out: 0 (but function returns BOOL; call GetLastError when false)
• optional out: 2

Output fields should be returned by the winsafe wrapper instead of being part of the input interface.

Unfortunately, hInstApp is optional and either a HINSTANCE or SE_ERR.

The documentation says

The SE_ERR_XXX error values are provided for compatibility with ShellExecute.
To retrieve more accurate error information, use GetLastError.

Therefore returning an error based on GetLastError and otherwise ignoring an optional SE_ERR could work. If Windows returns hInstApp < 32, Option<HINSTANCE> would be None.

fn ShellExecuteEx(...) -> SysResult<(Option<HINSTANCE>, Option<HANDLE>)>

Input interface

In the interest of staying close to the original Windows API, I would propose to use a struct stripped of the output fields. This has the added benefit that fields can be defaulted. The majority of fields is optional anyways and most of them are rarely used.

All optional input fields would be an Option<T>.

Automatic fMask flags

Some optional input fields require an fMask flag to be set in order to not be ignored. For example, lpClass requires the SEE_MASK_CLASSNAME flag. These flags could be added automatically if the parameter value is not None for improved ergonomics.

lpVerb

lpVerb is optional in the Windows API, but not optional in winsafe's implementation of ShellExecute. I believe it's an edge case to execute the default verb, which most of the time is "open" anyways. But using the default verb is currently not covered by the winsafe API, unless falling back to the unsafe implementation. It needs to be decided whether to make lpVerb optional or not.

hIcon

As described above, hIcon is obsolete since Vista. I am not sure which Windows versions you aim to support, but I would recommend to drop it from the interface. So instead of the union, it would just be an Option<HANDLE hMonitor>. Otherwise an enum could work to hold either hIcon or hMonitor.

(Why these 2 are mutually exclusive in the Windows API is beyond my understanding.)

[rodrigocfd]

Hi Chris,

I'm gonna carefully read your observations, but right away I can answer these:

Executing ShellExecute in winsafe does not allow hwnd to be zero

It does, just use the NULL associated constant, which all handles have:

w::HWND::NULL.ShellExecute(...)

I would propose to use a struct stripped of the output fields.

This is exactly what I am thinking. This case is very similar to TaskDialogIndirect, which I've been sketching a higher-level API. TaskDialogIndirect is even worse though, because the struct has a different memory alignment and has even more parameters, including 2 unions. I took me two weeks until I got it properly working. A nightmare. And I still don't like the results – that's why I'm sketching a new API.

I am not sure which Windows versions you aim to support

As many as possible, including older ones.

I'll reply back as soon as I can, thank you for your time designing this.

[binChris]

Hi Rodrigo,

It does, just use the NULL associated constant [...]

I stand corrected 😊 Updated above.

As many [Windows versions] as possible, including older ones.

For the hIcon param I would still tend to exclude it from the winsafe interface. My feeling is, its use is almost non-existent. We could scan public repos to get hard evidence. Including it would require an enum to hold either hMonitor (which has a legitimate use case) or hIcon, which would make the interface slightly more awkward.

A nightmare.

It's obvious in almost every Windows API call that they designed it when API design was not yet common knowledge. One wonders though why they never created a v2 API...

That's why winsafe brings so much added value to the table.

[rodrigocfd]

I'm still doing a lot of tinkering here, but so far I could come up with this signature for the function:

fn ShellExecuteEx_TEST(
    info: &SHELLEXECUTEINFO_TEST,
) -> SysResult<(HMONITOR, Option<HPROCESS>, Option<co::SE_ERR>)>;

And having this input struct:

#[derive(Default)]
struct SHELLEXECUTEINFO_TEST<'a, 'b> {
    pub hwnd: Option<&'a HWND>,
    pub verb: Option<String>,
    pub file: String,
    pub parameters: Vec<String>,
    pub directory: Option<String>,
    pub show: co::SW,
    // pub id_list: ???
    pub class: Option<String>,
    pub hkey_class: Option<&'b HKEY>,
    pub hot_key: Option<(co::VK, co::HOTKEYF)>,
}

Usage would be something like:

ShellExecuteEx_TEST(
    &SHELLEXECUTEINFO_TEST {
        file: "abcdef".to_owned(),
        show: co::SW::SHOW,
        ..Default::default()
    },
)?;

Or, handling the results:

match ShellExecuteEx_TEST(
    &SHELLEXECUTEINFO_TEST {
        file: "abcdef".to_owned(),
        show: co::SW::SHOW,
        ..Default::default()
    },
) {
    Ok((hmonitor, hprocess, se_err)) => todo!(),
    Err(e) => todo!(),
}

Following your guidelines, fMask will be automatic.

hIcon, indeed, is totally bonkers. It's so badly designed that I agree to remove it from the API.

lpIDList is giving me a hard time, I'm still thinking about it.

Internally, this high-level struct will be serialized into the C-compatible one, and then passed to the function.

What do you think of this design?

[binChris]

It's great to see progress =).

Sorry for the long response, but this interface is complex.

hwnd

Shouldn't the method get implemented on the shell_Hwnd: ole_Hwnd trait, just as ShellExecute? In that case hwnd should be removed from params.

fmask

fMask unfortunately cannot be fully automatic. Some of the flags passed in cannot be derived from other params being present, like ASYNCOK or WAITFORINPUTIDLE, so it needs to be a parameter. What I was proposing was to add the params related flags automatically, if the related param is Some(...). Would need proper documentation though, as it would be a bit "magic behind the scenes" (useful though).

parameters

parameters should be a plain String as it is on ShellExecute instead of a vector.

hmonitor

hMonitor, even if part of that silly union, has a valid use case and should not be missing, ergo be added to the struct as Option<>.

lpIDList

I'm a bit lost on this param too. If I understand correctly, the convoluted Windows data structures used for it hand over a byte array, see SHITEMID, BYTE[1] A variable-length item identifier. Testament to the age of this interface...

I have never used this param myself or seen it used, so I guess the options are:

• do not include it in winsafe for now until someone comes along who needs it and has a better understanding on how to use it
• implement it as Option<Vec<byte>>, figure out how to actually use it and test it out

Function signature

se_err

I assume that if this return value is set, the function call did not succeed. As pointed out above, MS docs recommend to call GetLastError(), which if winsafe would do that as part of the implementation, would return an Err. That makes it a bit weird to have se_err in the Ok(...) part.

If it stays there and winsafe does not do any internal handling of a possible se_err error if would mean the fn returns Ok but failed, which would make the usage of the fn awkward (but only as awkward as the original Windows API, haha).

Again citing MS docs:

The SE_ERR_XXX error values are provided for compatibility with ShellExecute. To retrieve more accurate error information, use GetLastError.

I would assume that Windows also consistently returns false in that error case, which would trigger the GetLastError() call in winsafe, returning an Err().

Therefore I would vote to remove se_err from the return values.

Align with ShellExecute?

Looking at the examples you provided, I began to think if it would be beneficial to put the params existing on ShellExecute into the fn signature instead of the struct, deviating from the original Windows API.

This would mean something like:

[deprecated("experimental; signature might be changed in the future")]
fn ShellExecuteEx(
  fmask: ...,
  file: &str,
  verb: Option<&str>,
  parameters: Option<&str>,
  directory: Option<&str>,
  show_cmd: co::SW,
  info: &SHELLEXECUTEINFO, // rest of the args
) -> SysResult<(HMONITOR, Option<HPROCESS>)>;

N.b. Option<co::SE_ERR> removed from return values as per above argument.

It's a question of taste / philosophy I guess. If you decide for this option, it probably makes sense to put fmask into the signature as well, as it is always used. info might even be optional as only a few params remain, all being optional themselves.

If you strive for improved ergonomics, take this into consideration; if you want to stick as closely as possible to Windows API, stay with struct only.

Usage

Usage would look like

ShellExecuteEx(
  SEE_MASK_NOCLOSEPROCESS | SEE_MASK_NOASYNC,
  "calc.exe",
  "open".into(),
  None,
  None,
  co::SW::SHOW,
  Some(&SHELLEXECUTEINFO {
    hMonitor,
    ..Default::default()
  }),
)?;

[rodrigocfd]

Shouldn't the method get implemented on the shell_Hwnd: ole_Hwnd trait, just as ShellExecute? In that case hwnd should be removed from params.

It could... but keeping it in the struct makes it a bit closer to the original API.

Some of the flags passed in cannot be derived from other params being present, like ASYNCOK or WAITFORINPUTIDLE, so it needs to be a parameter.

Indeed, you're right. Some flags will be pub and available to be set, and others will be set automatically and be pub(crate). This will nicely limit the available flags.

parameters should be a plain String as it is on ShellExecute instead of a vector.

Done.

hMonitor, even if part of that silly union, has a valid use case and should not be missing, ergo be added to the struct as Option<>.

I guess I misunderstood the docs, I thought hMonitor and hIcon were output parameters... so they're actually input ones? In this case, we can take an enum with either one.

lpIDList

I've found a few structs like this one. I even wrote a specific marker trait for them: VariableSized. I believe the Option<Vec<u8>> is the best approach. If someone ever comes with an actual use case, we'll revise it.

I assume that if this return value is set, the function call did not succeed. As pointed out above, MS docs recommend to call GetLastError(), which if winsafe would do that as part of the implementation, would return an Err. That makes it a bit weird to have se_err in the Ok(...) part.

Therefore I would vote to remove se_err from the return values.

You're right, I forgot the paragraph in the docs which points out that SE_ERR was kept just for compatibility. I agree to remove SE_ERR and just use GetLastError.

Looking at the examples you provided, I began to think if it would be beneficial to put the params existing on ShellExecute into the fn signature instead of the struct, deviating from the original Windows API.

It's a question of taste / philosophy I guess.

The problem with changing the original API is that we make it a little harder to C/C++ devs to grasp the abstraction. One of the primary goals of WinSafe is to be familiar to C/C++ Win32 devs (like myself), so the closer the better. Some things are obviously too cumbersome to be kept (like *mut string parameters, for example), but others are bearable. But yeah, taste/philosophy.

So, this is my current implementation. Does it look good?

[binChris]

Looking great to me, I'll give it a spin!

[rodrigocfd]

Hi Chris,

I made a minor change in SHELLEXECUTEINFO. Now, instead of storing string objects, it will store references, so its construction becomes a little less verbose, without the to_owned() suffixes.

Before:

SHELLEXECUTEINFO {
    file: "C:\\Temp\\foo.exe".to_owned(),
    ..Default::default()
}

Now:

SHELLEXECUTEINFO {
    file: "C:\\Temp\\foo.exe",
    ..Default::default()
}

I didn't see any side effects so far, so let me know if this works.

[binChris]

Hi Rodrigo,

That should work as well for cases the struct is used in conjunction with the function call. Makes it a bit harder to work with longer lived struct instances, but for that case the user could construct his/her own struct and wrap the function call.