Skip to content
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

Merged
merged 5 commits into from Jul 29, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion Cargo.toml
Expand Up @@ -12,4 +12,5 @@ sdl2 = "0.19"
sdl2_ttf = "0.19"
serde = "1.0.10"
serde_derive = "1.0.10"
serde_json = "1.0.2"
serde_json = "1.0.2"
clap = "*"
25 changes: 25 additions & 0 deletions src/error.rs
@@ -1,3 +1,28 @@
use std::{error, result};
use std::fmt::Display;

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

pub trait OrExit {
Copy link
Member

@rexim rexim Jul 29, 2017

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.

Copy link
Contributor Author

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.

Copy link
Member

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

type T;

fn or_exit(self, message: &str) -> Self::T;
}

impl<T, E> OrExit for result::Result<T, E>
where E: Display {
type T = T;

fn or_exit(self, message: &str) -> Self::T {
use std;
use std::io::Write;

match self {
Ok(value) => value,
Err(e) => {
writeln!(&mut std::io::stderr(), "{}: {}", message, e).unwrap();
std::process::exit(-1);
}
}
}
}
141 changes: 95 additions & 46 deletions src/main.rs
Expand Up @@ -8,6 +8,7 @@ extern crate sdl2;
extern crate sdl2_ttf;
extern crate portmidi as pm;
extern crate num;
extern crate clap;

extern crate serde;
extern crate serde_json;
Expand All @@ -30,82 +31,130 @@ mod error;
mod path;

use midi::PortMidiNoteTracker;
use pm::PortMidiDeviceId as DeviceId;
use ui::Popup;
use screen::*;
use config::Config;
use hardcode::*;
use error::Result;

fn print_devices(pm: &pm::PortMidi) {
for dev in pm.devices().unwrap() {
println!("{}", dev);
}
}
use error::{Result, OrExit};

fn config_path() -> Result<PathBuf> {
env::home_dir()
.ok_or("Home directory not found".into())
.map(|config_dir| config_dir.join(hardcode::CONFIG_FILE_NAME))
}

fn main() {
let context = pm::PortMidi::new().unwrap();

let (input_id, output_id) = {
let args: Vec<String> = std::env::args().collect();
type Looper = looper::Looper<PortMidiNoteTracker>;

if args.len() < 2 {
print_devices(&context);
println!("Usage: ./dimooper <input-port> <output-port>");
std::process::exit(1);
}

(args[1].trim().parse().unwrap(), args[2].trim().parse().unwrap())
};

let in_info = context.device(input_id).unwrap();
println!("Listening on: {} {}", in_info.id(), in_info.name());
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> {
Copy link
Member

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 out_info = try!(context.device(output_id));
println!("Sending recorded events: {} {}",
out_info.id(),
out_info.name());
let out_port = context.output_port(out_info, 1024).unwrap();
let out_port = try!(context.output_port(out_info, 1024));
let looper = looper::Looper::new(PortMidiNoteTracker::new(out_port));
Ok(looper)
}

let window_width = RATIO_WIDTH * RATIO_FACTOR;
fn create_event_loop(context: &pm::PortMidi, input_id: DeviceId) -> Result<EventLoop<'static>> {
let window_width = RATIO_WIDTH * RATIO_FACTOR;
let window_height = RATIO_HEIGHT * RATIO_FACTOR;
let sdl_context = sdl2::init().unwrap();
let video_subsystem = sdl_context.video().unwrap();
let timer_subsystem = sdl_context.timer().unwrap();
let ttf_context = sdl2_ttf::init().unwrap();

let window = video_subsystem.window("Dimooper", window_width, window_height)
let in_info = try!(context.device(input_id));
println!("Listening on: {} {}", in_info.id(), in_info.name());
let in_port = try!(context.input_port(in_info, 1024));

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

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.


let window = try!(video_subsystem.window("Dimooper", window_width, window_height)
.position_centered()
.opengl()
.build()
.unwrap();

let renderer = window.renderer().build().unwrap();
.build());

let bpm_popup = {
let font = ttf_context.load_font(Path::new(TTF_FONT_PATH), 50).unwrap();
Popup::new(font)
};
let renderer = try!(window.renderer().build());
let event_pump = try!(sdl_context.event_pump());
let event_loop = EventLoop::new(timer_subsystem, event_pump, in_port, renderer);
Ok(event_loop)
}

let event_pump = sdl_context.event_pump().unwrap();
fn create_popup() -> Result<Popup> {
let ttf_context = try!(sdl2_ttf::init());
let font = try!(ttf_context.load_font(Path::new(TTF_FONT_PATH), 50));
let popup = Popup::new(font);
Ok(popup)
}

let looper = looper::Looper::new(PortMidiNoteTracker::new(out_port));
fn main() {
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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

use clap::{App, AppSettings, Arg, SubCommand};

let context = pm::PortMidi::new().or_exit("Unable to initialize PortMidi");
let devices = context.devices()
.or_exit("Unable to get list of devices")
.iter()
.map(ToString::to_string)
.collect::<Vec<_>>()
.join("\n");

let input_id_arg = Arg::with_name("INPUT_ID")
.help("Input device id")
.index(1)
.required(true);

let output_id_arg = Arg::with_name("OUTPUT_ID")
.help("Output device id")
.index(2)
.required(true);

let ids = &[input_id_arg, output_id_arg];

let matches = App::new("Dimooper")
.about("Digital music looper")
.after_help(format!("Avaliable devices:\n{}", devices).as_ref())
.setting(AppSettings::SubcommandRequiredElseHelp)
.subcommand(SubCommand::with_name("looper")
.about("Looper mode")
.args(ids))
.subcommand(SubCommand::with_name("keyboard")
.about("Keyboard configuration mode")
.args(ids))
.get_matches();

let (mode, matches) = matches.subcommand();
let (input_id, output_id) = matches.map(|matches| {
let input_id = matches.value_of("INPUT_ID")
.unwrap() // arg is required
.parse()
.or_exit("Unable to parse input device id");
let output_id = matches.value_of("OUTPUT_ID")
.unwrap()
.parse()
.or_exit("Unable to parse output device id");

(input_id, output_id)
}).unwrap(); // subcommand is required

let mut config = config_path()
.and_then(|path| Config::load(path.as_path()))
// TODO(f19dedf2-afdb-4cd9-9dab-20ebbe89fd9d): Output the path to the config file
.map_err(|err| { println!("[WARNING] Cannot load config: {}. Using default config.", err); err })
.unwrap_or_default();

let mut event_loop = EventLoop::new(timer_subsystem, event_pump, in_port, renderer);
// event_loop.run(LooperScreen::<PortMidiNoteTracker>::new(looper, bpm_popup, &config));
config = event_loop.run(KeyboardLayoutScreen::new(config));
let mut event_loop = create_event_loop(&context, input_id)
.or_exit("Initialization error");
match mode {
"looper" => {
let bpm_popup = create_popup().or_exit("Unable to create popup");
let looper = create_looper(&context, output_id)
.or_exit("Looper initialization error");
event_loop.run(LooperScreen::<PortMidiNoteTracker>::new(looper, bpm_popup, &config))
},
"keyboard" => {
config = event_loop.run(KeyboardLayoutScreen::new(config));
},
_ => unreachable!()
}

config_path()
.and_then(|path| config.save(path.as_path()))
Expand Down