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

Refactor running shell command async #71

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ZimbiX
Copy link

@ZimbiX ZimbiX commented Apr 19, 2024

Facilitates paradoxxxzero#768.


This change is Reviewable

Copy link
Owner

@mgalgs mgalgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ZimbiX)


[email protected]/extension.js line 2160 at r1 (raw file):

    }
    refresh() {
        runCommandAndHandleOutputAsync(['/bin/bash', Me.dir.get_path() + '/gpu_usage.sh'], this._handleOutput.bind(this))

Maybe rather than passing a class method and doing the this binding we could just pass an arrow function (since this will be the Gpu instance from the outer scope).

asyncExecute([...], (success, lines) => if (success) this._readTemperature(lines));

[email protected]/extension.js line 2280 at r1 (raw file):

}

// For running a command asynchronously in refresh() to avoid freezing Gnome Shell

Since this could become a core function for the extension (#72) I'd like to make sure it has some nice documentation and a streamlined name.

ChatGPT's recommended docstrings and renaming look pretty good if you want to borrow that!

/**
 * Executes a shell command asynchronously and invokes a callback function with the formatted output and error object.
 * This function simplifies error handling and output processing for the caller by directly providing
 * the success status, output data as an array of lines, the subprocess object for further manipulation, and any error object if occurred.
 *
 * @param {string[]} commandAsArray - The shell command to execute, represented as an array of strings.
 * @param {Function} callback - The callback function that is called with the result of the command execution.
 *                              This function should take four parameters: (success, lines, proc, error).
 *                              'success' is a boolean indicating if the command was executed successfully,
 *                              'lines' is an array of strings representing the output of the command split by newline,
 *                              'proc' is the Gio.Subprocess object for additional actions if necessary,
 *                              'error' is the JavaScript Error object if the command failed, providing more detail about the issue.
 *
 * Example Usage:
 * asyncExecute(['/bin/bash', '/path/to/script.sh'], (success, lines, proc, error) => {
 *     if (success) {
 *         lines.forEach(line => console.log("Output Line:", line));
 *     } else {
 *         console.error('Script execution failed:', error.message);
 *     }
 * });
 */
const asyncExecute = (commandAsArray, callback) => {
    try {
        // Create a subprocess to execute the shell command and capture STDOUT
        let proc = new Gio.Subprocess({
            argv: commandAsArray,
            flags: Gio.SubprocessFlags.STDOUT_PIPE
        });
        proc.init(null);

        // Asynchronously handle the output once the command has been executed
        proc.communicate_utf8_async(null, null, (proc, result) => {
            let [ok, stdout] = proc.communicate_utf8_finish(result);
            let lines = stdout.trim().split('\n');  // Splitting the output into lines and trimming whitespace
            callback(ok, lines, proc, null);  // Calling the callback with structured data and no error object
        });
    } catch (err) {
        // Immediately call the callback with failure if an exception is caught
        callback(false, [], null, err);  // Passing the whole error object to the callback
        global.logError(err.message);
    }
}

[email protected]/extension.js line 2287 at r1 (raw file):

        proc.init(null);
        // Asynchronously call the output handler when command output is ready
        proc.communicate_utf8_async(null, null, Lang.bind(this, handleOutputFn));

I'm trying to understand the Lang.bind call. Why is it needed? We already bound this for our handler function to the Gpu instance in the call to runCommandAndHandleOutputAsync above, so the additional binding here seems redundant.

Actually if I'm reading things correctly the Lang.bind call isn't just redundant, it's confusing and possibly incorrect. Since this is a top-level arrow function I'm pretty sure the this object is just global (or whatever the GJS top-level window equivalent is). But we want this for _handleOutput to be bound to the Gpu instance. I'm a little surprised that this is actually working (seems like this._readTemperature would be undefined in _handleOutput since this is just the global object at that point), which makes me think I'm missing something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants