[HELP] `glib::LogLevels` vs `glib::LogLevelFlags` vs `glib::LogLevel`

[arcnmx]

The relationship between these 3 types isn't very clear from the documentation, and conversion traits between them don't seem to exist as one might expect (plus iterator conversions may also be useful?). The LogLevelFlags docs even mention log_set_handler and log_set_fatal_mask, which confusingly both take a LogLevels flag instead! Is there a reason why these two types haven't been merged, does the presence of the internal flags complicate it?

[sdroege]

Yes the API is a bit confused here. glib::logLevel is the one that should be public (and needs some more trait impls), the others are redundant. log_set_fatal_mask() should take a slice of these or similar.

Do you want to provide a PR that gets rid of the other two and implements log_set_fatal_mask() like that?

[arcnmx]

Short of pulling in something like enumflags2 for a BitFlags<LogLevel> type, I do think there's still value in keeping one of LogLevels or LogLevelFlags for glib::translate::* purposes. Vec<LogLevel> just feels a bit heavy? Though it certainly would be nice to impl FromIterator and friends to make it easier to use; a signature like log_set_fatal_mask<I: IntoIterator<Item=LogLevel>>(levels: I) could then be used to accept both*

Do you want to provide a PR that gets rid of the other two and implements log_set_fatal_mask() like that?

Sure! I mostly just wanted to report things I ran into using glib over the past week or two for discussion purposes before going ahead with actually making the changes.

* in 2021 only iirc, and with an array rather than slice, but... close enough?

[sdroege]

glib::LogLevel (the enum) should implement FromGlib and IntoGlib for FFI purposes.

And AFAICS the only place where it is actually used as flags is in log_set_fatal_mask(), and there working with a &[glib::LogLevel] or impl AsRef<[glib::LogLevel]>. Do you see a problem with that?

Though it certainly would be nice to impl FromIterator and friends to make it easier to use; a signature like log_set_fatal_mask<I: IntoIterator<Item=LogLevel>>(levels: I) could then be used to accept both*

IMHO that's overkill. How often are you going to call this function in your application and why wouldn't it be with some kind of slice/array? :)

[arcnmx]

glib::LogLevel (the enum) should implement FromGlib and IntoGlib for FFI purposes.

Sure, though FromGlib would be fallible and thus wouldn't be a good type for gir to choose for autogenerated bindings, for example. As an argument though, it usually is what you want!

And AFAICS the only place where it is actually used as flags is in log_set_fatal_mask(), and there working with a &[glib::LogLevel] or impl AsRef<[glib::LogLevel]>. Do you see a problem with that?

I just feel like it's worth keeping the signature explicit, and letting the argument type reflect that. impl Into<LogLevels> would be pretty clear and sane, no? I just generally think it's good practice to say what you mean, and rely on conversion traits to do the heavy lifting for you rather than encoding the conversion directly into the point of use. Sure it is a single function that barely anyone uses, so really doesn't matter - but it's somewhat worth being consistent for the sake of other bindings that may need the full type.

Though it certainly would be nice to impl FromIterator and friends to make it easier to use; a signature like log_set_fatal_mask<I: IntoIterator<Item=LogLevel>>(levels: I) could then be used to accept both*

IMHO that's overkill. How often are you going to call this function in your application and why wouldn't it be with some kind of slice/array? :)

Fair enough, for what it's worth I found these types by working on bindings that take it as an argument - for an API that really only needs a single LogLevel as an input argument. So I don't have any personal stake here and don't really have strong opinions.

(side-note: gir refused to bind some of them due to [ERROR libgir::analysis::record] Missing memory management functions for GLib.LogField - I wonder if there should be a tracking issue for "safe" wrappers of those types?)

[sdroege]

Sure, though FromGlib would be fallible and thus wouldn't be a good type for gir to choose for autogenerated bindings, for example. As an argument though, it usually is what you want!

Something to worry about for someone who actually exposes these types in their API IMHO. I don't think they're used anywhere outside GLib...

