sciter-sdk / rust-sciter

Rust bindings for Sciter
https://sciter.com
MIT License
805 stars 75 forks source link

Can't set `UxTheming(true)` #62

Closed DevJac closed 4 years ago

DevJac commented 5 years ago

This line in my code panics: sciter::set_options(sciter::RuntimeOptions::UxTheming(true)).unwrap(); It panics with the following message: thread 'main' panicked at 'calledResult::unwrap()on anErrvalue: ()', src/libcore/result.rs:999:5

I see the following code is related to this error:

/// Set various sciter engine global options, see the [`RuntimeOptions`](enum.RuntimeOptions.html).
pub fn set_options(options: RuntimeOptions) -> std::result::Result<(), ()> {
    use RuntimeOptions::*;
    use capi::scdef::SCITER_RT_OPTIONS::*;
    let (option, value) = match options {
        ConnectionTimeout(ms) => (SCITER_CONNECTION_TIMEOUT, ms as usize),
        OnHttpsError(behavior) => (SCITER_HTTPS_ERROR, behavior as usize),
        // GpuBlacklist(json) => (SCITER_SET_GPU_BLACKLIST, json.as_bytes().as_ptr() as usize),
        InitScript(script) => (SCITER_SET_INIT_SCRIPT, script.as_bytes().as_ptr() as usize),
        ScriptFeatures(mask) => (SCITER_SET_SCRIPT_RUNTIME_FEATURES, mask as usize),
        GfxLayer(backend) => (SCITER_SET_GFX_LAYER, backend as usize),
        DebugMode(enable) => (SCITER_SET_DEBUG_MODE, enable as usize),
        UxTheming(enable) => (SCITER_SET_UX_THEMING, enable as usize),
    LibraryPath(path) => {
      return set_library(path).map_err(|_|());
    }
    };
    let ok = (_API.SciterSetOption)(std::ptr::null_mut(), option, value);
    if ok != 0 {
        Ok(())
    } else {
        Err(())
    }
}

The value of ok ends up being 0, and so the Err(()) is returned. I'm not sure where to look for documentation on the Sciter C++ API, but the set option call is returning 0, what does that mean?

The error occurs on Linux (GTK / Wayland), but not on Windows.

pravic commented 5 years ago

https://github.com/c-smile/sciter-sdk/blob/3b9818518335bcd0d1f616c412f2cca551414c00/include/sciter-x-def.h#L444

It's BOOL, so 0 is an error. Why? No idea. I think, it's SDK related.

pravic commented 5 years ago

I think, it's better to ask on https://sciter.com/forums/ if you haven't done that.

Edit: it seems, you have :)

DevJac commented 5 years ago

I asked on the Sciter forum and Andrew said:

[UX Theming] is applicable only for Windows version where by default Sciter uses system theme that delegates inputs rendering to system. On other platforms that call does nothing and returns FALSE – no changes were made – MacOS and Linux use “unisex” theme by default as there are no reasonable ways there to ask system to draw control shapes.

See: https://sciter.com/forums/topic/should-sciter-look-the-same-on-windows-and-linux-and-other-platforms/

It looks like SciterSetOption returns false when a setting is not applicable and has no effect. These bindings represent the false value by returning Err. In C++ it looks like the return value of SciterSetOption is often ignored, which is fine I think. Returning Err when a setting is not applicable may be too strong, even though it plays nice with the Rust type system.

For now, I have solved this by doing the following:

let _ignore_err = sciter::set_options(sciter::RuntimeOptions::UxTheming(true));

Maybe set_option should return a bool? It's nice to deal with Result in Rust, and you can make it work easily like I did above, but it feels wrong to ignore an Err so I'm not sure what the best API would be. Just a thought.

If you want to make the judgement call on whether set_option should return a bool or remain the same, I'd be happy to submit a PR with any changes. At least I could document that it's OK to ignore the Err in most cases.

IMO, this issue can be closed.

pravic commented 5 years ago

I see. Perhaps, it must have been better to return bool here. But now it will be a breaking change.

I can skip this call on non-windows platforms, it will fix the issue. And also I need to add that quote to the documentation.

c-smile commented 5 years ago

I am making changes on my side so ::SciterSetOption(NULL,SCITER_SET_UX_THEMING,TRUE); will always return true.

DevJac commented 4 years ago

Looks like the change was made upstream, as @c-smile said. I can now do the following ...

sciter::set_options(sciter::RuntimeOptions::UxTheming(true)).unwrap();

... like I originally wanted; no more problems.

This issue can be closed.

pravic commented 4 years ago

Great. You are the reporter, you close it)