Skip to content

/ media

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

delegate player example args parsing to clap #277

Merged
merged 1 commit into from Jul 11, 2019
Merged

delegate player example args parsing to clap #277

merged 1 commit into from Jul 11, 2019
+61 −18

Conversation

@khodzha
Copy link
Contributor

khodzha commented Jul 6, 2019

Issue #259.

I'm not sure if --no-video and --gl help strings are valid though.
@ceyusa
Copy link
Contributor

ceyusa commented Jul 8, 2019

I already have a patch for this but "forgot" it because I'm rewriting the player example.

I should change that a push these things before duplicating work.
@ceyusa
ceyusa reviewed Jul 8, 2019
View changes
examples/Cargo.toml Outdated
@@ -20,6 +20,10 @@ webrender = "0.58.0"
websocket = "0.22"
ipc-channel = "0.11"

[dependencies.clap]
version = "2.33"
default-features = false

This comment has been minimized.

Sign in to view
@ceyusa

ceyusa Jul 8, 2019

Contributor

why are you disabling the default features?

I would just add clap = "2.33" in the dependencies section in alphabetic order.

This comment has been minimized.

Sign in to view
@khodzha

khodzha Jul 8, 2019

Author Contributor

to be honest i was assuming you wouldnt want dependecies bloat and since these features arent critical i can just disable them

default features are suggestions (for typos), color for coloring errors and vecmap for a slight performance, you can see them here

This comment has been minimized.

Sign in to view
@ceyusa

ceyusa Jul 10, 2019

Contributor

I saw the Cargo.lock diff and I got your point.

Though, as Captain Nitpick, I'd still change this to

[dependencies]
clap = { version = "2.33", default-features = false }
env_logger = "0.5"
examples/player/main.rs
};
let clap_matches = clap::App::new("Servo-media player example")
.setting(clap::AppSettings::DisableVersion)
.usage("player [--gl|--no-video] <FILE>")

This comment has been minimized.

Sign in to view
@ceyusa

ceyusa Jul 8, 2019

Contributor

The autogenerated usage string should be good enough, isn't it?

This comment has been minimized.

Sign in to view
@khodzha

khodzha Jul 8, 2019

Author Contributor

Maybe its my fault but the default usage string I got from clap here was:
<FILE> <--gl|--no-video> implying either of gl or no-video is required.
So this was the solution i went for.

Maybe it could be handled with args tuning but I couldn't find a way.

This comment has been minimized.

Sign in to view
@ceyusa

ceyusa Jul 10, 2019

Contributor

Agreed
examples/player/main.rs
.arg(
clap::Arg::with_name("gl")
.long("gl")
.display_order(1)

This comment has been minimized.

Sign in to view
@ceyusa

ceyusa Jul 8, 2019

Contributor

is display_order necessary?

This comment has been minimized.

Sign in to view
@khodzha

khodzha Jul 8, 2019

Author Contributor

No, it just reorders them in help message, bringing default --help flag to the bottom, looked fancier to me.

This comment has been minimized.

Sign in to view
@ceyusa

ceyusa Jul 10, 2019

Contributor

Agreed
examples/player/main.rs
panic!("Usage: cargo run --bin player [--gl] <file_path>")
};
let clap_matches = clap::App::new("Servo-media player example")
.setting(clap::AppSettings::DisableVersion)

This comment has been minimized.

Sign in to view
@ceyusa

ceyusa Jul 8, 2019

Contributor

nice!

Though I added

.author("Servo developers")
.about("Servo/MediaPlayer example using WebRender")
examples/player/main.rs Outdated
clap::Arg::with_name("gl")
.long("gl")
.display_order(1)
.help("use opengl"),

This comment has been minimized.

Sign in to view
@ceyusa

ceyusa Jul 8, 2019

Contributor

In my version:

.help("Tries to render frames as GL textures")
.conflicts_with("NOVIDEO"),

The conflict_with is important.
examples/player/main.rs Outdated
clap::Arg::with_name("no-video")
.long("no-video")
.display_order(2)
.help("dont render video"),

This comment has been minimized.

Sign in to view
@ceyusa

ceyusa Jul 8, 2019

Contributor 
 .help("Don't render video, only audio"),
examples/player/main.rs Outdated
.value_name("FILE")
.display_order(3),
)
.group(clap::ArgGroup::with_name("use_gl").args(&["gl", "no-video"]))

This comment has been minimized.

Sign in to view
@ceyusa

ceyusa Jul 8, 2019

Contributor

This grouping is required?

This comment has been minimized.

Sign in to view
@khodzha

khodzha Jul 8, 2019

Author Contributor

It's an alternative to .conflicts_with("NOVIDEO"), so I suppose its just a matter of preference
examples/player/main.rs Outdated

let no_video = clap_matches.is_present("no-video");
let use_gl = clap_matches.is_present("gl");
let filename = clap_matches.value_of("file").unwrap();

This comment has been minimized.

Sign in to view
@ceyusa

ceyusa Jul 8, 2019

Contributor 
+    let path = matches
+        .value_of("FILE")
+        .and_then(|s| Some(Path::new(s)))
+        .unwrap();
 
-    let path = Path::new(filename);
@khodzha khodzha force-pushed the khodzha:player_clap branch from 5be703c to 1991fdb Jul 8, 2019
@khodzha
Copy link
Contributor Author

khodzha commented Jul 8, 2019 •
edited

also sorry for kicking this from under you, i should've commented in the issue first
@khodzha khodzha force-pushed the khodzha:player_clap branch 3 times, most recently from 98eb47e to 41925cf Jul 9, 2019
@khodzha
delegate player example args parsing to clap
Loading status checks…
eb29563
@khodzha khodzha force-pushed the khodzha:player_clap branch from 41925cf to eb29563 Jul 11, 2019
@ceyusa
Copy link
Contributor

ceyusa commented Jul 11, 2019

lgtm!
@ceyusa
Copy link
Contributor

ceyusa commented Jul 11, 2019

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Jul 11, 2019

📌 Commit eb29563 has been approved by ceyusa
@bors-servo
Copy link
Contributor

bors-servo commented Jul 11, 2019

⌛ Testing commit eb29563 with merge f4101b7...
bors-servo added a commit that referenced this pull request Jul 11, 2019
@bors-servo
Auto merge of #277 - khodzha:player_clap, r=ceyusa
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
f4101b7 
delegate player example args parsing to clap

Issue #259.

I'm not sure if `--no-video` and `--gl` help strings are valid though.
@bors-servo
Copy link
Contributor

bors-servo commented Jul 11, 2019

☀️ Test successful - checks-travis
Approved by: ceyusa
Pushing f4101b7 to master...
@bors-servo bors-servo merged commit eb29563 into servo:master Jul 11, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ceyusa ceyusa

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@khodzha @ceyusa @bors-servo
You can’t perform that action at this time.