gio: Return glib::ExitCode from gio::Application signal handlers

[pjungkamp]

Add a manual override to use glib::ExitCode instead of the gir generated i32 return value for connect_command_line and connect_handle_local_options.

This fixes #1691

[sdroege]

See #1592

Also CC @A6GibKm

[pjungkamp]

Could we add another associated constant to glib::ExitCode, e.g. glib::ExitCode::CONTINUE? I dont feel comfortable with an enum or e.g. Option because Some(glib::ExitCode::from(-1)) and None would be two representations for the effectively the same value.

[pjungkamp]

I just checked the documentation for the return value of the handle-local-options signal.

An exit code. If you have handled your options and want to exit the process, return a non-negative option, 0 for success, and a positive value for failure. To continue, return -1 to let the default option processing continue.

It seems like the definition of glib::ExitCode should be:

• positive means failure
• zero means success
• negative means special value

This means that I'm really not comfortable with the implementation of Termination for glib::ExitCode. Any value that does not fit into u8 shoudl probably be mapped to e.g. u8::MAX.

I've pushed an additional commit that adds a glib::ExitCode::CONTINUE associated constant and some initial documentation for the glib::ExitCode struct.

[A6GibKm]

It seems like the definition of glib::ExitCode should be:

That proposal only makes sense if the negative values have a meaning whenever they are used. As far as I have seen it is only a handful of fns that do something special with negative values. Please correct me if I am wrong.

[sdroege]

Like @A6GibKm says, the main problem here is that handle_local_options() is special. It should probably have a different type than the other cases.

[pjungkamp]

I wouldn't like to have multiple representations for the same handling. If we'd use Option<glib::ExitCode> there shouldn't be a way to say Some(glib::ExitCode::from(-1)) and None which would lead to the same effect.

We could constrain the glib::ExitCode type though.

• Remove impl From<i32> for glib::ExitCode
• Add impl From<u8> for glib::ExitCode instead
• Add impl TryFrom<i32> for glib::ExitCode
• Add struct InvalidExitCode(i32) as the TryFrom::Error type

We could then modify interfaces that currently return a glib::ExitCode which may originate from foreign code, e.g. gio::ApplicationExtManual::run to return a Result<glib::ExitCode, glib::InvalidExitCode> instead. While we'd enforce good exit codes from Rust code through the traits and conversion methods. Using Option<glib::ExitCode> or std::ops::ControlFlow<glib::ExitCode> would then be fine for handle-local-options.

[sdroege]

I think this implementation makes sense. @A6GibKm what do you think?

[pjungkamp]

I've now changed the handle-local-options signal and local-command-line virtual function to use std::ops::ControlFlow<ExitCode>. ExitCode now contains a u8 and only implements TryFrom<i32> instead of From<i32>.

One benefit of std::ops::ControlFlow<ExitCode> over Option<ExitCode> is the handling with the ? operator.

This required me to bump the MSRV to 1.83.

Using Option<ExitCode>

impl ApplicationExt for MyApplication {
    fn local_command_line(&self, arguments: &mut ArgumentList) -> Option<ExitCode> {
        // The `?` operator won't work here since it returns on `None` 
        if let Some(exit_code) = self.parent_local_command_line(arguments) {
            return Some(exit_code);
        }
        
        // Handle my own options
        
        None // Continue with default handling
    }
}

Using ControlFlow<ExitCode>

impl ApplicationExt for MyApplication {
    fn local_command_line(&self, arguments: &mut ArgumentList) -> ControlFlow<ExitCode> {
        // This will return if the parent class has handled the entire command line. 
        self.parent_local_command_line(arguments)?;
        
        // Handle my own options
        
        ControlFlow::Continue(()) // Continue with default handling
    }
}

[sdroege]

Let's get this in then

[swsnr]

@pjungkamp @sdroege While upgrading I came across this change and noticed that while this PR changes some parts of the command line API to use ExitCode, ApplicationCommandline::set_exit_code still takes an i32 instead of an ExitCode.

Shouldn't it now take ExitCode as well? (Though, arguably, that'd be a breaking change now).

[sdroege]

@pjungkamp Maybe we can get around that by changing this to impl TryInto<ExitCode> or so and map errors to ExitCode::FAILURE? But yes, that should've been changed too. Do you want to provide a PR?

Alternatively could add a set_exit_code_what_do_we_name_it() :)

[swsnr]

@sdroege Wrt to a PR, are you asking me or @pjungkamp? I can make a PR, but I'd not bother much with backwards compatibility, and just change the API right away. Then you can either release a quick 0.21.1 with this change; even though it's technically breaking, I doubt it has much practical impact, since it's a bit of a niche API, I guess. Or you just save the change for 0.22; effectively it only saves one .into() call anyway 🙂

[pjungkamp]

I agree that the gio::ApplicationCommandlLineExt::{exit_status,set_exit_status} APIs are a good fit for the glib::ExitCode type since it's used for process termination.

The Rust standard library uses the ExitStatus type for values from wait and the ExitCode type for process termination.

We could follow this naming convention in std (Status vs. Code) and introduce gio::ApplicationCommandLineExt::{set_exit_code,exit_code} methods and mark the exit_status ones as deprecated and remove them with the next minor version bump. Other APIs like glib::spawn_check_wait_status and gio::Subprocess::exit_status would already match this convention.

[sdroege]

That sounds good to me. Do you want to provide a PR @pjungkamp ?