I just feel like it's worth keeping the signature explicit, and letting the argument type reflect that. impl Into<LogLevels> would be pretty clear and sane, no? I just generally think it's good practice to say what you mean, and rely on conversion traits to do the heavy lifting for you rather than encoding the conversion directly into the point of use. Sure it is a single function that barely anyone uses, so really doesn't matter - but it's somewhat worth being consistent for the sake of other bindings that may need the full type.

Having all this in the API would lead to questions and confusion probably, especially if it's just for that single function.

(side-note: gir refused to bind some of them due to [ERROR libgir::analysis::record] Missing memory management functions for GLib.LogField - I wonder if there should be a tracking issue for "safe" wrappers of those types?)

If you want to work on that, sure :)

[arcnmx]

Looking into it more, log_set_handler handles this awkwardly by turning each flag into an explicit argument. The whole boolean arguments considered harmful for hindering readability thing aside, it also then never passes those flags back along to the handler via the trampoline (which I believe are relevant?).

Having all this in the API would lead to questions and confusion probably, especially if it's just for that single function.

I do feel like have an Into<X> gives you a simple enough link to documentation about its ability to be converted from slices and whatnot, which could even be directly shown in an example. Another approach might be to impl ops::Or<LogLevel> for LogLevel and LogLevels, making LogLevel::Critical | LogLevel::Error the canonical way to combine them instead of using slices/iters. This also enables things like LogLevel::Critical | LogLevelFlags::FLAG_FATAL for log_set_handler, which isn't necessarily intuitive to write but it is more descriptive than log_set_handler(domain, LogLevel::Critical, true, false, callback)

I feel like a bigger problem here is the context switching and subtle differences in meaning/requirements. For some APIs, you do only want a single level. For others, you want a list/bitset/whatever of levels, but perhaps not the flags. For yet others you want both, or perhaps even to split them up into level (single or multiple?), and then just the flags.

I'm now on the fence regarding whether to just keep all 3 and make sure conversions just work and compose in the ways they need to... Documentation can serve the role of differentiating them and describe what each variant is for. My initial confusion stemmed from the fact that none of them were documented, which made it look like it may have been an oversight/mistake that all 3 existed. There do seem to be legitimate arguments for all 3 to exist, really. I could instead argue for just the two (LogLevel and LogLevelFlags), with the main downside that the compiler wouldn't stop you from passing along flags when it's pointless to do so... I'm not really sure where to fall on this?

[arcnmx]

And a completely new angle that also throws a wrench into the works: G_LOG_LEVEL_USER_SHIFT permits libraries to use the unallocated bits in the log flags for their own levels. The library I'm trying to bind indeed uses this, and LogLevelFlags::LEVEL_MASK is the only reason they don't currently get thrown out (bitflags! would otherwise truncate them if not assigned to at least one const). glib::LogLevels would currently truncate them, which is kind of bad. Likewise for glib::LogLevel, which has no Unknown(u32) fallback for them to fall into - which triggers a panic when trying to decode them!

[sdroege]

it also then never passes those flags back along to the handler via the trampoline (which I believe are relevant?).

How do you mean?

I'm now on the fence regarding whether to just keep all 3

Why 3? You only need the enum and one of the flags types or not?

Looking into it more, log_set_handler handles this awkwardly by turning each flag into an explicit argument.

If you were passing the bitflags it makes matching on the level a bit awkward: is it valid to have critical and error set at the same time or can only one be set at a time? Really these things should've been one enum and one bitflags at the C level, not this strange combination that they are now.

I'm fine with keeping those two and having conversions from the enum to the flags and to a slice of enum values and then making use of the flags in that single function.

And a completely new angle that also throws a wrench into the works: G_LOG_LEVEL_USER_SHIFT permits libraries to use the unallocated bits in the log flags for their own levels.

Yeah that's annoying and was left as a problem to solve once someone needs it. Which seems to be the case now :) Do you have an API suggestion for this?

[arcnmx]

it also then never passes those flags back along to the handler via the trampoline (which I believe are relevant?).

How do you mean?

