~mil/mepo-devel

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
8 2

[RFC PATCH mepo 0/5] Make shellpipe_sync cancelable

Details
Message ID
<20240709141534.516065-1-cnx@loang.net>
DKIM signature
pass
Download raw message
There are two problems with patch 3/5:

1. The intended way to kill a process and all its descendents recursively
   is through process group.  Zig's high-level abstraction for spawning
   child processes currently does not support process group.
2. Semantically correct signals for termination does not work on the
   spawned process and its descendents.  The only ones I found working
   are SIGUSR{1,2} which is a hack relying on their default action.

Due to the first issue, the patch series when applied is supposed
to be built with Zig's standard library as patched here:
https://github.com/ziglang/zig/pull/20556

I will keep that PR compatible with the latest patch series sent here.
To build Mepo with the patched std lib, first acquire Zig 0.13.0's lib
(your distro would install it in $PREFIX/lib/zig, or clone/download Zig
from upstream and look into lib), then apply the following:
https://github.com/ziglang/zig/pull/20556.diff

Then build Mepo with the additional flag --zig-lib-dig:

zig build --zig-lib-dir /path/to/zig-lib-dir

Nguyễn Gia Phong (5):
  Move get_env_vars to api/shellpipe_async.zig
  Derive shellpipe_sync from *_async
  Create button and keybind to cancel sync shellpipe
  Switch to alpha blend mode
  Gray out UI during sync shellpipe

 src/Mepo.zig                |  54 +++++++++++------
 src/api/shellpipe_async.zig |  55 ++++++++++++++----
 src/api/shellpipe_sync.zig  | 113 ++++++++++++++++++------------------
 src/blit/blit.zig           |  43 +++++++++-----
 src/types.zig               |   1 +
 src/util/utilsdl.zig        |   2 +-
 6 files changed, 167 insertions(+), 101 deletions(-)

-- 
2.45.1

[RFC PATCH mepo 1/5] Move get_env_vars to api/shellpipe_async.zig

Details
Message ID
<20240709141534.516065-2-cnx@loang.net>
In-Reply-To
<20240709141534.516065-1-cnx@loang.net> (view parent)
DKIM signature
pass
Download raw message
Patch: +27 -30
---
 src/api/shellpipe_async.zig | 28 +++++++++++++++++++++++++++-
 src/api/shellpipe_sync.zig  | 29 -----------------------------
 2 files changed, 27 insertions(+), 30 deletions(-)

diff --git a/src/api/shellpipe_async.zig b/src/api/shellpipe_async.zig
index e0f722ef8ddf..5650e1434061 100644
--- a/src/api/shellpipe_async.zig
+++ b/src/api/shellpipe_async.zig
@@ -7,7 +7,6 @@ const utildbg = @import("../util/utildbg.zig");
const utilsdl = @import("../util/utilsdl.zig");
const p = @import("../util/utilprefs.zig");
const sdl = @import("../sdlshim.zig");
const get_env_vars = @import("./shellpipe_sync.zig").get_env_vars;

const AsyncShellpipeRequest = struct {
    mepo: *Mepo,
@@ -102,3 +101,30 @@ fn async_shellpipe_run_catch_errors(mepo: *Mepo, unique_handle_id: i8, cmd: []co
    _ = try child.kill();
    _ = mepo.async_shellpipe_threads.swapRemove(unique_handle_id);
}

fn get_env_vars(mepo: *Mepo, allocator: std.mem.Allocator) !std.process.EnvMap {
    var v = try std.process.getEnvMap(allocator);

    // Window and zoom info
    try v.put("MEPO_WIN_W", try std.fmt.allocPrint(allocator, "{d}", .{mepo.win_w}));
    try v.put("MEPO_WIN_H", try std.fmt.allocPrint(allocator, "{d}", .{mepo.win_h}));
    try v.put("MEPO_ZOOM", try std.fmt.allocPrint(allocator, "{d}", .{p.get(p.pref.zoom).u}));

    // Center position
    try v.put("MEPO_CENTER_LAT", try std.fmt.allocPrint(allocator, "{d}", .{p.get(p.pref.lat).f}));
    try v.put("MEPO_CENTER_LON", try std.fmt.allocPrint(allocator, "{d}", .{p.get(p.pref.lon).f}));

    // Bbox
    const bbox = mepo.bounding_box();
    try v.put("MEPO_TL_LAT", try std.fmt.allocPrint(allocator, "{d}", .{bbox.topleft_lat}));
    try v.put("MEPO_TL_LON", try std.fmt.allocPrint(allocator, "{d}", .{bbox.topleft_lon}));
    try v.put("MEPO_BR_LAT", try std.fmt.allocPrint(allocator, "{d}", .{bbox.bottomright_lat}));
    try v.put("MEPO_BR_LON", try std.fmt.allocPrint(allocator, "{d}", .{bbox.bottomright_lon}));

    // Cursor position
    const cursor_latlon = mepo.cursor_latlon();
    try v.put("MEPO_CURSOR_LAT", try std.fmt.allocPrint(allocator, "{d}", .{cursor_latlon.Lat}));
    try v.put("MEPO_CURSOR_LON", try std.fmt.allocPrint(allocator, "{d}", .{cursor_latlon.Lon}));

    return v;
}
diff --git a/src/api/shellpipe_sync.zig b/src/api/shellpipe_sync.zig
index d0f6186b7d18..41e2cb94b0bc 100644
--- a/src/api/shellpipe_sync.zig
+++ b/src/api/shellpipe_sync.zig
@@ -51,32 +51,3 @@ fn shellpipe_sync(mepo: *Mepo, cmd: []const u8) !void {
        try mepo.update_debug_message(process_result.stderr);
    }
}

