Added support for rotation #19
Conversation
src/store.rs
Outdated
| @@ -201,6 +211,13 @@ named!(parse_position<&[u8], OutputArg>, do_parse!( | |||
| >> (OutputArg::Position(x, y)) | |||
| )); | |||
|
|
|||
| named!(parse_rotation<&[u8], OutputArg>, do_parse!( | |||
| tag!("rotation") | |||
There was a problem hiding this comment.
Could we use the same syntax as sway for this?
There was a problem hiding this comment.
Yeah I initially did but decided to split it as I was only doing rotation in a first run and sway combines rotation and other transforms into one string whereas monitors.xml doesn't.
If the design philosophy you want is more sway-centric and anything else is just a bonus, I'm more than happy to switch that back. Will probably be able to merge in flipping while I'm at it but I doubt monitors.xml support will be all that simple.
There was a problem hiding this comment.
I'd like to keep the kanshi store as close as sway's config as possible, otherwise it'll be confusing for users. It can unpack the transform command into an internal rotation field and leave flipped outputs support for later.
We can ignore monitors.xml support for now.
There was a problem hiding this comment.
Okidoki! I'll rewrite it.
|
Yeah, monitors.xml support is old and would need more love. I don't think the current implementation handles v2 at all. |
|
It doesn't from what I could see. We could do that but then it's a lot of case by case. Maybe it's worth focusing on functionality over input compatibility in a first moment. Your call at the end of the day. |
Yeah, I agree. I'm thinking of removing it for now. |
|
Ready for review. |
src/frontend.rs
Outdated
| @@ -41,9 +41,15 @@ impl SwayFrontend { | |||
| if saved.width > 0 && saved.height > 0 { | |||
| l += &format!(" resolution {}x{}", saved.width, saved.height); | |||
| } | |||
| let mut v = ["90".to_string(),"180".into(),"270".into(),"flipped".into(),"flipped-90".into(),"flipped-180".into(),"flipped-270".into()]; | |||
| if v.contains(&saved.transform) { | |||
There was a problem hiding this comment.
Maybe we can replace this with a match?
There was a problem hiding this comment.
Yes, it most certainly can.
src/frontend.rs
Outdated
| if saved.scale > 0. { | ||
| l += &format!(" scale {}", saved.scale); | ||
| } | ||
| } |
src/store.rs
Outdated
| @@ -22,7 +22,7 @@ pub struct SavedOutput { | |||
| pub rate: f32, | |||
| pub x: i32, | |||
| pub y: i32, | |||
| //pub rotation: Rotation, | |||
| pub transform: String, | |||
There was a problem hiding this comment.
I'm not sure I follow here. This may be my general inexperience talking but since we're parsing the same string we want to output, what is the benefit of transforming this to an enum? Could you briefly explain how you would see this?
There was a problem hiding this comment.
This ensures the value is valid, and isn't garbage
There was a problem hiding this comment.
Isn't this redundant with the matching in the frontend?
The way I've got it right now (not committed) is that it matches the string against the valid options and passes through if matched or passes "transform normal" if not. This serves to both handle garbage and reset the output to the normal rotation if transform is absent (something swaymsg doesn't do on it's own if the output already has a rotation prior to passing a new command).
If you'd rather I change this to an enum I'll gladly do so, but then there's little point in matching in the frontend as the parser will silently fail before it gets there in the case of incorrect input.
There was a problem hiding this comment.
Isn't this redundant with the matching in the frontend?
All frontends and backends may not use the same string.
but then there's little point in matching in the frontend as the parser will silently fail before it gets there in the case of incorrect input.
The parser silently failing is a separate issue that needs to be fixed anyway. See #18
|
Hey, |
|
Updated with your suggestions. |
|
Thanks! |
Done:
rotation <0, 90, 180, 270>.To do:
transform flipped-90)