Add playback controls #231
Conversation
|
Finally figured out how to actually pass the style linter, it turns out vim's pymode doesn't quite cover the same stuff as pep8 |
|
The pause functionality works great for me. I'm actually going to use this in a presentation next week. Thanks! |
|
Thanks for the PR @cactorium. I appreciate the work you put into this, however I won't be able to merge this now. First and foremost, if you read our contribution guidelines (which is also linked/mentioned during PR creation) you would notice we prefer to discuss any new features before implementing them. This way we can save time of contributors and maintainers, by pointing contributors to alternative/existing solutions, simplify approach etc etc. This also allows us to accept (and later maintain) features that make sense to most of asciinema users. The other thing is we're working on asciinema 2.0 on We are yet to see if we'd like this particular feature to be part of 2.0, or maybe 2.1. In general, asciinema's main mission is to smoothly record and share on the web, so in-terminal playback is nice, but not essential. However I still think this might be a useful addition, so I'm open to having a separate GH issue to discuss this. |
|
Guys, I believe this is a nice little contribution, why not spending 5min to review and incorporate? That's the open source spirit no? |
|
To be honest, I'm not up for spending what sounds like weeks or months of
discussion to maybe to land a 22 line change. If you or anyone else is
interested, go for it :)! Afaict this'd merge almost perfectly fine on the
v2 branch, you'd just need to fork off my branch, rebase off of v2, and add
in a little bit of code to use the local variable stdout instead of
asciinema.stdout()
…On Fri, Oct 20, 2017, 6:37 PM Fernando P. ***@***.***> wrote:
Guys, I believe this is a nice little contribution, why not spending 5min
to review and incorporate? That's the open source spirit no?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#231 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABcW3684GGxh4UJRwMsQ0Po8ocyyHSKSks5suSCegaJpZM4PaCGH>
.
|
|
For what is worth I forked your repo and merged this PR branch in the develop branch. |
|
@ferdonline I really don't like that you're doing this "open source spirit" guilt trip as a way to force this PR to be merged. I didn't say we don't want this functionality, and I explained why it wasn't merged at this point. I'm pretty sure you are not the one who would later maintain this functionality. Like most open source maintainers I'm working on asciinema in my free time and it happens more often than not that people submit new features which I myself later have to maintain, because they simply move on. Doesn't matter whether it's 22 lines change or 220. Sorry for not being this "accept-all-the-PRs" guy, but I don't think rage-forking is a good definition of "open source spirit" either. |
|
Sorry if any of that ill will is my fault. I understand the burdens
involved with maintaining code, which is why I was and am totally willing
to leave this PR closed. My instructions were meant for setting up and the
testing this code on the right branch in case anyone else wants to try to
work on this feature, not for trying to start a fork of the project
…On Sat, 21 Oct 2017 at 11:47 Marcin Kulik ***@***.***> wrote:
@ferdonline <https://github.com/ferdonline> I really don't like that
you're doing this "open source spirit" guilt trip as a way to force this PR
to be merged.
I didn't say we don't want this functionality, and I explained why it
wasn't merged at this point. I'm pretty sure you are not the one who would
later maintain this functionality. Like most open source maintainers I'm
working on asciinema in my free time and it happens more often than not
that people submit new features which *I myself* later have to maintain,
because they simply move on. Doesn't matter whether it's 22 lines change or
220. Sorry for not being this "accept-all-the-PRs" guy, but I don't think
rage-forking is a good definition of "open source spirit" either.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#231 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABcW3xLM_179Zt2FuXiTOlV9akjDz2pzks5suhIAgaJpZM4PaCGH>
.
|
|
So I took a stab at this again. @cactorium's patch required some major changes to work with current develop branch, but we have it now: fb1e25d. Thanks Kelvin for the original implementation! (I looked for a way to set multiple authors on git commit but it seems there can be only one author, so you're credited in the commit message) |
|
I'm glad you guys managed to add it in! |
Resolves #133 , but using some different controls because it's simpler to implement.
This was fairly simple so I'm guessing I'm missing some reasons why this wasn't implemented earlier, so please lemme know what changes need to be made!