-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add XY movement & refactor monitor types #18
base: main
Are you sure you want to change the base?
Add XY movement & refactor monitor types #18
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there, sorry for the delay in response. This looks great! I'm also fairly new to OSS and Rust, this is actually my first and only Rust project.
The refactors you did are a nice improvement to code cleanliness, and it's a great new feature.
There are a couple of changes I'd like you to make based on my vision for this, but there's no way you would've known that since I haven't put it to words yet anywhere.
short, | ||
long, | ||
help = "A desired x,y, and scale.", | ||
long_help = "A comma-separated list of displacement information '100,88,1'. The order is always X, Y, Scale. This does not check whether monitors can be arranged in that way." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like that there's a repetition between --scale
and --displacement
, and I also am not a fan of comma-separated syntax and would like to keep it to a minimum.
Can you make this --position
and allow position and scale to be set separately? Otherwise the behavior if you set both --displacement
and --scale
is unclear, and you end up with a more opaque --displacement 100,88,1
instead of --position 100,88 --scale 1
which reads a lot clearer to me.
use super::{Action}; | ||
|
||
pub struct DisplacementAction { | ||
pub displacement: Displacement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like what you've done here with the string parsing, but there are a couple issues with it:
- Like I said below, I'd rather keep the scale as a separate argument and make this just the
x,y
- The string parsing is now in the
display_config
directory, which is supposed to be a separate library. I'd rather avoid putting CLI details in the library wherever possible, which is why I had the rotation struct to begin with. This is a relatively minor transgression though so I don't mind much.
I would make a position
struct that gets parsed to an x and y value and use that as the property on this Action.
Main Change
Created an Action to handle changing monitor position in X and Y. Tied this action into the CLI. Tested functionality - tests pass.
Secondary Changes
This change also led me to refactor most of display_config. physical_monitor and virtual_monitor now share distinct types for:
There are no longer two 'rotation' types ("Actions::Rotation" and "LogicalMonitor::Transform") - Actions::Rotation has been superseded, extending the CLI's rotation capabilities to include flipping, without breaking previous behavior.
I'm relatively new to Open Source / GitHub tradition, and I'm very very new to Rust. If there's something I can do to help in any way, or if I've made a mistake in some way, please let me know.