// TODO: perhaps this should be moved to Mepo.zig ? used by both shellpipe_{,a}sync
// TODO: document ENV vars usable in shell scripts context
pub fn get_env_vars(mepo: *Mepo, allocator: std.mem.Allocator) !std.process.EnvMap {
    var v = try std.process.getEnvMap(allocator);

    // Window and zoom info
    try v.put("MEPO_WIN_W", try std.fmt.allocPrint(allocator, "{d}", .{mepo.win_w}));
    try v.put("MEPO_WIN_H", try std.fmt.allocPrint(allocator, "{d}", .{mepo.win_h}));
    try v.put("MEPO_ZOOM", try std.fmt.allocPrint(allocator, "{d}", .{p.get(p.pref.zoom).u}));

    // Center position
    try v.put("MEPO_CENTER_LAT", try std.fmt.allocPrint(allocator, "{d}", .{p.get(p.pref.lat).f}));
    try v.put("MEPO_CENTER_LON", try std.fmt.allocPrint(allocator, "{d}", .{p.get(p.pref.lon).f}));

    // Bbox
    const bbox = mepo.bounding_box();
    try v.put("MEPO_TL_LAT", try std.fmt.allocPrint(allocator, "{d}", .{bbox.topleft_lat}));
    try v.put("MEPO_TL_LON", try std.fmt.allocPrint(allocator, "{d}", .{bbox.topleft_lon}));
    try v.put("MEPO_BR_LAT", try std.fmt.allocPrint(allocator, "{d}", .{bbox.bottomright_lat}));
    try v.put("MEPO_BR_LON", try std.fmt.allocPrint(allocator, "{d}", .{bbox.bottomright_lon}));

    // Cursor position
    const cursor_latlon = mepo.cursor_latlon();
    try v.put("MEPO_CURSOR_LAT", try std.fmt.allocPrint(allocator, "{d}", .{cursor_latlon.Lat}));
    try v.put("MEPO_CURSOR_LON", try std.fmt.allocPrint(allocator, "{d}", .{cursor_latlon.Lon}));

    return v;
}
-- 
2.45.1

[RFC PATCH mepo 2/5] Derive shellpipe_sync from *_async

Details
Message ID
<20240709141534.516065-3-cnx@loang.net>
In-Reply-To
<20240709141534.516065-1-cnx@loang.net> (view parent)
DKIM signature
pass
Download raw message
Patch: +57 -57
Instead of actually blocking the event loop,
use alternative event handlers.
---
 src/Mepo.zig                | 34 ++++++++++++-----------
 src/api/shellpipe_async.zig | 26 +++++++++++-------
 src/api/shellpipe_sync.zig  | 54 +++++++++++++++----------------------
 3 files changed, 57 insertions(+), 57 deletions(-)