Well, the inputs to the function are both a set of log levels and the fatal/recursive flags (indicating support for messages that match those conditions). The callback is then called with (presumably) a single log level, and the relevant flags (but not the same ones you passed to the function). For example, if you register a function with LogLevelFlags::all() to catch all levels and flags, the callback will presumably be called with LEVEL_ERROR | FLAG_FATAL as an example, but LEVEL_WARNING would not be associated with any flags. I'm basically just saying that the C callback is passed a level+flags, but the trampoline strips them away and only passes a single LogLevel?

I'm now on the fence regarding whether to just keep all 3

Why 3? You only need the enum and one of the flags types or not?

This is just referring to what I said above, how each function may require a mix-and-match of LogLevel | LogLevels | FlagsOnly | LogLevelsPlusFlags. LogLevels can still serve a purpose when the flags are irrelevant, but it's not strictly necessary. One could even imagine a solution where impl FromGlib<ffi::GLogLevelFlags> for (LogLevels, LogFlags) makes sense!

Looking into it more, log_set_handler handles this awkwardly by turning each flag into an explicit argument.

If you were passing the bitflags it makes matching on the level a bit awkward: is it valid to have critical and error set at the same time or can only one be set at a time? Really these things should've been one enum and one bitflags at the C level, not this strange combination that they are now.

Maybe this is an argument for exposing the flags as their own separate bitflags in rust! They can always be merged with levels at FFI boundaries via translate traits. Though to be fair, there's nothing particularly wrong with passing both LogLevel, LogLevelFlags in these cases. Particularly in the case of the callback (where the values are constructed by glib, not the user/caller), it just means there's a tiny bit of redundancy in that the level is contained in both values.

And a completely new angle that also throws a wrench into the works: G_LOG_LEVEL_USER_SHIFT permits libraries to use the unallocated bits in the log flags for their own levels.

Yeah that's annoying and was left as a problem to solve once someone needs it. Which seems to be the case now :) Do you have an API suggestion for this?

Do you have any opinions regarding the following two approaches?

/// use a single variant to capture all user levels
enum LogLevel1 {
  Info, Error, Etc,
  User { bitshift: u8, }, // or User(u8), whatever
}

/// use individual variants per level
enum LogLevel2 {
  Info, Error, Etc,
  User8,
  User9,
  User10,
  // ...
  User31,
}

(or some mix of the two like having a separate LogLevelUser enum. LogLevel1 is more streamlined but allows invalid levels like User(33) to be constructed by the user. If you really cared, you could make a LogLevelUser(u8) newtype with constructors that ensure invalid levels never exist)

[sdroege]

but the trampoline strips them away and only passes a single LogLevel?

Good point but IIRC the C code also only passes a log level to the function or not?

Maybe this is an argument for exposing the flags as their own separate bitflags in rust!

Sure, why not? :) I think that wasn't done in the past because of the general sentiment that "bitflags are not idiomatic Rust".

I think it should be a level enum plus a flags for the flags though, not something combined as that allows to represent impossible values.

Do you have any opinions regarding the following two approaches?

I prefer the second.

[arcnmx]

but the trampoline strips them away and only passes a single LogLevel?

Good point but IIRC the C code also only passes a log level to the function or not?

AFAICT the docs say that it's the log level including flags.

Maybe this is an argument for exposing the flags as their own separate bitflags in rust!

Sure, why not? :) I think that wasn't done in the past because of the general sentiment that "bitflags are not idiomatic Rust".

I think it should be a level enum plus a flags for the flags though, not something combined as that allows to represent impossible values.

Yeah, that would be the advantage of splitting the flags off into their own type - being able to mix them with either a single level or multiple where it's appropriate to do so!

[sdroege]

AFAICT the docs say that it's the log level including flags.

Ah I see. Well, then let's go with passing both to it :) I still think it would be useful and less confusing to keep log level (as enum) and flags in separate parameters though as that makes it clear that only one level is active at a time.

[sdroege]

@arcnmx How should we proceed here?

[arcnmx]

Sorry I've been pretty busy recently and haven't had a chance to put this together. We have a working plan here, I can try to work on it later this week.