218 keyboard mapping screen by 0xd34d10cc · Pull Request #234 · tsoding/dimooper

0xd34d10cc · 2017-07-26T21:03:15Z

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.

…ments parsing

codecov-io · 2017-07-26T21:11:25Z

Codecov Report

Merging #234 into 218-keyboard-mapping-screen will decrease coverage by 11.58%.
The diff coverage is 0%.

@@                       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

Impacted Files	Coverage Δ
src/error.rs	`0% <0%> (ø)`
src/main.rs	`1.2% <0%> (-0.8%)`	⬇️
src/looper/looper.rs	`23.03% <0%> (-74.41%)`	⬇️
src/looper/sample.rs	`66.66% <0%> (-6.58%)`	⬇️
src/screen/keyboard_layout.rs	`0% <0%> (ø)`	⬆️
src/screen/looper_screen.rs	`0% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update caf69de...9cb5bd8. Read the comment docs.

rexim

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.

rexim · 2017-07-27T04:41:30Z

+}

-    let event_pump = sdl_context.event_pump().unwrap();
+fn init_font() -> Result<Popup> {


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.

rexim · 2017-07-27T04:42:46Z

+
+    let matches = App::new("Dimooper")
+        .about("Digital music looper")
+        .after_help(format!("Avaliable devices:\n{}", devices).as_ref())


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.

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.

I think that having help command that lists available options is okay.

I also think so. Why would I think differently?

I meant that help could list available options for that INPUT_ID argument. And it will be helpful.

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

rexim · 2017-07-27T04:43:48Z

+            .help("Output device id")
+            .index(2)
+            .required(true))
+        .arg(Arg::with_name("SCREEN")


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.

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 and output_id arguments. Also dimooper 1 2 won't work in that case (only dimooper <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?

This will lead to some code duplication as both main and keyboard subcommands require same input_id and output_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.

rexim · 2017-07-27T04:44:18Z

+            .help("Output device id")
+            .index(2)
+            .required(true))
+        .arg(Arg::with_name("SCREEN")


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.

No abstraction actually leaked. Im just bad at naming things. Any suggestions? Maybe mode?

Yeah, mode is a pretty good name. :)

rexim · 2017-07-27T04:50:58Z

+}

-    let looper = looper::Looper::new(PortMidiNoteTracker::new(out_port));
+fn main() {


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.

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

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

rexim · 2017-07-27T05:01:50Z

-    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> { 


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.

rexim · 2017-07-27T05:05:31Z

+
+    let sdl_context = try!(sdl2::init());
+    let video_subsystem = try!(sdl_context.video());
+    let timer_subsystem = try!(sdl_context.timer());


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.

rexim · 2017-07-27T05:07:22Z


-    let event_pump = sdl_context.event_pump().unwrap();
+fn init_font() -> Result<Popup> {
+    let ttf_context = try!(sdl2_ttf::init());


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.

rexim · 2017-07-27T05:13:16Z

I'm not gonna look at the unit test coverage this time, because main.rs is not really unit testable at the moment.

…sion trait for Result

rexim · 2017-07-29T04:48:34Z

@0xd34d10cc Dude! Freakin' awesome! Thank you so much for your help! You're the best! :)
@ForNeVeR you're ok too, thanks for the additional review :D

rexim · 2017-07-29T04:52:16Z


 pub type Result<T> = result::Result<T, Box<error::Error>>;
+
+pub trait OrExit {


@0xd34d10cc did you invent this pattern or is it something common? :) Kinda like unwrap but with a custom message which is pretty convenient.

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.

@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'.

0xd34d10cc added 3 commits July 27, 2017 00:20

Refactoring of initialization. Switched to clap for command line argu…

f0168aa

…ments parsing

Replaced typedef with 'use as'

0908ee0

Replaced subcommand with argument that takes value

7a9c548

rexim requested changes Jul 27, 2017

View reviewed changes

rexim assigned 0xd34d10cc Jul 27, 2017

0xd34d10cc added 2 commits July 28, 2017 21:53

renamed init_font() function

814472b

Replaced '--screen' argument with subcommand. Also added OrExit exten…

9cb5bd8

…sion trait for Result

rexim assigned rexim and unassigned 0xd34d10cc Jul 29, 2017

rexim mentioned this pull request Jul 29, 2017

Decouple components initialization from SDL initialization #235

Open

rexim merged commit 7649b86 into tsoding:218-keyboard-mapping-screen Jul 29, 2017

rexim reviewed Jul 29, 2017

View reviewed changes

rexim mentioned this pull request Jul 29, 2017

Application crashes when you change the BPM #239

Closed


		pub type Result<T> = result::Result<T, Box<error::Error>>;

		pub trait OrExit {

Conversation

0xd34d10cc commented Jul 26, 2017

Uh oh!

codecov-io commented Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rexim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xd34d10cc Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rexim commented Jul 27, 2017

Uh oh!

rexim commented Jul 29, 2017

Uh oh!

rexim Jul 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Jul 26, 2017 •

edited

Loading

0xd34d10cc Jul 27, 2017 •

edited

Loading

rexim Jul 29, 2017 •

edited

Loading