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
218 keyboard mapping screen #234
218 keyboard mapping screen #234
Conversation
Codecov Report
@@ Coverage Diff @@
## 218-keyboard-mapping-screen #234 +/- ##
================================================================
- Coverage 39.13% 27.54% -11.59%
================================================================
Files 19 21 +2
Lines 644 915 +271
================================================================
Hits 252 252
- Misses 392 663 +271
Continue to review full report at Codecov.
|
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.
Thank you so much for spending your personal time on this contribution! I really appreaciate that! And also thank you for showing me how to use clap properly. :)
Please fix the remarks that are not marked as non-blocking and I'm gonna merge your PR.
src/main.rs
Outdated
|
||
let event_pump = sdl_context.event_pump().unwrap(); | ||
fn init_font() -> Result<Popup> { |
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.
Why does the function that is called init_font
return Popup
? How on earth does that make any sense? :D Such function should either return something different or be called differently.
src/main.rs
Outdated
|
||
let matches = App::new("Dimooper") | ||
.about("Digital music looper") | ||
.after_help(format!("Avaliable devices:\n{}", devices).as_ref()) |
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.
Available devices should not be part of the --help
output. The list of available devices is constantly changing, but for me output of --help
has always been static. Having a dynamic content on --help
just doesn't feel right.
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 think that having help
command that lists available options is okay. Some programs (e.g. Qt's configure
script) do that, and it helps.
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 think that having help command that lists available options is okay.
I also think so. Why would I think differently?
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 meant that help
could list available options for that INPUT_ID
argument. And it will be helpful.
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.
@ForNeVeR I think I understand what you mean. Yeah, you're right. I'm taking back this remark. But #234 (comment) should be addressed anyway.
src/main.rs
Outdated
.help("Output device id") | ||
.index(2) | ||
.required(true)) | ||
.arg(Arg::with_name("SCREEN") |
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 the different mode is selected via some awkward --screen
flag. I want it to be a subcommand in the git style. Like git commit
or git push
. I'm looking for dimooper keyboard 1 2
.
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.
We have 2 options here:
- Either make it necessary to provide subcommand (like it is done in git). This will lead to some code duplication as both main and keyboard subcommands require same
input_id
andoutput_id
arguments. Alsodimooper 1 2
won't work in that case (onlydimooper <main_command_name> 1 2
) because default subcommand feature is not implemented yet (see Default subcommand clap-rs/clap#975). - Or leave it as is. Maybe rename
--screen
flag to something less awkward (your suggestions?).
Which option do you prefer?
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.
This will lead to some code duplication as both main and keyboard subcommands require same
input_id
andoutput_id
arguments.
YES! That was exactly my struggle on the stream! :D Honestly, I don't know why I always overreact over such stupid things as code duplicate, I'm really sorry for my behaviour. :) Probably bad tea, I dunno...
I still insist on having the subcommand approach. I'm ok with the lack of the default command. Let's make it like:
$ dimooper looper 1 2 # looper mode
$ dimooper keyboard 1 2 # keyboard configuration mode
I'm also ok with the code duplicate for now. I think later when we have more modes it's gonna be more clear how to generalize all of that.
src/main.rs
Outdated
.help("Output device id") | ||
.index(2) | ||
.required(true)) | ||
.arg(Arg::with_name("SCREEN") |
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 the notion of screen is leaked through the abstractions right to the user. There is no such thing for the user as 'screen'. It's an internal term. The user should know only about 'subcommands' for different modes of the application.
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.
No abstraction actually leaked. Im just bad at naming things. Any suggestions? Maybe mode
?
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.
Yeah, mode
is a pretty good name. :)
|
||
let looper = looper::Looper::new(PortMidiNoteTracker::new(out_port)); | ||
fn main() { |
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 know where to put such remark, so I put it here:
When I run just dimooper
without any arguments it doesn't print the available devices. That is the most crucial use case for me and it's currently broken.
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.
It can be easily done with SubcommandRequiredElseHelp
or ArgsRequiredElseHelp
settings (see https://docs.rs/clap/2.25.1/clap/enum.AppSettings.html), but it contradicts with your view of how help
subcommand should work. Alternatively, we can handle case when no arguments provided manually.
IMO, program should not show list of devices by default. It causes 'WTF' effect. I think it should show what that program is (about) and how to use it (help).
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.
In #234 (comment) Mr @ForNeVeR convinced me (somehow) that it's ok to have available devices on --help
. So I'm ok with the SubcommandRequiredElseHelp
solution here. :)
let in_port = context.input_port(in_info, 1024).unwrap(); | ||
|
||
let out_info = context.device(output_id).unwrap(); | ||
fn create_looper(context: &pm::PortMidi, output_id: DeviceId) -> Result<Looper> { |
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.
This entire function suggests that PortMidiNoteTracker
should have additional constructor that constructs the tracker from pm::PortMidi
and DeviceId
.
This remark doesn't block the merge. Feel free to not fix it. I'm gonna file it as a techdebt to fix later.
|
||
let sdl_context = try!(sdl2::init()); | ||
let video_subsystem = try!(sdl_context.video()); | ||
let timer_subsystem = try!(sdl_context.timer()); |
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 the creation of EventLoop
is coupled with SDL initialization. EventLoop
was design as a first class entity and I will probably create several instances of it in different places. So this code should be decoupled.
This remark doesn't block the merge. Feel free to not fix it. I'm gonna file it as a techdebt to fix later.
src/main.rs
Outdated
|
||
let event_pump = sdl_context.event_pump().unwrap(); | ||
fn init_font() -> Result<Popup> { | ||
let ttf_context = try!(sdl2_ttf::init()); |
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 the creation of Popup
is coupled with SDL_ttf initialization.
This remark doesn't block the merge. Feel free to not fix it. I'm gonna file it as a techdebt to fix later.
I'm not gonna look at the unit test coverage this time, because |
@0xd34d10cc Dude! Freakin' awesome! Thank you so much for your help! You're the best! :) |
|
||
pub type Result<T> = result::Result<T, Box<error::Error>>; | ||
|
||
pub trait OrExit { |
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.
@0xd34d10cc did you invent this pattern or is it something common? :) Kinda like unwrap but with a custom message which is pretty convenient.
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.
Introducing a new trait is pretty common way to extend behaviour of some object. This pattern is called 'extension trait'. For example, tokio-openssl uses this pattern to add asyncronous versions of connect() and accept() to corresponding types from openssl crate.
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.
@0xd34d10cc haha, I know what is a trait, thanks! :D By the pattern I meant the OrExit
pattern. "We either have a result or the further executing doesn't make any sense anymore so we quit". It looks very conventional and I'm surprised that something like OrExit
is not a part of standard library. It can be also implemented by Option for example, or pretty much by anything 'unwrappable'.
Command line arguments parsing now delegated to clap (replace '*' with version that compatible with 1.17).
Initialization code was somewhat refactored.
Also removed all unsafe 'unwrap()' calls.