api: provide an error when the output does not exist where to add an tag

[xMAC94x]

similar to the error for move_to_output

[xMAC94x]

It feels kind of weird. The api gets a OutputHandle and we can prob always expect the outputHandle to actually map to an existing Output.
It feels weird that you have to check on that error as user again. Maybe its okay to just tracing::error() or expect in this case.

The same goes for move_to_output when the output is provided via a handle.

WDYT ?

[Ph4ntomas]

The api gets a OutputHandle and we can prob always expect the OutputHandle to actually map to an existing Output.

The OutputHandle could be stale or become stale sometime between calling this function and the server receiving the message. We can't do any assumption on the validity of the data (beside, user input should not be trusted, so let's not assume anything).

It feels weird that you have to check on that error as user again. Maybe its okay to just tracing::error() or expect in this case.

IMO the API should never hide or consume errors, unless the "error" is an expected case and a documented fallback behavior exists (and even then the calling code should be notified).

[xMAC94x]

in that case I think the PR is good to go

[xMAC94x]

@Ph4ntomas @Ottatop PR is ready for review

[Ottatop]

After giving this some thought, my main concern is that returning an error introduces a bit more friction when using this API. Yes, if we really wanted to be "correct" here we would add this. However, there are also a slew of methods on the other *Handles that also silently fail when the associated object is gone, and making those also return errors would kinda suck tbh. I'm not sure where to draw the line.

With move_to_output the error returns useful information that can't be deduced from just the input and output of the function. In contrast, we could already deduce an error when adding tags if the input array is non-empty and the output is, and the output being gone is the only reason.

I'm not saying returning a result here is useless, but I'm not convinced that the upside is large enough to warrant the change. For example, yes, Mutex poisoning is good information, but also most people don't care and unwrap the lock anyway. I can envision a similar situation if this change is implemented.

[Ph4ntomas]

After giving this some thought, my main concern is that returning an error introduces a bit more friction when using this API. Yes, if we really wanted to be "correct" here we would add this. However, there are also a slew of methods on the other *Handles that also silently fail when the associated object is gone, and making those also return errors would kinda suck tbh. I'm not sure where to draw the line.

Just my 2c here. Most of the time, errors will end up in the log file by the server. This is more that okay for now, but IMO it's confusing for user not to have any kind of (visual) feedback. OTOH keeping the API simple is nice.

My proposition would be:

• We transmit errors to the client/config via protobuf.
• The current API is kept as-is¹.
• We add try* method if users want to perform error checking themselves.

¹ At some point, we could log errors/warning coming from the server in non-failible function by calling tracing::* method. This would allow user to add a sink that would transform the errors in some kind of notification and display. The fallback sink could just be a println as we currently have.

[Ottatop]

• We add try* method if users want to perform error checking themselves.

I considered this, but my concern here is that we have to expose a second fallible function for everything that can fail, which is... a lot of things. Well, we don't have to, but again, where do we draw the line? Also, seeing as this doubles the function count and how there are already functions duplicated for async-ness on the Rust side, that would be 4 slightly different functions for the same thing. Also also, same thing with just making the function return an error, how useful is it going to be? Turning to this PR, realistically what is someone going to do if they encounter an OutputDoesNotExist error that they wouldn't have otherwise done if they get an empty array with the current add? Yes, it's more explicit, but I feel like encoding that specific "does not exist" error into everything would be better served by people just connecting to the corresponding destroy/closed signal and doing handling there, similar to how Wayland works with destroyed/closed events.

Looking at prior arts, Jay has a Rust config and doesn't appear to return a lot of errors. Actually that API seems to go in the complete opposite direction and encodes null/invalid states within objects to make a lot of functions infallible. Awesome's tag.add also appears to be infallible. Not actually sure if that just returns nil if the screen doesn't exist or what.

¹ At some point, we could log errors/warning coming from the server in non-failible function by calling tracing::* method. This would allow user to add a sink that would transform the errors in some kind of notification and display. The fallback sink could just be a println as we currently have.

This is an interesting idea. While I feel exposing all the errors has ergonomic issues, better visibility of the errors that do occur is a good middle ground. But obviously that's out-of-scope here and needs further discussion.

[xMAC94x]

I can totally understand all of those concerns. I am not sure which approach is the right one tbh. However I want that error handling is kinda similar across the API.

I you want me, I can adjust this MR to be

We transmit errors to the client/config via protobuf.
The current API is kept as-is.

[Ottatop]

Let's hold off on that until we figure out how we're actually going to use the errors, no point in implementing that now if plans change later