Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions api/lua/pinnacle/grpc/defs.lua
Original file line number Diff line number Diff line change
Expand Up @@ -1334,6 +1334,10 @@ local pinnacle_v1_Backend = {

---@class pinnacle.tag.v1.AddResponse
---@field tag_ids integer[]?
---@field error pinnacle.tag.v1.AddResponse.Error?

---@class pinnacle.tag.v1.AddResponse.Error
---@field output_does_not_exist google.protobuf.Empty?

---@class pinnacle.tag.v1.RemoveRequest
---@field tag_ids integer[]?
Expand Down Expand Up @@ -1613,6 +1617,7 @@ pinnacle.tag.v1.GetRequest = {}
pinnacle.tag.v1.GetResponse = {}
pinnacle.tag.v1.AddRequest = {}
pinnacle.tag.v1.AddResponse = {}
pinnacle.tag.v1.AddResponse.Error = {}
pinnacle.tag.v1.RemoveRequest = {}
pinnacle.tag.v1.MoveToOutputRequest = {}
pinnacle.tag.v1.MoveToOutputResponse = {}
Expand Down
10 changes: 9 additions & 1 deletion api/lua/pinnacle/tag.lua
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ end
---@param ... string The names of the new tags.
---
---@return pinnacle.tag.TagHandle[] tags Handles to the created tags.
---@return pinnacle.tag.AddError|nil err Error on failure.
---
---@overload fun(output: pinnacle.output.OutputHandle, tag_names: string[]): pinnacle.tag.TagHandle[]
function tag.add(output, ...)
Expand All @@ -154,11 +155,15 @@ function tag.add(output, ...)

if err then
log.error(err)
return {}
return {}, nil
end

assert(response)

if response.error and response.error.output_does_not_exist then
return {}, { output_does_not_exist = true }
end

---@type pinnacle.tag.TagHandle[]
local handles = {}

Expand Down Expand Up @@ -272,6 +277,9 @@ local signal_name_to_SignalName = {
---@field output_does_not_exist boolean?
---This operation would cause the provided windows to be on multiple outputs.
---@field same_window_on_two_outputs pinnacle.window.WindowHandle[]?
---@class pinnacle.tag.AddError
---`true` if the output does not exist.
---@field output_does_not_exist boolean?

---Connects to a tag signal.
---
Expand Down
7 changes: 7 additions & 0 deletions api/protobuf/pinnacle/tag/v1/tag.proto
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,14 @@ message AddRequest {
repeated string tag_names = 2;
}
message AddResponse {
message Error {
oneof kind {
google.protobuf.Empty output_does_not_exist = 1;
}
}

repeated uint32 tag_ids = 1;
optional Error error = 2;
}

message RemoveRequest {
Expand Down
4 changes: 3 additions & 1 deletion api/rust/examples/default_config/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,9 @@ async fn config() {

// Setup all monitors with tags "1" through "9"
output::for_each_output(move |output| {
let mut tags = tag::add(output, tag_names);
let Ok(mut tags) = tag::add(output, tag_names) else {
return;
};
tags.next().unwrap().set_active(true);
});

Expand Down
2 changes: 1 addition & 1 deletion api/rust/src/output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ pub async fn get_focused_async() -> Option<OutputHandle> {
/// # use pinnacle_api::tag;
/// // Add tags 1-3 to all outputs and set tag "1" to active
/// output::for_each_output(|op| {
/// let mut tags = tag::add(op, ["1", "2", "3"]);
/// let Ok(mut tags) = tag::add(op, ["1", "2", "3"]) else { return };
/// tags.next().unwrap().set_active(true);
/// });
/// ```
Expand Down
41 changes: 28 additions & 13 deletions api/rust/src/tag.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use pinnacle_api_defs::pinnacle::{
tag::v1::{
AddRequest, GetActiveRequest, GetNameRequest, GetOutputNameRequest, GetRequest,
MoveToOutputRequest, RemoveRequest, SetActiveRequest, SwitchToRequest,
move_to_output_response::error::Kind,
add_response::error::Kind as AddErrorKind, move_to_output_response::error::Kind,
},
util::v1::SetOrToggle,
};
Expand All @@ -53,29 +53,37 @@ use crate::{
/// # use pinnacle_api::output;
/// # use pinnacle_api::tag;
/// // Add tags 1-5 to the focused output
/// if let Some(op) = output::get_focused() {
/// let tags = tag::add(&op, ["1", "2", "3", "4", "5"]);
/// }
/// # || -> Result<(), tag::AddError> {
/// let Some(op) = output::get_focused() else { return Ok(()); };
/// let tags = tag::add(&op, ["1", "2", "3", "4", "5"])?;
/// # Ok(())
/// # };
/// ```
pub fn add<I, T>(output: &OutputHandle, tag_names: I) -> impl Iterator<Item = TagHandle> + use<I, T>
pub fn add<I, T>(
output: &OutputHandle,
tag_names: I,
) -> Result<impl Iterator<Item = TagHandle> + use<I, T>, AddError>
where
I: IntoIterator<Item = T>,
T: ToString,
{
let output_name = output.name();
let tag_names = tag_names.into_iter().map(|name| name.to_string()).collect();

Client::tag()
let response = Client::tag()
.add(AddRequest {
output_name,
tag_names,
})
.block_on_tokio()
.unwrap()
.into_inner()
.tag_ids
.into_iter()
.map(|id| TagHandle { id })
.into_inner();

let error = response.error.and_then(|error| error.kind);
match error {
None => Ok(response.tag_ids.into_iter().map(|id| TagHandle { id })),
Some(AddErrorKind::OutputDoesNotExist(_)) => Err(AddError::OutputDoesNotExist),
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in that case I think the PR is good to go

}
}

/// Gets handles to all tags across all outputs.
Expand Down Expand Up @@ -167,7 +175,7 @@ pub async fn get_on_output_async(name: impl ToString, output: &OutputHandle) ->
/// # use pinnacle_api::tag;
/// # use pinnacle_api::output;
/// # || {
/// let tags = tag::add(&output::get_by_name("DP-1")?, ["1", "2", "Buckle", "Shoe"]);
/// let tags = tag::add(&output::get_by_name("DP-1")?, ["1", "2", "Buckle", "Shoe"]).ok()?;
///
/// tag::remove(tags); // "DP-1" no longer has any tags
/// # Some(())
Expand Down Expand Up @@ -240,6 +248,13 @@ where
}
}

/// Error that happens when adding tags to a different output.
#[derive(Debug, PartialEq, Clone)]
pub enum AddError {
/// The requested output to add tags to, does not exist
OutputDoesNotExist,
}

/// Connects to a [`TagSignal`].
///
/// # Examples
Expand Down Expand Up @@ -390,8 +405,8 @@ impl TagHandle {
/// # use pinnacle_api::tag;
/// # use pinnacle_api::output;
/// # || {
/// let tags =
/// tag::add(&output::get_by_name("DP-1")?, ["1", "2", "Buckle", "Shoe"]).collect::<Vec<_>>();
/// let tags = tag::add(&output::get_by_name("DP-1")?, ["1", "2", "Buckle", "Shoe"]).ok()?
/// .collect::<Vec<_>>();
///
/// tags[1].remove();
/// tags[3].remove();
Expand Down
17 changes: 9 additions & 8 deletions src/api/tag.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ pub mod v1;
use std::mem;

use indexmap::IndexSet;
use tracing::warn;

use crate::{
output::OutputName,
Expand Down Expand Up @@ -69,13 +68,9 @@ pub fn add(
state: &mut State,
tag_names: impl IntoIterator<Item = String>,
output_name: OutputName,
) -> Vec<Tag> {
) -> Result<Vec<Tag>, TagAddError> {
let Some(output) = output_name.output(&state.pinnacle) else {
warn!(
"Tried to add tags to output {} but it doesn't exist",
output_name.0
);
return Vec::new();
return Err(TagAddError::OutputDoesNotExist);
};

let new_tags = tag_names.into_iter().map(Tag::new).collect::<Vec<_>>();
Expand Down Expand Up @@ -106,7 +101,13 @@ pub fn add(
state.pinnacle.signal_state.tag_created.signal(tag);
}

new_tags
Ok(new_tags)
}

#[derive(Debug, PartialEq, Clone)]
pub enum TagAddError {
/// Its impossible to add tags to an output that does not exist. Create it first
OutputDoesNotExist,
}

pub fn remove(state: &mut State, tags_to_remove: Vec<Tag>) {
Expand Down
20 changes: 16 additions & 4 deletions src/api/tag/v1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,11 +131,23 @@ impl v1::tag_service_server::TagService for super::TagService {
let tag_names = request.tag_names;

run_unary(&self.sender, move |state| {
let tags = crate::api::tag::add(state, tag_names, output_name);

let tag_ids = tags.into_iter().map(|tag| tag.id().to_inner()).collect();
use crate::api::tag::TagAddError;
use pinnacle_api_defs::pinnacle::tag::v1::add_response::{Error, error::Kind};

let (tag_ids, error) = match crate::api::tag::add(state, tag_names, output_name) {
Ok(tags) => (
tags.into_iter().map(|tag| tag.id().to_inner()).collect(),
None,
),
Err(TagAddError::OutputDoesNotExist) => (
Vec::new(),
Some(Error {
kind: Some(Kind::OutputDoesNotExist(())),
}),
),
};

Ok(AddResponse { tag_ids })
Ok(AddResponse { tag_ids, error })
})
.await
}
Expand Down
11 changes: 10 additions & 1 deletion src/handlers/ext_workspace.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use crate::api::tag::TagAddError;
use crate::delegate_ext_workspace;
use crate::output::OutputName;
use crate::protocol::ext_workspace::{ExtWorkspaceHandler, ExtWorkspaceManagerState};
Expand Down Expand Up @@ -28,7 +29,15 @@ impl ExtWorkspaceHandler for State {
}

fn add_workspace(&mut self, output: &Output, name: String) {
crate::api::tag::add(self, [name.clone()], OutputName(output.name()));
match crate::api::tag::add(self, [name.clone()], OutputName(output.name())) {
Err(TagAddError::OutputDoesNotExist) => {
tracing::warn!(
output_name = output.name(),
"Tried to add tags to output but it doesn't exist",
);
}
Ok(_) => (),
}
}
}

Expand Down
1 change: 1 addition & 0 deletions tests/integration/api/tag.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ fn tag_add() {
&pinnacle_api::output::get_focused().unwrap(),
["nubby's", "number", "factory"],
);
let tags = tags.unwrap();
assert_eq!(tags.count(), 3);
}),
Lang::Lua => spawn_lua_blocking! {
Expand Down
1 change: 1 addition & 0 deletions wlcs_pinnacle/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ mod inner {
async fn config() {
pinnacle_api::output::for_each_output(|output| {
pinnacle_api::tag::add(output, ["1"])
.unwrap()
.next()
.unwrap()
.set_active(true);
Expand Down
Loading