alexherbo2 / krabby

A keyboard interface to the web, inspired by Kakoune
https://krabby.netlify.app
The Unlicense
311 stars 20 forks source link

Add Troubleshooting doc + instructions to fix mpv crashing on Vivaldi #20

Closed SeerLite closed 4 years ago

SeerLite commented 4 years ago

Closes #19

I hope the description about the problem isn't too wordy. Feel free to edit out the context of the problem if you think it's unnecessary. Also, the fix depends on settings['mpv-environment'] being implemented, of course. I also added an index, to mimic the example you linked me. I don't know if it looks weird with only one entry though.

I also changed the README and moved the inline krabby logo a little bit lower so that the new link to Troubleshooting didn't wrap around and look weird on desktop. I hope it looks fine like this.

This is my first time ever doing a pull request so forgive me if I forgot anything.

alexherbo2 commented 4 years ago

I do not see where settings['mpv-environment'] is used in Krabby.

Maybe also add an entry in the Configuration document, so that variable is documented.

SeerLite commented 4 years ago

Ah. I misunderstood your last reply in the issue then. I thought adding the actual mpv-environment was something you'd do. I hope that wasn't a dumb assumption.

Hey, I can still try to implement it myself. But as I said, I really don't know much javascript.

Should the mpv commands implementations change to something like this in extension.js then? As I understand, that's the only way to add environment variables to the call of a command. Or is there a way to set them when using extensions.shell.send() directly?

alexherbo2 commented 4 years ago

Add a default mpv-environment in krabby.js and use that variable in the shell call yeah.

SeerLite commented 4 years ago

I mean something like that. Is that alright?

Also, is it ok if instead of using the shell extension directly in the extension.js mapping, it uses mpvResume? It seems to accomplish the same thing, the only difference being that mpvResume pauses the video too (and I think that's a plus).

SeerLite commented 4 years ago

Also, is it ok if instead of using the shell extension directly in the extension.js mapping, it uses mpvResume? It seems to accomplish the same thing, the only difference being that mpvResume pauses the video too (and I think that's a plus).

The latest commit is about that. I await your thoughts on this

alexherbo2 commented 4 years ago

Using the current URL seemed more general purpose when we are not in the video context, because m now assumes it will find a video object (pausing and passing the -start flag to mpv).

alexherbo2 commented 4 years ago

@SeerLite It’s funny that despite you don’t know JavaScript, you managed to make things work. The changes look quite good to me.

Can you verify one last time everything is working and documented (I see the Configuration document has not been updated), and squash the commits into a single one (except the contributing waiver).

SeerLite commented 4 years ago

It’s funny that despite you don’t know JavaScript, you managed to make things work. The changes look quite good to me.

Haha, thanks! I did play around a bit with javascript one time and I also do have some experience with other programming languages, so the change here didn't feel too complicated. By doing this I also learned more about git/gihub so that's good too.

Using the current URL seemed more general purpose when we are not in the video context, because m now assumes it will find a video object (pausing and passing the -start flag to mpv).

Oh, I see the problem now. Using m on a page without a video will show an error in vivaldi://extensions/. What should I do about it then? Do I change the binding to use a similar implementation instead of mpvResume(), or can this be ignored?

SeerLite commented 4 years ago

(I see the Configuration document has not been updated)

I did add an example similar to the one about mpv-config in this commit though.

SeerLite commented 4 years ago

Alright so I changed m to use a similar implementation instead of using mpvResume(). I think everything looks good now.