ewnd9 / record-desktop

:movie_camera: Effortless GIFs and screenshots on Linux, built with Electron.
66 stars 7 forks source link

Future-proofing slop #26

Closed naelstrof closed 7 years ago

naelstrof commented 7 years ago

The newest slop no longer prints out eval formatted strings. I didn't test this code so you probably should test this before merging, but the general idea is that you should tell slop what format you want since the default output is changing.

ewnd9 commented 7 years ago

Thank you for slop and the pull request :clap:

Could you please apply the patch below to your commit?

-f%g I believe is a stype

dim[0].split('x') should be dim.split('x') because dim is a string

diff --git a/src/unix-utils/wrappers/slop.js b/src/unix-utils/wrappers/slop.js
index fd595a4..7548920 100644
--- a/src/unix-utils/wrappers/slop.js
+++ b/src/unix-utils/wrappers/slop.js
@@ -1,9 +1,10 @@
 import { exec } from '../utils';

 export default function slop() {
-  return exec(`slop -f%g`).then(res => {
+  return exec(`slop -f "%g"`).then(res => {
     const [dim, x, y] = res.split('+');
-    const [width, height] = dim[0].split('x');
+    const [width, height] = dim.split('x');
+
     return { width, height, x, y };
   });
 };

Or, if you don't have time you could give me permission to alter your branch from pull request (https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/)

naelstrof commented 7 years ago

You should have permissions to edit, and I don't mind if you were to fix it yourself and leave out the pull request. I was just hunting down some things that use slop, and making sure that my update doesn't royally screw everyone.

Also, slop is now also a library. There's no bindings to any other languages yet, but just letting you know just in case you don't like the idea of running shell commands in javascript. 🎉

ewnd9 commented 7 years ago

No problem, I will merge it as is and fix afterward

Also, slop is now also a library. There's no bindings to any other languages yet, but just letting you know just in case you don't like the idea of running shell commands in javascript.

Good to know :+1: In this particular project, I think running shell commands is a better approach. Ideally, I want to extract everything into a few shell scripts which do all command line work instead of operating everything from js.