diff --git a/src/Mepo.zig b/src/Mepo.zig
index b8f9fe3bc4a9..558103dd96e1 100644
--- a/src/Mepo.zig
+++ b/src/Mepo.zig
@@ -35,6 +35,7 @@ pin_group_active_item: ?u32 = null,
pin_groups: [10]std.ArrayList(types.Pin) = undefined,
renderer: *sdl.SDL_Renderer = undefined,
renderer_type: types.RendererType = .Auto,
sync_shellpipe_pid: ?std.process.Child.Id = null,
table_gestures: std.array_hash_map.AutoArrayHashMap(types.GestureInput, []const u8),
table_clicks: std.array_hash_map.AutoArrayHashMap(types.ClickInput, []const u8),
table_keybindings: std.array_hash_map.AutoArrayHashMap(types.KeyInput, []const u8),
@@ -510,24 +511,25 @@ pub fn sdl_event_loop(mepo: *@This()) !void {
        // Process SDL events
        if (sdl.SDL_WaitEventTimeout(&e, config.DragThresholdTicks) > 0) {
            const pending_fn = switch (e.type) {
                sdl.SDL_FINGERDOWN => &event_fingerdown,
                sdl.SDL_FINGERUP => &event_fingerup,
                sdl.SDL_KEYUP => &event_keyup,
                sdl.SDL_MOUSEBUTTONDOWN => &event_mousebuttondown,
                sdl.SDL_MOUSEBUTTONUP => &event_mousebuttonup,
                sdl.SDL_MOUSEMOTION => &event_mousemotion,
                sdl.SDL_MOUSEWHEEL => &event_mousewheel,
                sdl.SDL_MULTIGESTURE => &event_multigesture,
                sdl.SDL_TEXTINPUT => &event_textinput,
                sdl.SDL_QUIT => &event_quit,
                sdl.SDL_WINDOWEVENT => &event_windowevent,
                else => b: {
                    // Async events triggered outside of main thread:
                    // 1) signals & 2) mepolang trigged via shellpipe or STDIN
                    if (e.type == utilsdl.sdl_usereventtype(.Mepolang)) break :b &event_mepolangexecution;
                    if (e.type == utilsdl.sdl_usereventtype(.Signal)) break :b &event_signal;
                    // Unhandled case
                    break :b &event_unhandled;
                else => if (e.type == utilsdl.sdl_usereventtype(.Signal))
                    &event_signal
                else if (e.type == utilsdl.sdl_usereventtype(.Mepolang))
                    &event_mepolangexecution // via shellpipe or stdin
                else if (mepo.sync_shellpipe_pid == null) switch (e.type) {
                    sdl.SDL_FINGERDOWN => &event_fingerdown,
                    sdl.SDL_FINGERUP => &event_fingerup,
                    sdl.SDL_KEYUP => &event_keyup,
                    sdl.SDL_MOUSEBUTTONDOWN => &event_mousebuttondown,
                    sdl.SDL_MOUSEBUTTONUP => &event_mousebuttonup,
                    sdl.SDL_MOUSEMOTION => &event_mousemotion,
                    sdl.SDL_MOUSEWHEEL => &event_mousewheel,
                    sdl.SDL_MULTIGESTURE => &event_multigesture,
                    sdl.SDL_TEXTINPUT => &event_textinput,
                    else => &event_unhandled,
                } else switch (e.type) {
                    else => &event_unhandled,
                },
            };

diff --git a/src/api/shellpipe_async.zig b/src/api/shellpipe_async.zig
index 5650e1434061..2fdb142a9237 100644
--- a/src/api/shellpipe_async.zig
+++ b/src/api/shellpipe_async.zig
@@ -38,21 +38,28 @@ fn async_shellpipe(mepo: *Mepo, unique_handle_id: i8, cmd: []const u8) !void {

fn async_shellpipe_run(userdata: ?*anyopaque) callconv(.C) c_int {
    const shellpipe_request: *AsyncShellpipeRequest = @alignCast(@ptrCast(userdata.?));
    defer shellpipe_request.mepo.allocator.destroy(shellpipe_request);
    async_shellpipe_run_catch_errors(shellpipe_request.mepo, shellpipe_request.unique_handle_id, shellpipe_request.cmd) catch |err| {
    const mepo = shellpipe_request.mepo;
    defer mepo.allocator.destroy(shellpipe_request);
    const unique_handle_id = shellpipe_request.unique_handle_id;
    run_shellpipe(mepo, unique_handle_id, shellpipe_request.cmd) catch |err| {
        utildbg.log("Error running async shellpipe: {}\n", .{err});
    };
    _ = mepo.async_shellpipe_threads.swapRemove(unique_handle_id);
    return 0;
}

fn async_shellpipe_run_catch_errors(mepo: *Mepo, unique_handle_id: i8, cmd: []const u8) !void {
/// Spawn given command and wait for its output from another thread.
/// If async_id is null (i.e. called from shellpipe_sync), signal the UI
/// to soft-block until the command completes or is canceled.
pub fn run_shellpipe(mepo: *Mepo, async_id: ?i8, cmd: []const u8) !void {
    defer mepo.allocator.free(cmd);
    const thread_id = sdl.SDL_GetThreadID(null);
    if (mepo.async_shellpipe_threads.get(unique_handle_id)) |already_spawned_thread_id| {
        utildbg.log("Async shellpipe id {} already present with thread_id {}\n", .{ unique_handle_id, already_spawned_thread_id });
        return;
    if (async_id) |unique_handle_id| {
        if (mepo.async_shellpipe_threads.get(unique_handle_id)) |thread_id| {
            utildbg.log("Async shellpipe id {} already present with thread ID {}\n", .{ unique_handle_id, thread_id });
            return;
        }
        try mepo.async_shellpipe_threads.put(unique_handle_id, sdl.SDL_GetThreadID(null));
    }
    try mepo.async_shellpipe_threads.put(unique_handle_id, thread_id);

    // More or less from zig 0.9 stdlib child process's spawnPosix
    const argv = [_][]const u8{ "sh", "-c", cmd };
@@ -65,6 +72,8 @@ fn async_shellpipe_run_catch_errors(mepo: *Mepo, unique_handle_id: i8, cmd: []co
    const env = try get_env_vars(mepo, mepo.allocator);
    child.env_map = &env;
    try child.spawn();
    if (async_id == null)
        mepo.sync_shellpipe_pid = child.id;
    var stdout = std.ArrayList(u8).init(mepo.allocator);
    errdefer stdout.deinit();

@@ -99,7 +108,6 @@ fn async_shellpipe_run_catch_errors(mepo: *Mepo, unique_handle_id: i8, cmd: []co
        }
    }
    _ = try child.kill();
    _ = mepo.async_shellpipe_threads.swapRemove(unique_handle_id);
}

fn get_env_vars(mepo: *Mepo, allocator: std.mem.Allocator) !std.process.EnvMap {
diff --git a/src/api/shellpipe_sync.zig b/src/api/shellpipe_sync.zig
index 41e2cb94b0bc..7c5951e9235f 100644
--- a/src/api/shellpipe_sync.zig
+++ b/src/api/shellpipe_sync.zig
@@ -1,12 +1,9 @@
const Mepo = @import("../Mepo.zig");
const types = @import("../types.zig");
const std = @import("std");
const utilconversion = @import("../util/utilconversion.zig");
const utilplatform = @import("../util/utilplatform.zig");
const utildbg = @import("../util/utildbg.zig");
const utilsdl = @import("../util/utilsdl.zig");
const p = @import("../util/utilprefs.zig");
const sdl = @import("../sdlshim.zig");
const run_shellpipe = @import("shellpipe_async.zig").run_shellpipe;

pub const spec = .{
    .name = "shellpipe_sync",
@@ -19,35 +16,28 @@ pub const spec = .{

fn execute(mepo: *Mepo, args: [types.MepoFnNargs]types.MepoArg) !void {
    const cmd = args[0].Text;
    try shellpipe_sync(mepo, cmd);
    var request = try mepo.allocator.create(Request);
    request.mepo = mepo;
    request.cmd = try mepo.allocator.dupeZ(u8, cmd);
    for (mepo.uibuttons.items[0..1]) |*button| {
        if (!button.only_visible_when_sync_shellpipe)
            unreachable;
        mepo.allocator.free(button.text);
        button.text = try std.fmt.allocPrintZ(mepo.allocator, "Cancel {s}", .{ cmd });
    }
    sdl.SDL_DetachThread(sdl.SDL_CreateThread(run_sync_shellpipe, "sync_shellpipe", request));
}

fn shellpipe_sync(mepo: *Mepo, cmd: []const u8) !void {
    var arena = std.heap.ArenaAllocator.init(mepo.allocator);
    defer arena.deinit();
const Request = struct {
    mepo: *Mepo,
    cmd: []const u8,
};

    try mepo.update_debug_message(
        try std.fmt.allocPrint(arena.allocator(), "Shellpipe sync ({s}) in progress - run!", .{cmd}),
    );
    try mepo.blit();
    const env_vars = try get_env_vars(mepo, arena.allocator());
    const args = [_][]const u8{ "sh", "-c", cmd };
    const process_result = try std.process.Child.run(.{
        .allocator = arena.allocator(),
        .argv = args[0..],
        .env_map = &env_vars,
        .max_output_bytes = 10000 * 1024,
    });
    const exitcode = process_result.term.Exited;
    try mepo.update_debug_message(
        try std.fmt.allocPrint(arena.allocator(), "Shellpipe sync ({s}) completed - execute, {d}!", .{ cmd, process_result.stdout.len }),
    );
    try mepo.blit();
    if (exitcode == 0) {
        try mepo.mepolang_execute(process_result.stdout);
        try mepo.update_debug_message(null);
    } else {
        utildbg.log("shellpipe error: exited with code {d}\n{s}", .{ exitcode, process_result.stderr });
        try mepo.update_debug_message(process_result.stderr);
    }
fn run_sync_shellpipe(userdata: ?*anyopaque) callconv(.C) c_int {
    const request: *Request = @alignCast(@ptrCast(userdata.?));
    defer request.mepo.allocator.destroy(request);
    run_shellpipe(request.mepo, null, request.cmd) catch |err|
        utildbg.log("Error running sync shellpipe: {}\n", .{err});
    request.mepo.sync_shellpipe_pid = null;
    return 0;
}
-- 
2.45.1

[RFC PATCH mepo 3/5] Create button and keybind to cancel sync shellpipe

Details
Message ID
<20240709141534.516065-4-cnx@loang.net>
In-Reply-To
<20240709141534.516065-1-cnx@loang.net> (view parent)
DKIM signature
pass
Download raw message
Patch: +60 -2
The button's text contains the shellpipe command to be canceled.

Escape key is bound for cancel to mirror Zenity's behavior.

Implements: https://todo.sr.ht/~mil/mepo-tickets/65
---
 src/Mepo.zig                | 20 +++++++++++++++++--
 src/api/shellpipe_async.zig |  1 +
 src/api/shellpipe_sync.zig  | 38 +++++++++++++++++++++++++++++++++++++
 src/blit/blit.zig           |  2 ++
 src/types.zig               |  1 +
 5 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/src/Mepo.zig b/src/Mepo.zig
index 558103dd96e1..feb597f6b123 100644
--- a/src/Mepo.zig
+++ b/src/Mepo.zig
@@ -12,6 +12,7 @@ const p = @import("util/utilprefs.zig");
const utildbg = @import("util/utildbg.zig");
const utilsdl = @import("util/utilsdl.zig");
const datastructure = @import("datastructure/datastructure.zig");
const shellpipe_sync = @import("api/shellpipe_sync.zig");
const zoom_relative = @import("./api/zoom_relative.zig").zoom_relative;
const FnTable = @import("./api/_FnTable.zig");

@@ -224,7 +225,7 @@ fn event_mousemotion(mepo: *@This(), e: sdl.SDL_Event) types.Pending {
    return .None;
}

fn within_touch_bounds(mepo: *@This(), x: c_int, y: c_int) bool {
pub fn within_touch_bounds(mepo: *@This(), x: c_int, y: c_int) bool {
    return x > 0 and y > 0 and x < mepo.win_w and y < mepo.win_h;
}

@@ -529,6 +530,8 @@ pub fn sdl_event_loop(mepo: *@This()) !void {
                    sdl.SDL_TEXTINPUT => &event_textinput,
                    else => &event_unhandled,
                } else switch (e.type) {
                    sdl.SDL_KEYUP => &shellpipe_sync.event_keyup,
                    sdl.SDL_MOUSEBUTTONUP => &shellpipe_sync.event_mousebuttonup,
                    else => &event_unhandled,
                },
            };
@@ -797,7 +800,20 @@ pub fn init(allocator: std.mem.Allocator, tile_cache: *TileCache, use_config: []
            break :pin_groups pgs;
        },
        .tile_cache = tile_cache,
        .uibuttons = std.ArrayList(types.UIButton).init(allocator),
        .uibuttons = b: {
            var uibuttons = std.ArrayList(types.UIButton).init(allocator);
            errdefer uibuttons.deinit();
            try uibuttons.append(.{
                .text = try allocator.dupeZ(u8, "Cancel sync shellpipe"),
                .group_number = null,
                .only_visible_when_sync_shellpipe = true,
                .only_visible_when_pin_active = false,
                // Hardcoded action to kill the current sync shellpipe process
                .mepolang_click_single = "",
                .mepolang_click_hold = "",
            });
            break :b uibuttons;
        },
        .renderer_type = renderer_type,
    });
}
diff --git a/src/api/shellpipe_async.zig b/src/api/shellpipe_async.zig
index 2fdb142a9237..2df134791452 100644
--- a/src/api/shellpipe_async.zig
+++ b/src/api/shellpipe_async.zig
@@ -71,6 +71,7 @@ pub fn run_shellpipe(mepo: *Mepo, async_id: ?i8, cmd: []const u8) !void {
    child.stdout_behavior = .Pipe;
    const env = try get_env_vars(mepo, mepo.allocator);
    child.env_map = &env;
    child.pgid = 0; // create new process group
    try child.spawn();
    if (async_id == null)
        mepo.sync_shellpipe_pid = child.id;
diff --git a/src/api/shellpipe_sync.zig b/src/api/shellpipe_sync.zig
index 7c5951e9235f..17ef541e453a 100644
--- a/src/api/shellpipe_sync.zig
+++ b/src/api/shellpipe_sync.zig
@@ -2,6 +2,7 @@ const Mepo = @import("../Mepo.zig");
const types = @import("../types.zig");
const std = @import("std");
const utildbg = @import("../util/utildbg.zig");
const blitfns = @import("../blit/blit.zig");
const sdl = @import("../sdlshim.zig");
const run_shellpipe = @import("shellpipe_async.zig").run_shellpipe;

@@ -41,3 +42,40 @@ fn run_sync_shellpipe(userdata: ?*anyopaque) callconv(.C) c_int {
    request.mepo.sync_shellpipe_pid = null;
    return 0;
}

/// Helper function for SDL event handler during sync shellpipe
/// for killing the spawn processes.
fn kill_sync_shellpipe(mepo: *Mepo) types.Pending {
    if (mepo.sync_shellpipe_pid) |pid| {
        // FIXME: SIGHUP, SIGINT, SIGPIPE and SIGTERM are somehow swallowed
        // by descendent processes, even though no trap relevant trap
        // is set in the shell scripts or zenity, so we rely on SIGUSR1's
        // default action (termination).  User scripts must not handle SIGUSR1.
        if (std.posix.kill(-pid, std.posix.SIG.USR1)) |_|
            utildbg.log("Killing sync shellpipe process {}\n", .{ pid })
        else |err|
            utildbg.log("Error killing sync shellpipe process {}: {}\n", .{ pid, err });
        // No need to wait for termination as run_shellpipe already handles HUP events.
        return .Redraw;
    } else return .None;
}

pub fn event_keyup(mepo: *Mepo, e: sdl.SDL_Event) types.Pending {
    return switch (e.key.keysym.sym) {
        // XXX: Make binding configurable?  Escape key also cancel Zenity.
        sdl.SDLK_ESCAPE => kill_sync_shellpipe(mepo),
        else => .None,
    };
}

pub fn event_mousebuttonup(mepo: *Mepo, _: sdl.SDL_Event) types.Pending {
    if (mepo.fingers.items.len > 1) return .None;
    const cursor = mepo.scaled_mouse_position();
    if (!mepo.within_touch_bounds(cursor.x, cursor.y)) return .None;
    mepo.drag = null;
    return if (blitfns.blit_uibuttons(mepo, cursor) catch null) |clicked_btn|
        if (clicked_btn.only_visible_when_sync_shellpipe)
            kill_sync_shellpipe(mepo)
        else .None
    else .None;
}
diff --git a/src/blit/blit.zig b/src/blit/blit.zig
index 5abba0c9c69d..80e1955d88c5 100644
--- a/src/blit/blit.zig
+++ b/src/blit/blit.zig
@@ -744,6 +744,8 @@ pub fn blit_uibuttons(mepo: *Mepo, determine_click_target: ?sdl.SDL_Point) !?typ

        if (mepo.uibuttons.items[btn_i].only_visible_when_pin_active and mepo.pin_group_active_item == null)
            continue;
        if (mepo.uibuttons.items[btn_i].only_visible_when_sync_shellpipe == (mepo.sync_shellpipe_pid == null))
            continue;

        const surf = try utilsdl.errorcheck_ptr(
            sdl.SDL_Surface,
diff --git a/src/types.zig b/src/types.zig
index 7e2a47eb9ef7..a3c95dd28296 100644
--- a/src/types.zig
+++ b/src/types.zig
@@ -29,6 +29,7 @@ pub const UIButton = struct {
    mepolang_click_single: [:0]const u8,
    mepolang_click_hold: [:0]const u8,
    only_visible_when_pin_active: bool,
    only_visible_when_sync_shellpipe: bool = false,
    group_number: ?u8,
};

-- 
2.45.1

[RFC PATCH mepo 4/5] Switch to alpha blend mode

Details
Message ID
<20240709141534.516065-5-cnx@loang.net>
In-Reply-To
<20240709141534.516065-1-cnx@loang.net> (view parent)
DKIM signature
pass
Download raw message
Patch: +16 -16
New opacity for white is calculated assuming the destination's
brightness of 80% (mean of water, green land and other land).
---
 src/blit/blit.zig    | 30 +++++++++++++++---------------
 src/util/utilsdl.zig |  2 +-
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/src/blit/blit.zig b/src/blit/blit.zig
index 80e1955d88c5..b39778dded0f 100644
--- a/src/blit/blit.zig
+++ b/src/blit/blit.zig
@@ -301,7 +301,7 @@ fn blit_debugmessage(mepo: *Mepo) !void {
            mepo,
            0xff0000,
            null,
            types.Color{ .value = 0xffffff, .opacity = 200 },
            types.Color{ .value = 0xffffff, .opacity = 240 },
            .{ .h = mepo.win_h - bottom_off, .align_x = .Left, .align_y = .Bottom },
            0,
            5,
@@ -417,9 +417,9 @@ fn blit_pin(mepo: *Mepo, pin: *types.Pin, prev_pin: ?*types.Pin, pin_group: u8,
            break :render_pin_label;

        const pin_label_bg_value: u24 = if (is_active) 0xe8e8e8 else 0xffffff;
        const pin_label_bg: types.Color = .{ .value = pin_label_bg_value, .opacity = 255 };
        const pin_label_bg: types.Color = .{ .value = pin_label_bg_value };
        const pin_label_border_value: u24 = if (is_active) 0x000000 else 0xe8e8e8;
        const pin_label_border = .{ .value = pin_label_border_value, .opacity = 255 };
        const pin_label_border = .{ .value = pin_label_border_value };

        const label_color: u24 = 0x000000;
        const label = lab: {
@@ -449,13 +449,13 @@ fn blit_overlay_debugbar(mepo: *Mepo) !void {

    const bg: types.Color = color: {
        if (mepo.tile_cache.thread_download == null) {
            break :color .{ .value = 0xffdfd1, .opacity = 255 };
            break :color .{ .value = 0xffdfd1 };
        } else if (mepo.tile_cache.queue_lifo_ui.count() > 0) {
            break :color .{ .value = 0xd7ffd6, .opacity = 255 };
            break :color .{ .value = 0xd7ffd6 };
        } else if (mepo.tile_cache.queue_lifo_bg.count() > 0) {
            break :color .{ .value = 0xc7d4ff, .opacity = 255 };
            break :color .{ .value = 0xc7d4ff };
        } else {
            break :color .{ .value = 0xffffff, .opacity = 100 };
            break :color .{ .value = 0xffffff, .opacity = 180 };
        }
    };

@@ -531,7 +531,7 @@ fn blit_help(mepo: *Mepo) !void {
        mepo,
        0x000000,
        null,
        types.Color{ .value = 0xe8e8e8, .opacity = 120 },
        types.Color{ .value = 0xe8e8e8, .opacity = 225 },
        .{ .align_x = .Center, .align_y = .Center },
        0,
        0,
@@ -550,7 +550,7 @@ fn blit_table(mepo: *Mepo, x: i32, y: i32, padding: c_int, rows: []const [2][:0]
    var width_value: c_int = 0;
    var height_row: c_int = 0;

    const border_color = types.Color{ .value = 0x888888, .opacity = 255 };
    const border_color = types.Color{ .value = 0x888888 };

    // Precalculate width/height for each label
    for (rows) |row| {
@@ -570,7 +570,7 @@ fn blit_table(mepo: *Mepo, x: i32, y: i32, padding: c_int, rows: []const [2][:0]
    }

    // Background color & border
    try utilsdl.sdl_renderer_rect(mepo.renderer, types.Color{ .value = 0xffffff, .opacity = 125 }, sdl.SDL_Rect{
    try utilsdl.sdl_renderer_rect(mepo.renderer, types.Color{ .value = 0xffffff, .opacity = 225 }, sdl.SDL_Rect{
        .x = x,
        .y = y,
        .w = width_label + width_value + padding * 4,
@@ -794,23 +794,23 @@ pub fn blit_uibuttons(mepo: *Mepo, determine_click_target: ?sdl.SDL_Point) !?typ

            const bg_color = bg_color: {
                if (mepo.drag != null and sdl.SDL_TRUE == sdl.SDL_PointInRect(&mepo.drag.?.point, &dest_rect_text_lab)) {
                    break :bg_color types.Color{ .value = 0xe8e8e8, .opacity = 255 };
                    break :bg_color types.Color{ .value = 0xe8e8e8 };
                } else if (mepo.uibuttons.items[btn_i].group_number == mepo.pin_group_active) {
                    break :bg_color types.Color{ .value = 0xe8e8e8, .opacity = 255 };
                    break :bg_color types.Color{ .value = 0xe8e8e8 };
                } else {
                    break :bg_color types.Color{ .value = 0xffffff, .opacity = 125 };
                    break :bg_color types.Color{ .value = 0xffffff, .opacity = 225 };
                }
            };

            try utilsdl.sdl_renderer_rect(mepo.renderer, bg_color, target, .Fill);
            try utilsdl.sdl_renderer_rect(mepo.renderer, types.Color{ .value = 0x888888, .opacity = 255 }, target, .Draw);
            try utilsdl.sdl_renderer_rect(mepo.renderer, types.Color{ .value = 0x888888 }, target, .Draw);
            if (mepo.uibuttons.items[btn_i].group_number) |group_number| {
                var highlight_rect = target;
                highlight_rect.h = 3;
                highlight_rect.x += 2;
                highlight_rect.y += 2;
                highlight_rect.w -= 4;
                const color = types.Color{ .value = p.get(p.pingroup_prop(group_number, .Color)).u24, .opacity = 255 };
                const color = types.Color{ .value = p.get(p.pingroup_prop(group_number, .Color)).u24 };
                try utilsdl.sdl_renderer_rect(mepo.renderer, color, highlight_rect, .Fill);
            }
            const text = try utilsdl.errorcheck_ptr(sdl.SDL_Texture, sdl.SDL_CreateTextureFromSurface(mepo.renderer, surf));
diff --git a/src/util/utilsdl.zig b/src/util/utilsdl.zig
index ec2f4f9a528f..d3d32e5aefbd 100644
--- a/src/util/utilsdl.zig
+++ b/src/util/utilsdl.zig
@@ -53,7 +53,7 @@ pub fn sdl_push_event_signal(signal: c_int) callconv(.C) void {

pub fn sdl_renderer_set_draw_color(renderer: *sdl.SDL_Renderer, color: types.Color) errors.SDLError!void {
    const sdl_color = color.to_sdl();
    const blend_mode = if (sdl_color.a != sdl.SDL_ALPHA_OPAQUE) sdl.SDL_BLENDMODE_ADD else sdl.SDL_BLENDMODE_NONE;
    const blend_mode = if (sdl_color.a != sdl.SDL_ALPHA_OPAQUE) sdl.SDL_BLENDMODE_BLEND else sdl.SDL_BLENDMODE_NONE;
    try errorcheck(sdl.SDL_SetRenderDrawBlendMode(renderer, @intCast(blend_mode)));
    try errorcheck(sdl.SDL_SetRenderDrawColor(renderer, sdl_color.r, sdl_color.g, sdl_color.b, color.opacity));
}
-- 
2.45.1

[RFC PATCH mepo 5/5] Gray out UI during sync shellpipe

Details
Message ID
<20240709141534.516065-6-cnx@loang.net>
In-Reply-To
<20240709141534.516065-1-cnx@loang.net> (view parent)
DKIM signature
pass
Download raw message
Patch: +11 -0
Implements: https://todo.sr.ht/~mil/mepo-tickets/65
---
 src/blit/blit.zig | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/src/blit/blit.zig b/src/blit/blit.zig
index b39778dded0f..eba8f7c8a613 100644
--- a/src/blit/blit.zig
+++ b/src/blit/blit.zig
@@ -732,6 +732,16 @@ fn blit_multiline_text(
    }
}

/// Grays out the UI if a sync shellpipe is in progress.
fn blit_overlay_blocked_ui(mepo: *Mepo) !void {
    if (mepo.sync_shellpipe_pid != null)
        try utilsdl.sdl_renderer_rect(mepo.renderer, .{
            .value = 0x888888, .opacity = 128,
        }, sdl.SDL_Rect{
            .x = 1, .y = 1, .w = @intCast(mepo.win_w), .h = @intCast(mepo.win_h),
        }, .Fill);
}

pub fn blit_uibuttons(mepo: *Mepo, determine_click_target: ?sdl.SDL_Point) !?types.UIButton {
    var btn_i = mepo.uibuttons.items.len;
    const pad: c_int = 5;
@@ -844,6 +854,7 @@ pub fn blit(mepo: *Mepo) !void {
    try blit_overlay_pindetails(mepo);
    try blit_overlay_debugbar(mepo);
    try blit_help(mepo);
    try blit_overlay_blocked_ui(mepo);
    try blit_debugmessage(mepo);
    _ = try blit_uibuttons(mepo, null);

-- 
2.45.1
Details
Message ID
<46089d48-15aa-4f98-b105-1514526ab001@app.fastmail.com>
In-Reply-To
<20240709141534.516065-1-cnx@loang.net> (view parent)
DKIM signature
pass
Download raw message
Hey Phong -

Thanks for your work on implementation & sending, comments on the two
points below:

> 1. The intended way to kill a process and all its descendents recursively
>    is through process group.  Zig's high-level abstraction for spawning
>    child processes currently does not support process group.

I see that the pgid PR was finally merged in the last few days which
is great news. Thank you for opening the PR on Zig & figuring out the
correct way this should be done in Zig's stdlib.

I've rebased your changes & added a few minor fixups on the branch
`cnx-shellpipe-plus-rebase`. On that branch I also did a bit of work to
extract the shell exec implementation to `utilprocess.run_subprocess`.

I think the way forward (atleast for now) is likely just to sub
out the implementation for utilprocess.run_subprocess to rather use
C's fork/setpgid/exec since mepo builds against libc in anycase. By
extracting to a separate helper function we can keep this integration
(zig vs c way) relatively isolated. By using C subprocess spawning we
should be able to get pgid spawning without waiting for zig to roll a
minor or patch release; and can switch back to the zig stdlib way of
doing things when a new zig release is versioned.

> 2. Semantically correct signals for termination does not work on the
>    spawned process and its descendents.  The only ones I found working
>    are SIGUSR{1,2} which is a hack relying on their default action.

In my testing SIGKILL on the spawned process works fine & consistently
so I just switched over to that.

Really great work, thank you for taking this on. This will be a huge UX
improvement once merged.

Miles
Details
Message ID
<D2Z5D1PEOH22.RUAADZQJPJ6H@guix>
In-Reply-To
<46089d48-15aa-4f98-b105-1514526ab001@app.fastmail.com> (view parent)
DKIM signature
pass
Download raw message
On 2024-07-23 at 18:33-04:00, Miles Alan wrote:
> > 2. Semantically correct signals for termination does not work on the
> >    spawned process and its descendents.  The only ones I found working
> >    are SIGUSR{1,2} which is a hack relying on their default action.
>
> In my testing SIGKILL on the spawned process works fine & consistently
> so I just switched over to that.

I don't think SIGKILL is appropriate for job cancelation as it prevents
the terminated process from freeing resources, e.g. temporary file
in mepo_ui_menu_route_mobroute.sh (which BTW is not cleaned up).

I tried to asked about the swallowed SIGTERM on the Zig Matrix room
(basically the only system programming chat room I participate in)
and did not get any answer.  I will try to come up with a min reprod
to see if it's Zig-specific, and either way ask somewhere that's
_not instant_ messaging.  Not understanding why it behaves so oddly
really bugs me.

On 2024-07-23 at 18:33-04:00, Miles Alan wrote:
> I've rebased your changes & added a few minor fixups on the branch
> `cnx-shellpipe-plus-rebase`. On that branch I also did a bit of work to
> extract the shell exec implementation to `utilprocess.run_subprocess`.
>
> I think the way forward (atleast for now) is likely just to sub
> out the implementation for utilprocess.run_subprocess to rather use
> C's fork/setpgid/exec since mepo builds against libc in anycase. By
> extracting to a separate helper function we can keep this integration
> (zig vs c way) relatively isolated. By using C subprocess spawning we
> should be able to get pgid spawning without waiting for zig to roll a
> minor or patch release; and can switch back to the zig stdlib way of
> doing things when a new zig release is versioned.

Thanks, and that sounds like a good plan to me.  If you have yet
to work on the libc-based impl I would like to take a stab at it.

I took a quick look at that branch and wanna do some nitpicking as well!

On 2024-07-21 at 10:33-04:00, Miles Alan wrote:
>Remove continual stdout functionality from async_shellpipe
>---
>     run_shellpipe(mepo, unique_handle_id, ...
>     _ = mepo.async_shellpipe_threads.swapRemove(unique_handle_id);
[snip]
> pub fn run_shellpipe(mepo: *Mepo, async_id: ?i8, cmd: []const u8) !void {
[snip]
>     if (async_id) |unique_handle_id| {
[snip]
>+        defer _ = mepo.async_shellpipe_threads.swapRemove(unique_handle_id);

It seems the thread ID is removed twice here by accident
(BTW unlike Go's, defer in Zig is scoped at the block level).

I also noticed that QueueHashMap.swapRemove is always used
to just remove the item, i.e. the return value is never used
in the current codebase.

There are also some trailing whitespaces
at the function signature of utilprocess.run_subprocess.
Details
Message ID
<b28ffcca-2848-45ec-84e5-06ae4c2b6100@app.fastmail.com>
In-Reply-To
<D2Z5D1PEOH22.RUAADZQJPJ6H@guix> (view parent)
DKIM signature
pass
Download raw message
On Fri, Jul 26, 2024, at 12:37 AM, Nguyễn Gia Phong wrote:
> On 2024-07-23 at 18:33-04:00, Miles Alan wrote:
>> > 2. Semantically correct signals for termination does not work on the
>> >    spawned process and its descendents.  The only ones I found working
>> >    are SIGUSR{1,2} which is a hack relying on their default action.
>>
>> In my testing SIGKILL on the spawned process works fine & consistently
>> so I just switched over to that.
>
> I don't think SIGKILL is appropriate for job cancelation as it prevents
> the terminated process from freeing resources, e.g. temporary file
> in mepo_ui_menu_route_mobroute.sh (which BTW is not cleaned up).
>
> I tried to asked about the swallowed SIGTERM on the Zig Matrix room
> (basically the only system programming chat room I participate in)
> and did not get any answer.  I will try to come up with a min reprod
> to see if it's Zig-specific, and either way ask somewhere that's
> _not instant_ messaging.  Not understanding why it behaves so oddly
> really bugs me.

I do agree SIGKILL is less ideal as it won't allow for resource
cleanup. And yeah.. I still have a bit of work to do on the
mepo_ui_menu_route_mobroute.sh script mentioned wrt. cleanup.

I still think SIGKILL is preferable to the old SIGUSR1/2, but if we
can figure a way to make SIGTERM (which is what it should actually be)
work reliably I'm all for it. I am also quite curious if this is a zig issue
or if a libc would have the same behavior as well.


> On 2024-07-23 at 18:33-04:00, Miles Alan wrote:
>> I've rebased your changes & added a few minor fixups on the branch
>> `cnx-shellpipe-plus-rebase`. On that branch I also did a bit of work to
>> extract the shell exec implementation to `utilprocess.run_subprocess`.
>>
>> I think the way forward (atleast for now) is likely just to sub
>> out the implementation for utilprocess.run_subprocess to rather use
>> C's fork/setpgid/exec since mepo builds against libc in anycase. By
>> extracting to a separate helper function we can keep this integration
>> (zig vs c way) relatively isolated. By using C subprocess spawning we
>> should be able to get pgid spawning without waiting for zig to roll a
>> minor or patch release; and can switch back to the zig stdlib way of
>> doing things when a new zig release is versioned.
>
> Thanks, and that sounds like a good plan to me.  If you have yet
> to work on the libc-based impl I would like to take a stab at it.
>

Absolutely, feel free to take a stab at the libc based subprocess spawning
implementation. I hadn't started on it as I'm working on a few other
things at the moment.

> I took a quick look at that branch and wanna do some nitpicking as well!
>
> On 2024-07-21 at 10:33-04:00, Miles Alan wrote:
>>Remove continual stdout functionality from async_shellpipe
>>---
>>     run_shellpipe(mepo, unique_handle_id, ...
>>     _ = mepo.async_shellpipe_threads.swapRemove(unique_handle_id);
> [snip]
>> pub fn run_shellpipe(mepo: *Mepo, async_id: ?i8, cmd: []const u8) !void {
> [snip]
>>     if (async_id) |unique_handle_id| {
> [snip]
>>+        defer _ = mepo.async_shellpipe_threads.swapRemove(unique_handle_id);
>
> It seems the thread ID is removed twice here by accident
> (BTW unlike Go's, defer in Zig is scoped at the block level).
>
> I also noticed that QueueHashMap.swapRemove is always used
> to just remove the item, i.e. the return value is never used
> in the current codebase.
>
> There are also some trailing whitespaces
> at the function signature of utilprocess.run_subprocess.

Thanks for the catch on the extra defer - should be fixed now.

As for swapRemove: if returns false, that means it was just a nop
(key didn't exist). And IIRC in the majority of these cases; nop'ing
resultingly from this as well is the correct behavior (so yea it seems
the return value is never used). I suppose we could remove the return
value altogether, though it's just a proxy to AutoArrayHashMap. If
there are places you find where it shouldn't be a nop from key missing /
behavior is incorrect do let me know.

And thanks for the reminder on whitespace - I need to get back into the
habit of running zig fmt; my editor doesn't show trailing whitespace
currently.
Reply to thread Export thread (mbox)