You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
restore unrestricted GTK4 manual resizing across portrait and multi-monitor Windows desktops while respecting taskbar and docked-appbar work areas
account for GTK client-side shadow insets so visible content reaches the work-area edge without covering the taskbar
expose the real GTK maximize button through native HTMAXBUTTON handling for Windows 11 Snap Layouts while preserving maximize/restore clicks
scope MSYS2 compiler overrides to the Rust target so MSVC-hosted build dependencies retain the Visual Studio toolchain
move Rhythmbox import from the main-window menu to a button below Library in Preferences
enforce LF-only repository text and normalize the seven legacy CRLF/mixed files
Root cause and impact
GTK 4.22 performs CSD resize drags with pointer capture and repeated SetWindowPos calls. The earlier workaround capped MINMAXINFO.max_track_size to one monitor's work area, preventing a restored window from reaching the full width of a high-DPI portrait monitor. Removing that cap restored width but allowed the manually dragged outer surface, including GTK's transparent CSD shadow, into the taskbar strip.
The new subclass preserves GTK's tracking limits, adjusts only maximized bounds in WM_GETMINMAXINFO, and constrains only the active edge during GTK's captured WM_WINDOWPOSCHANGING resize loop. It does not force WS_THICKFRAME or other frame styles.
Validation
live Windows 11 test on three 200%-DPI monitors, including a portrait secondary monitor
unrestricted manual width, exact taskbar alignment, maximize/restore, Snap Layouts, and Preferences placement confirmed by the maintainer
.\scripts\build-windows.ps1 -Clippy — strict debug/all-target and release passes
complete Windows bundle dependency closure (477 binaries), packaged runtime probe, resource validation, and ZIP creation passed on the final functional head
git diff --check
every changed text file is LF in both index and worktree
Summary by CodeRabbit
New Features
Added improved Windows Snap Layout support, including more reliable maximize, restore, resizing, and monitor boundary behavior.
Added a Rhythmbox migration action to Library Location preferences.
Changes
Moved Rhythmbox migration from the primary menu to Preferences.
Improved Windows build tooling and formatting checks for more consistent builds.
Bug Fixes
Fixed edge cases affecting window snapping and interactive resizing on Windows.
The pull request revises Windows Snap Layout integration, moves Rhythmbox migration into preferences, adds formatting enforcement, updates Windows build variables, and normalizes text-file line endings.
Changes
Windows Snap Layout support
Layer / File(s)
Summary
GTK-derived Snap Layout geometry and installation src/ui/win32_snap.rs, src/ui/window.rs
Snap Layout derives the maximize-button rectangle from GTK widgets, surface transforms, and scale. Installation returns a boolean result and uses shared state with retry handling.
Native message handling and geometry validation src/ui/win32_snap.rs, src/ui/window.rs
The Win32 subclass handles DWM messages, maximize and restore clicks, active-edge resizing, state cleanup, and expanded geometry tests.
The migration button appears in Library Location preferences and uses win.migrate-rhythmbox. The primary menu item was removed, and localization coverage scans preferences.rs.
Repository format and build tooling
Layer / File(s)
Summary
Text normalization and format checks .gitattributes, hooks/pre-commit, .github/workflows/fuzz.yml, fuzz/*, src/audio/output.rs, src/radio/api.rs, src/ui/persistence.rs
Text files use LF endings. The pre-commit hook checks both the main workspace and fuzz package. The fuzz checkout no longer persists credentials. Other listed files received formatting-only changes.
Windows build environment scripts/build-windows.ps1, tests/packaging_metadata.rs
MSYS2 tool variables include the Rust target qualifier, with packaging metadata tests for the mappings.
Focused review of src/ui/win32_snap.rs (core windowing logic)
Review other changed files (preferences, persistence, window.rs, build script, .gitattributes)
Post inline comments for specific issues
Post final summary review
Scope note
7 of the 14 changed files (.github/workflows/fuzz.yml, fuzz/Cargo.toml, fuzz/fuzz_targets/fuzz_dmap.rs, hooks/pre-commit, src/audio/output.rs, src/radio/api.rs, src/ui/persistence.rs) are pure CRLF→LF line-ending normalization — verified with git diff --ignore-all-space, which shows zero content differences for these files. Review focused on the substantive changes: .gitattributes, scripts/build-windows.ps1, src/ui/win32_snap.rs, src/ui/window.rs, src/ui/header_bar.rs, src/ui/preferences.rs, src/ui/rhythmbox_migration.rs.
Findings
1. Asymmetric retry on Snap Layout install failure (src/ui/window.rs:2474, posted inline)
The deferred path (connect_is_active_notify) retries enable_snap_layout until it returns true. The immediate "HWND ready at present" path discards the same bool return, so a first-attempt SetWindowSubclass failure (e.g. comctl32 not fully ready) silently disables Snap Layout for the whole session with only a tracing::warn!. Worth unifying so both paths share the same retry-until-success logic.
Things that look solid
win32_snap.rs FFI layer: Verified the #[repr(C)] struct field order/types for MinMaxInfo, Rect, MonitorInfo, and WindowPos against the real Win32 MINMAXINFO/RECT/MONITORINFO/WINDOWPOS layouts — all match. SetWindowSubclass/RemoveWindowSubclass/DwmDefWindowProc usage, the Arc::into_raw/Arc::from_raw lifecycle tied to WM_NCDESTROY, and the GetCapture/GetAsyncKeyState(VK_LBUTTON) guard restricting WM_WINDOWPOSCHANGING handling to an actual CSD drag are all correctly reasoned through and match their SAFETY comments.
Test coverage: the pure logic extracted into apply_monitor_work_area, apply_sizing_work_area, active_sizing_edges, physical_hit_rect, physical_csd_insets, and point_from_lparam is unit-tested, including edge/corner-drag disambiguation and half-open hit-rect bounds. Good coverage for logic that can't be exercised without live Win32 message pumping.
.gitattributes: confirmed locales/en.yml (whose old comment claimed "historically CRLF") already contains zero CR bytes in the committed blob on origin/main — the removed special-case attribute was already stale, so dropping it alongside the new blanket * text=auto eol=lf doesn't reintroduce any inconsistency.
scripts/build-windows.ps1: switching from unscoped $env:CC/$env:CXX/$env:AR/$env:DLLTOOL to target-scoped CC_<target_with_underscores> etc. via [Environment]::SetEnvironmentVariable correctly matches the cc crate's target-suffixed lookup convention, and no other unscoped $env:CC-style reference remains in the script that would rely on the old global override.
Rhythmbox migration move: win.migrate-rhythmbox action registration in window.rs is unchanged, and the rhythmbox_migration.rs test that scans source files for rhythmbox_migration.* localization keys was correctly updated to scan preferences.rs instead of header_bar.rs.
Not verified
This sandbox is Linux-only, and win32_snap.rs is #![cfg(target_os = "windows")], so none of it — including the new tests — could be compiled or run here. Review is based on static reading against documented Win32 semantics; the PR body's reported live Windows 11 multi-monitor testing and build-windows.ps1 -Test/-Clippy passes are the actual functional validation.
Codacy's "18 high" security findings weren't independently reproducible in this environment; given the file is a documented raw Win32 FFI/unsafe boundary with a SAFETY comment on every unsafe block, I'd expect most of those to be expected flags on unsafe/FFI usage rather than novel vulnerabilities, but worth a quick check of the Codacy report to confirm none are outside win32_snap.rs.
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
The PR introduces significant improvements to the Windows windowing experience and refines the migration UI. However, the current implementation is not up to standards due to high complexity and a lack of coverage in critical areas. Specifically, src/ui/win32_snap.rs has been identified as a high-risk file where complexity has spiked, masking potential edge cases in window management and memory handling.
A major blocking concern is the manual memory management within the Win32 subclassing logic, which may lead to leaks if the subclass is removed before the window is destroyed. Additionally, the acceptance criterion regarding MSYS2 target-prefixed environment variable generation remains unverified by existing tests. These issues, along with the high volume of unresolved quality findings, should be addressed before merging.
About this PR
The centralization of Win32 FFI, state management, and complex coordinate math in src/ui/win32_snap.rs without sufficient modularization or unit testing creates a high maintenance burden. Significant effort should be directed at refactoring this file and increasing test coverage for the hit-testing and resizing logic.
1 comment outside of the diffscripts/build-windows.ps1
line 375 🟡 MEDIUM RISK
Empty catch blocks prevent error visibility. Instead of an empty block, use Write-Debug or check the process status before attempting to kill it.
Verify hit-test coordinates are correctly handled as signed values for monitors with negative virtual-screen origins.
Verify that logical GTK surface coordinates and transforms are correctly mapped to physical pixels for hit-testing.
Verify correct calculation of physical CSD insets from surface and widget dimensions at various DPI scales.
Verify that maximized window bounds are clipped to the work area without restricting the manual tracking size.
Verify that interactive resizing is constrained by the taskbar only when the specific edge entering the taskbar strip is being dragged.
Verify correct identification of active sizing edges based on cursor proximity during corner and edge drags.
Verify that localization key verification tests correctly scan the new Preferences file location for migration strings.
Verify the build script generates the correct target-prefixed environment variables for different MSYS2 environments (Clang vs GCC).
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify the build script generates the correct target-prefixed environment variables for different MSYS2 environments (Clang vs GCC).
The reason will be displayed to describe this comment to others. Learn more.
🔴 HIGH RISK
Suggestion: Manual memory management via raw pointers in FFI callbacks is high-risk. While this implementation correctly balances the Arc reference count, ensure that RemoveWindowSubclass (line 790) does not prevent WM_NCDESTROY from reaching this handler, or the memory will leak.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
The function logic should be split. Separating the pure coordinate math (proposed rectangle calculations) from the side-effect-heavy Win32 API calls will make the logic easier to test and maintain. This is particularly important given the high complexity (89 delta) and lack of coverage in this file.
Try running the following prompt in your IDE agent:
Refactor the constrain_interactive_window_pos_to_work_area function in src/ui/win32_snap.rs to reduce cyclomatic complexity. Extract the logic that calculates the proposed Rect and the sizing edge detection into separate, pure helper functions that can be unit tested without FFI.
The reason will be displayed to describe this comment to others. Learn more.
Return value of enable_snap_layout is discarded on the immediate-install path.
In the else branch below (deferred install via connect_is_active_notify), a false return is stored in installed_for_handler, so the handler keeps retrying on every subsequent notify::is-active until SetWindowSubclass succeeds (src/ui/win32_snap.rs:648-674).
Here, on the "HWND ready at present" path, the same bool is dropped. If SetWindowSubclass fails on this first attempt (e.g. comctl32 not yet ready), win32_snap::enable_snap_layout only logs tracing::warn!("Failed to install Windows 11 Snap Layout subclass") — there's no retry hookup at all in this branch, so Snap Layout support is silently lost for the rest of the session.
Consider unifying: always register the notify::is-active retry handler (guarded by the same installed cell) regardless of which path performs the first attempt, so a failed immediate install still gets a second chance.
scripts/build-windows.ps1 contains U+2014 and U+2500 but has no UTF-8 BOM. Save it as UTF-8 with BOM, or make it ASCII-only.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/build-windows.ps1` around lines 1 - 7, Update the build-windows.ps1
file encoding to UTF-8 with a BOM while preserving the existing non-ASCII
characters, or replace those characters with ASCII equivalents so the script is
ASCII-only.
Both functions build a MonitorInfo, call GetMonitorInfoW, and return early on failure. A shared unsafe fn monitor_bounds(monitor: *mut c_void) -> Option<(Rect, Rect)> would remove the duplication and reduce the reported 75-line length of constrain_interactive_window_pos_to_work_area.
♻️ Sketch of the shared helper
unsafefnmonitor_bounds(monitor:*mutc_void) -> Option<(Rect,Rect)>{if monitor.is_null(){returnNone;}letmut info = MonitorInfo{size: std::mem::size_of::<MonitorInfo>()asu32,monitor:Rect::default(),work_area:Rect::default(),flags:0,};// SAFETY: monitor is non-null and info has the documented size.(unsafe{GetMonitorInfoW(monitor,&rawmut info)} != 0).then_some((info.monitor, info.work_area))}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/win32_snap.rs` around lines 499 - 628, Extract the duplicated monitor
metadata initialization and GetMonitorInfoW failure handling into an unsafe
monitor_bounds helper returning Option<(Rect, Rect)>. Update
constrain_maximized_window_to_work_area and
constrain_interactive_window_pos_to_work_area to use this helper while
preserving their existing early-return behavior and monitor/work-area values.
active_sizing_edges falls back to cursor proximity when both bounds shift. The tests cover only the unambiguous corner drag. Add a case where proposed.left and proposed.right both differ from current, and assert that the cursor position selects the expected edge.
💚 Proposed test
#[test]fnambiguous_bounds_resolve_by_cursor_proximity(){let current = Rect{left:100,top:200,right:1_100,bottom:1_000,};// Both outer bounds shifted, as DPI bookkeeping can do.let proposed = Rect{left:99,top:200,right:1_400,bottom:1_000,};assert_eq!(
active_sizing_edges(current, proposed,Point{ x:1_399, y:600}),SizingEdges{
horizontal:Some(HorizontalEdge::Right),
vertical:None,});assert_eq!(
active_sizing_edges(current, proposed,Point{ x:101, y:600}),SizingEdges{
horizontal:Some(HorizontalEdge::Left),
vertical:None,});}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/win32_snap.rs` around lines 1005 - 1027, Add an ambiguous
horizontal-bounds test near
active_edges_preserve_the_fixed_sides_of_a_corner_drag, using a proposed Rect
where both left and right differ from current. Assert that active_sizing_edges
selects the right edge for a cursor near the right bound and the left edge for a
cursor near the left bound, with no vertical edge selected.
The horizontal and vertical blocks apply the same algorithm to different fields. A shared helper that takes the two current bounds, the two proposed bounds, and the cursor coordinate would remove the duplication and lower the reported cyclomatic complexity of 13.
♻️ Sketch of the shared helper
/// Returns `Some(false)` for the low edge and `Some(true)` for the high edge.fnactive_edge(current_low:i32,current_high:i32,proposed_low:i32,proposed_high:i32,cursor:i32,) -> Option<bool>{if current_high - current_low == proposed_high - proposed_low {returnNone;}if proposed_low != current_low && proposed_high == current_high {returnSome(false);}if proposed_low == current_low && proposed_high != current_high {returnSome(true);}let low_distance = (i64::from(cursor) - i64::from(proposed_low)).abs();let high_distance = (i64::from(cursor) - i64::from(proposed_high)).abs();Some(low_distance > high_distance)}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/win32_snap.rs` around lines 454 - 497, Extract the duplicated per-axis
logic from active_sizing_edges into a shared active_edge helper accepting
current/proposed low and high bounds plus the cursor coordinate, returning the
selected low/high edge or None when the size is unchanged. Use this helper for
both horizontal and vertical detection, mapping its result to HorizontalEdge and
VerticalEdge while preserving the existing boundary and proximity behavior.
Check the standalone fuzz workspace in the formatting hook.
fuzz/Cargo.toml declares a separate [workspace]. Verify whether the root cargo fmt --check at Line 21 checks fuzz/fuzz_targets/fuzz_dmap.rs. If it does not, add a second check for fuzz/Cargo.toml, or state that this hook covers only the application workspace.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hooks/pre-commit` at line 21, Update the pre-commit formatting hook around
the existing cargo fmt check to explicitly verify the standalone fuzz workspace
declared by fuzz/Cargo.toml, including fuzz/fuzz_targets/fuzz_dmap.rs. Add a
separate cargo fmt --check invocation for that manifest while preserving the
existing application workspace check and failure behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/fuzz.yml:
- Line 29: Update the actions/checkout step in the fuzz workflow to set
persist-credentials to false, ensuring no checkout token is stored for later
repository-controlled commands; leave the existing checkout behavior unchanged.
In `@hooks/pre-commit`:
- Line 1: Change the executable mode of the hooks/pre-commit file from 100644 to
100755 so Git records it as executable and Unix-like clients can run it.
In `@scripts/build-windows.ps1`:
- Around line 1398-1409: Update the `$ToolEnvTarget` normalization to replace
both hyphens and periods with underscores before constructing the `DLLTOOL_`,
`CC_`, `CXX_`, and `AR_` environment variable names.
- Around line 1394-1409: In the target tool environment setup around
$ToolEnvTarget, set RC_$ToolEnvTarget to the MSYS2 llvm-rc.exe path in both the
clang and non-clang branches. Keep the existing DLLTOOL, CC, CXX, and AR
assignments unchanged, using the same target-specific environment-variable
mechanism.
---
Outside diff comments:
In `@scripts/build-windows.ps1`:
- Around line 1-7: Update the build-windows.ps1 file encoding to UTF-8 with a
BOM while preserving the existing non-ASCII characters, or replace those
characters with ASCII equivalents so the script is ASCII-only.
---
Nitpick comments:
In `@hooks/pre-commit`:
- Line 21: Update the pre-commit formatting hook around the existing cargo fmt
check to explicitly verify the standalone fuzz workspace declared by
fuzz/Cargo.toml, including fuzz/fuzz_targets/fuzz_dmap.rs. Add a separate cargo
fmt --check invocation for that manifest while preserving the existing
application workspace check and failure behavior.
In `@src/ui/win32_snap.rs`:
- Around line 499-628: Extract the duplicated monitor metadata initialization
and GetMonitorInfoW failure handling into an unsafe monitor_bounds helper
returning Option<(Rect, Rect)>. Update constrain_maximized_window_to_work_area
and constrain_interactive_window_pos_to_work_area to use this helper while
preserving their existing early-return behavior and monitor/work-area values.
- Around line 1005-1027: Add an ambiguous horizontal-bounds test near
active_edges_preserve_the_fixed_sides_of_a_corner_drag, using a proposed Rect
where both left and right differ from current. Assert that active_sizing_edges
selects the right edge for a cursor near the right bound and the left edge for a
cursor near the left bound, with no vertical edge selected.
- Around line 454-497: Extract the duplicated per-axis logic from
active_sizing_edges into a shared active_edge helper accepting current/proposed
low and high bounds plus the cursor coordinate, returning the selected low/high
edge or None when the size is unchanged. Use this helper for both horizontal and
vertical detection, mapping its result to HorizontalEdge and VerticalEdge while
preserving the existing boundary and proximity behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c9b03e01-6c9c-4dea-b36a-cb10c2d66f4c
📥 Commits
Reviewing files that changed from the base of the PR and between 9adc55b and 5e56cef.
Git records the hook as 100644, so Unix-like clients will not run it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hooks/pre-commit` at line 1, Change the executable mode of the
hooks/pre-commit file from 100644 to 100755 so Git records it as executable and
Unix-like clients can run it.
#!/usr/bin/env bashset -euo pipefail
python3 - <<'PY'from pathlib import Pathfor p in Path('.').rglob('Cargo.toml'): text = p.read_text(errors='replace') if any(x in text for x in ('cc', 'vswhom', 'bindgen', 'cmake')): print(f'--- {p} ---') for i, line in enumerate(text.splitlines(), 1): if any(x in line for x in ('cc', 'vswhom', 'bindgen', 'cmake', 'build-dependencies')): print(f'{i}: {line}')PYprintf'%s\n''--- Environment assignments in the PowerShell script ---'
rg -n -C 8 'SetEnvironmentVariable|RemoveItem Env:|Env:' scripts/build-windows.ps1
Repository: jm2/tributary
Length of output: 1896
🏁 Script executed:
#!/usr/bin/env bashset -euo pipefail
printf'%s\n''--- Root manifest and build script ---'
sed -n '1,175p' Cargo.toml
cat -n build.rs
printf'%s\n''--- Relevant lockfile package records ---'
python3 - <<'PY'from pathlib import Pathtext = Path('Cargo.lock').read_text()for name in ('name = "cc"', 'name = "embed-resource"', 'name = "vswhom"', 'name = "winresource"'): pos = 0 while True: pos = text.find(name, pos) if pos < 0: break start = text.rfind('[[package]]', 0, pos) end = text.find('[[package]]', pos + len(name)) print(text[start:end if end >= 0 else None].rstrip()) pos += len(name)PYprintf'%s\n''--- All repository references to native variables ---'
rg -n 'DLLTOOL|CC_|CXX_|AR_|cc::|embed_resource|winresource|vswhom' --glob '!fuzz/Cargo.lock' --glob '!Cargo.lock' --glob '*.rs' --glob '*.toml' --glob '*.ps1'.
Repository: jm2/tributary
Length of output: 13801
🌐 Web query:
cc-rs target-qualified CC CXX AR environment variables DLLTOOL embed-resource vswhom source
💡 Result:
The cc-rs crate uses various environment variables to configure the C/C++ build process, allowing for target-specific overrides and flexible integration with external tools. Environment Variables and Target Qualification The cc-rs crate supports target-qualified environment variables to allow granular control when building for specific architectures or platforms [1][2][3]. For any configuration variable (e.g., CC, CXX, AR, CFLAGS), you can provide versions prefixed or suffixed by the target triple [1][2]. The search order for these variables is as follows [1][2][3]: 1. : e.g., CC_x86_64-unknown-linux-gnu [1][2] 2. <TARGET_WITH_UNDERSCORES>: e.g., CC_x86_64_unknown_linux_gnu (where periods and dashes are replaced by underscores) [1][2] 3. <BUILD_KIND>: e.g., HOST_CC or TARGET_CFLAGS [1][2] 4. : The plain variable (e.g., CC, AR) [1][2] Core Configuration Variables - CC / CXX: Specifies the C or C++ compiler [2][4]. These can also be used to specify compiler wrappers like sccache by setting CC="sccache cc" [2][4]. - AR: Specifies the archiver executable used for static libraries [1][2]. - DLLTOOL: Used in specific cross-compilation contexts (often associated with mingw-w64 toolchains) to generate import libraries for DLLs. - CRATE_CC_NO_DEFAULTS: If set, disables the generation of default compiler flags, which can be useful to avoid conflicts in complex cross-compilation setups [2][4]. Related Tools and Integration - embed-resource: This crate is commonly used for Windows resource compilation [5]. It interacts with cc-rs to find MSVC tools and include directories [5][6]. Specifically, it uses environment variables like RC$TARGET or RC to override the default resource compiler [5]. When using LLVM-RC, it often relies on the cc-rs crate to provide the correct preprocessor configuration and environment (such as INCLUDE paths) [5][6]. - vswhom: This is an internal or related utility often used by these crates (via cc::windows_registry) to locate the MSVC toolchain on Windows, including the compiler (cl.exe), linker, and other build tools [5][6]. For programmatic configuration, the Build struct provides methods like env to set variables for child processes and try_flags_from_environment to dynamically inject flags from user-defined environment variables [1][7].
docs.rs embed-resource DLLTOOL CC target environment variables vswhom-sys
💡 Result:
The embed-resource crate is a tool for compiling and linking Windows resource files (.rc) into Rust executables via build.rs scripts [1][2]. The vswhom-sys crate provides low-level FFI bindings to a C-based discovery script (originally by Jon Blow) designed to locate Visual Studio and Windows SDK installations, which helps other tools find necessary compilers, linkers, and libraries when building C/C++ code on Windows [3][4][5]. Regarding the specific technical environment variables and interactions you mentioned: Environment Variable Overrides for embed-resource: When building for Windows, embed-resource defaults to using x86_64-w64-mingw32-windres for *-pc-windows-gnu targets and llvm-rc for *-pc-windows-msvc targets [1][6]. These defaults can be overridden by setting the RC_$TARGET, RC_${TARGET//-/_}, or RC environment variables [1][6]. Role of C Compilers and Discovery: - When using llvm-rc to compile resource files, an external C compiler is employed to preprocess the resource [1][6]. This process is pre-configured using settings from the cc crate [1][6]. - While embed-resource can attempt to find Windows SDK tools like RC.EXE or MIDL.EXE [1][7][8], it often leverages cc or vswhom (and by extension vswhom-sys) for more robust discovery of the Visual Studio environment, including compiler and linker locations [3][8][5]. - vswhom-sys specifically aids in finding paths for Visual Studio libraries (e.g., libvcruntime.lib) and executables (e.g., link.exe), as well as Windows SDK libraries (e.g., kernel32.lib) [3][5]. Note on vswhom-sys: The vswhom-sys crate is a pure FFI wrapper. Users should be aware that the underlying native code has been documented to cause memory corruption in some cases [4][9]. Most users typically interact with the higher-level vswhom crate rather than the -sys binding directly [8].
#!/usr/bin/env bashset -euo pipefail
# Inspect only the lockfile records that can introduce native build consumers.
python3 - <<'PY'from pathlib import Pathfor filename in ('Cargo.lock', 'fuzz/Cargo.lock'): text = Path(filename).read_text() print(f'--- {filename} ---') records = text.split('[[package]]') for record in records: if any(f'name = "{n}"' in record for n in ('cc', 'embed-resource', 'vswhom', 'winresource')): print('[[package]]' + record.rstrip())PYprintf'%s\n''--- Build script behavior ---'
cat -n build.rs
Repository: jm2/tributary
Length of output: 4144
🏁 Script executed:
#!/usr/bin/env bashset -euo pipefail
python3 - <<'PY'import json, urllib.requesturls = { 'embed-resource Cargo.toml': 'https://raw.githubusercontent.com/nabijaczleweli/rust-embed-resource/v3.0.11/Cargo.toml', 'embed-resource src/lib.rs': 'https://raw.githubusercontent.com/nabijaczleweli/rust-embed-resource/v3.0.11/src/lib.rs', 'cc 1.2.64 src/lib.rs': 'https://raw.githubusercontent.com/rust-lang/cc-rs/cc-1.2.64/src/lib.rs',}for label, url in urls.items(): print(f'--- {label} ---') try: text = urllib.request.urlopen(url, timeout=10).read().decode() except Exception as e: print(f'FETCH ERROR: {e}') continue lines = text.splitlines() needles = ('DLLTOOL', 'RC_', 'CC_', 'CXX_', 'AR_', 'windows_registry', 'env::var', 'var_os', 'Build::new', 'Tool::with_features') for i, line in enumerate(lines, 1): if any(n in line for n in needles): lo, hi = max(1, i-3), min(len(lines), i+5) print(f'[{lo}-{hi}]') for j in range(lo, hi+1): print(f'{j}: {lines[j-1]}')PY
The term "compile_for" refers to a function provided by the rust-embed-resource Rust crate, rather than a standard option or command for the binutils tool "dlltool" [1][2]. In the context of the rust-embed-resource crate, which is used to compile and include Windows resources (like icons or version information) in Rust projects, the compile_for function is used to associate a specific resource file with one or more designated binaries or targets [1][2]. Syntax and Usage: The function typically takes the path to the resource file, a list of binaries for which the resource should be compiled, and any additional configuration parameters [1][2]. Example: embed_resource::compile_for("assets/poke-a-mango.rc", &["poke-a-mango", "poke-a-mango-installer"], &["VERSION="0.5.0""]).manifest_required.unwrap; This allows developers to manage different resource files for different parts of a project, such as assigning a specific icon to an installer binary versus the main application binary [1][2]. If you were searching for a "compile for" option related to dlltool, no such parameter exists in the standard GNU binutils dlltool implementation.
In technical contexts, particularly within Rust build scripts and software development for Windows, CC_, RC_, and src are typically related to build configuration and environment variables: 1. CC_: Often refers to the C compiler (e.g., set via the CC environment variable). In many build systems, including those using the 'cc' crate in Rust, this variable defines the path to the C compiler executable [1]. 2. RC_: Frequently refers to the Resource Compiler (e.g., rc.exe or windres). In Windows development, the resource compiler is used to compile.rc files into.res files, which are then linked into the final executable. Build tools often look for an environment variable named RC or RC_ to locate the correct resource compiler for a specific target architecture [2][3][1]. 3. src: This is a standard convention for a directory name, typically standing for "source," which contains the source code files for a project. In Rust, it is standard for crates to have a src/ directory containing lib.rs or main.rs [4][2]. These terms often appear together in the context of build automation where a build script needs to locate compilers (CC for C code, RC for Windows resources) to process files found in the project's source directory (src) [4][2][3].
#!/usr/bin/env bashset -euo pipefail
python3 - <<'PY'import io, tarfile, urllib.requestfor crate, version in [('embed-resource', '3.0.11'), ('cc', '1.2.64')]: url = f'https://crates.io/api/v1/crates/{crate}/{version}/download' print(f'--- {crate} {version} ---') try: data = urllib.request.urlopen(url, timeout=20).read() archive = tarfile.open(fileobj=io.BytesIO(data), mode='r:gz') except Exception as e: print(f'FETCH ERROR: {e}') continue for member in archive.getmembers(): if not member.isfile() or not member.name.endswith(('.rs', '.toml')): continue text = archive.extractfile(member).read().decode(errors='replace') lines = text.splitlines() if any(token in text for token in ('DLLTOOL', 'RC_', 'CC_', 'CXX_', 'AR_', 'windows_registry', 'Build::new')): print(f'### {member.name}') for i, line in enumerate(lines, 1): if any(token in line for token in ('DLLTOOL', 'RC_', 'CC_', 'CXX_', 'AR_', 'windows_registry', 'Build::new')): for j in range(max(1, i-3), min(len(lines), i+5)+1): print(f'{j}: {lines[j-1]}') print()PY
Repository: jm2/tributary
Length of output: 495
🏁 Script executed:
#!/usr/bin/env bashset -euo pipefail
python3 - <<'PY'import io, tarfile, subprocessfor crate, version in [('embed-resource', '3.0.11'), ('cc', '1.2.64')]: url = f'https://crates.io/api/v1/crates/{crate}/{version}/download' print(f'--- {crate} {version} ---') data = subprocess.check_output(['curl', '-fsSLk', url]) archive = tarfile.open(fileobj=io.BytesIO(data), mode='r:gz') for member in archive.getmembers(): if not member.isfile() or not member.name.endswith(('.rs', '.toml')): continue text = archive.extractfile(member).read().decode(errors='replace') lines = text.splitlines() tokens = ('DLLTOOL', 'RC_', 'CC_', 'CXX_', 'AR_', 'windows_registry', 'Build::new', 'env::var') if any(token in text for token in tokens): print(f'### {member.name}') shown = set() for i, line in enumerate(lines, 1): if any(token in line for token in tokens): lo, hi = max(1, i-3), min(len(lines), i+5) if (lo, hi) in shown: continue shown.add((lo, hi)) for j in range(lo, hi+1): print(f'{j}: {lines[j-1]}') print()PY
Repository: jm2/tributary
Length of output: 768
🌐 Web query:
embed-resource 3.0.11 source RC_TARGET cc Build target gnullvm DLLTOOL
💡 Result:
The query relates to the behavior of the rust-embed-resource crate (version 3.0.11), which is a library used in Rust build scripts to handle the compilation and inclusion of Windows resources [1][2]. When building for Windows targets, especially the *-windows-gnullvm target family (e.g., aarch64-pc-windows-gnullvm), the library uses specific environmental configurations and toolchains to invoke resource compilers [3][4]. Key technical points regarding your query: 1. Resource Compilation: For *-windows-gnullvm targets, which rely on LLVM-based tools rather than the traditional GNU binutils (like windres and dlltool), the crate is designed to use compatible tools [4][5]. Specifically, llvm-rc is preferred and used for these targets [3][6]. 2. Environment Variable Overrides: The library allows users to override the default tool selection by setting environment variables such as RC_$TARGET, RC_${TARGET//-/_}, or RC [3]. 3. Interaction with cc: When using LLVM-RC, an external C compiler is employed to preprocess the resource file, using configurations derived from the cc crate [3]. Recent developments in the ecosystem have addressed issues where invalid target triples (containing the -gnullvm suffix) were passed to Clang by the cc crate; fixes involve stripping the llvm suffix to produce a valid target string (e.g., converting aarch64-pc-windows-gnullvm to aarch64-pc-windows-gnu for the compiler invocation) [7][8]. 4. dlltool: The gnullvm target architecture intentionally moves away from reliance on GNU dlltool [4]. Efforts within the Rust ecosystem have sought to further reduce reliance on dlltool for creating DLL import libraries to improve compatibility and reduce complexity [5]. In summary, the specific combination of build targets and tools you mentioned is part of the orchestration required to manage Windows resource embedding using modern LLVM-based toolchains on Rust platforms [3][4]. Version 3.0.11 of embed-resource includes refinements to these tool selection processes, such as improved support for llvm-rc on gnullvm targets [9][6].
The cc-rs crate does not have a dedicated environment variable named DLLTOOL. Environment variables supported by cc-rs are used to configure compilers and their flags [1][2]. While cc-rs supports target-qualified environment variables for tools like CC, CXX, and AR (e.g., CC_x86_64-unknown-linux-gnu) [1][2], the specific tool dlltool is not typically managed through cc-rs environment variables. Instead, the -Cdlltool option is a rustc codegen option used to specify the path to the dlltool executable, particularly for Windows cross-compilation targets using the raw-dylib feature [3][4][5]. This option is passed to the Rust compiler (e.g., via RUSTFLAGS) rather than the cc-rs crate's configuration mechanism [5]. Projects encountering issues with dlltool may sometimes use RUSTFLAGS to explicitly set the path to the correct dlltool binary to work around host/target environment discrepancies [5].
embed-resource selects the Windows resource compiler through RC_<target> or RC. DLLTOOL_<target> does not configure this path. Set RC_$ToolEnvTarget to the MSYS2 llvm-rc.exe for both default *-windows-gnullvm targets.
🧰 Tools🪛 PSScriptAnalyzer (1.25.0)
[warning] Missing BOM encoding for non-ASCII encoded file 'build-windows.ps1'
(PSUseBOMForUnicodeEncodedFile)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/build-windows.ps1` around lines 1394 - 1409, In the target tool
environment setup around $ToolEnvTarget, set RC_$ToolEnvTarget to the MSYS2
llvm-rc.exe path in both the clang and non-clang branches. Keep the existing
DLLTOOL, CC, CXX, and AR assignments unchanged, using the same target-specific
environment-variable mechanism.
Comprehensive review of new/changed hunks (correctness, security, performance, testing, docs)
Post final summary
What this commit changed
Scoped to .github/workflows/fuzz.yml, hooks/pre-commit, scripts/build-windows.ps1, src/ui/win32_snap.rs, src/ui/window.rs, tests/packaging_metadata.rs.
Prior findings — verified as fixed
Asymmetric Snap Layout retry (my earlier comment on src/ui/window.rs): fixed correctly. try_enable_snap_layout is now shared by all three install attempts (immediate, notify::is-active, and a new one-shot glib::idle_add_local_once fallback for windows that are already active and won't re-fire the notify signal), all gated on one shared Rc<Cell<bool>>. Traced through the closures: header/window are captured as weak refs (downgrade()), so no reference cycles; since GTK's main loop is single-threaded, there's no race between the notify handler and the idle callback over the shared flag.
persist-credentials: false added to the fuzz.yml checkout step (Codacy/zizmor artipacked finding) — correct, no later step needs an authenticated token.
hooks/pre-commit file mode changed to 100755, and the fmt check now also runs cargo fmt --manifest-path fuzz/Cargo.toml --package tributary-fuzz -- --check, closing the gap where fuzz/fuzz_targets/fuzz_dmap.rs wasn't covered by the formatting gate.
$ToolEnvTarget period normalization: .Replace("-", "_").Replace(".", "_") now matches cc-rs's actual target-suffix normalization (confirmed against upstream cc crate docs — it replaces both - and .). tests/packaging_metadata.rs gained a matching test (windows_build_scopes_compiler_tools_to_the_rust_target) that string-checks the exact SetEnvironmentVariable calls and confirms no unscoped $env:CC=-style assignment remains — I grepped the script and confirmed no such unscoped assignment exists anywhere else in the file, so that assertion holds.
Codacy's Arc/WM_NCDESTROY ordering concern (win32_snap.rs): addressed with a clarifying comment rather than a code change, which is correct — RemoveWindowSubclass is called from inside the WM_NCDESTROY handler, after the message has already been delivered, so it cannot suppress delivery. The Arc reclamation below it is unaffected.
CodeRabbit's extraction/testability nitpicks: active_sizing_edges duplication was cleanly extracted into a generic active_axis_edge<T> helper reused for both axes; constrain_interactive_window_pos_to_work_area was decomposed into pure helpers (proposed_window_rect, rect_size, apply_window_pos, cursor_monitor_info, window_rect) that are independently testable. Two new tests cover the ambiguous-bounds-shift case CodeRabbit specifically asked for (active_edges_use_cursor_proximity_when_both_bounds_shift, active_edges_break_equal_distance_ties_toward_start_edges), plus tests for proposed_window_rect's move/no-move (SWP_NOMOVE) branches.
Not addressed (minor, non-blocking)
CodeRabbit's RC_$ToolEnvTarget suggestion (scripts/build-windows.ps1): not applied. The project does depend on embed-resource (Cargo.toml:141, used in build.rs for the tributary binary icon/version resource) targeting *-pc-windows-gnullvm, so the underlying concern is real in principle. However, the script already prepends the MSYS2 bin directory to PATH (line 1392) before these tool variables are set, and embed-resource falls back to a PATH search for llvm-rc when no RC/RC_<target> override is set. Given the PR's reported successful Windows builds (icon/version resource embedding would fail loudly if this were broken), this is likely fine in practice, but I can't verify Win32 resource compilation from this Linux sandbox — worth a quick manual confirmation rather than blocking on it.
constrain_maximized_window_to_work_area still constructs its own MonitorInfo/GetMonitorInfoW call inline rather than sharing the monitor-lookup helper (cursor_monitor_info only serves the interactive-resize path). CodeRabbit flagged this as "Trivial | Low value" originally, and the duplication is small (a handful of lines), so leaving it as-is is a reasonable call.
The pre-existing empty catch { } blocks around taskkillProcess.Kill() / Process.Kill() (build-windows.ps1:375,389) that Codacy flagged are untouched by this PR (confirmed via git diff origin/main...HEAD -- scripts/build-windows.ps1, which only touches lines 1391-1410) — out of scope for this review per the instructions to review only PR-touched code.
Overall
This commit does a solid, targeted job of closing out the substantive feedback from the prior review round (my own comment, Codacy's high-risk memory-safety flag, and CodeRabbit's actionable items) without scope creep. No new correctness, security, or concurrency issues found in the diff. Nothing here blocks merge from my read; the two "not addressed" items above are worth a quick look but aren't clear bugs.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/packaging_metadata.rs`:
- Around line 363-365: Update the MSVC host dependency assertion in the
packaging metadata test to reject generic compiler environment assignments made
through both the `$env:` syntax and `[Environment]::SetEnvironmentVariable`,
while preserving target-specific handling so only unqualified overrides are
rejected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b5500ca-dd25-4b14-a698-939a3f0dcaad
📥 Commits
Reviewing files that changed from the base of the PR and between 5e56cef and 576a36c.
📒 Files selected for processing (6)
.github/workflows/fuzz.yml
hooks/pre-commit
scripts/build-windows.ps1
src/ui/win32_snap.rs
src/ui/window.rs
tests/packaging_metadata.rs
🚧 Files skipped from review as they are similar to previous changes (3)
Gather context (new commit, prior review rounds, CodeRabbit/Codacy findings)
Re-diff since last review (git diff origin/main...HEAD, full PR diff still 1,673/−722 across 15 files, matching PR metadata)
Verify the latest commit actually addresses the outstanding CodeRabbit finding on tests/packaging_metadata.rs
Sanity-check other previously-fixed items haven't regressed
Post final summary
What this commit changed
Per its message, this closes out CodeRabbit's one remaining actionable comment from round 2 (posted 17:08:18, after my prior review): the windows_build_scopes_compiler_tools_to_the_rust_target test in tests/packaging_metadata.rs only rejected the exact string [Environment]::SetEnvironmentVariable("cc", and would stay green even if the script contained a differently-quoted or otherwise-generic override.
Verified as fixed
tests/packaging_metadata.rs:363-372: the assertion now checks three forms — $env:{tool}=, setenvironmentvariable("{tool}",, and setenvironmentvariable('{tool}', (case-folded via compact_build_windows, so quote style and casing can't dodge it) — for all four tool variables (DLLTOOL/CC/CXX/AR). This closes the gap CodeRabbit identified: a generic, non-target-qualified override through either PowerShell syntax would now fail the test.
Cross-checked against the actual script (scripts/build-windows.ps1:1398-1409): every SetEnvironmentVariable call there is target-qualified ("{TOOL}_$ToolEnvTarget"), and no bare $env:cc=/$env:cxx=/etc. assignment exists anywhere else in the file — the test's negative assertions hold against current code, not just in theory.
hooks/pre-commit is 100755 in the index (confirmed via git ls-files --stage) — the executable-bit fix from round 2 held.
.github/workflows/fuzz.yml:29-31 still has persist-credentials: false on the checkout step — the artipacked fix from round 1 held.
Not independently re-verified
src/ui/win32_snap.rs (+342/−~) and src/ui/window.rs (+61/−~) show the same line counts as when I reviewed the prior commit in round 2 (the shared-retry unification, active_axis_edge extraction, and new ambiguous-tie-break tests) — I didn't find evidence those files changed again in this commit beyond what was already reviewed and confirmed. As before, this sandbox is Linux-only and win32_snap.rs is #![cfg(target_os = "windows")], and cargo/build commands required an approval this run that wasn't available, so I could not compile or execute the test suite directly here — static review only.
Still open (non-blocking, carried over from round 2)
RC_$ToolEnvTarget for embed-resource is still not set explicitly. As noted previously, the script already prepends the MSYS2 bin directory to PATH before these variables are set, and embed-resource falls back to a PATH search for llvm-rc/windres when no RC/RC_<target> override exists, so this is likely fine in practice given the PR's reported successful Windows builds — but still worth a manual confirmation since it can't be verified from this Linux sandbox.
Overall
This commit does exactly what it claims: it closes the one remaining actionable review comment without touching anything else. No new correctness, security, or test-quality issues found in this delta. Nothing here blocks merge.
active_axis_edge returns None when the axis size does not change. The suite covers corner drags, ambiguous shifts, and ties, but not this branch. A single-axis drag test would lock the behavior that a translated axis is never clamped.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/win32_snap.rs` around lines 1057 - 1079, Add a unit test alongside
active_edges_preserve_the_fixed_sides_of_a_corner_drag covering a proposed
rectangle whose horizontal axis only translates while the vertical axis changes.
Assert that active_edges_ignore_an_axis_that_only_translates yields horizontal:
None and vertical: Some(VerticalEdge::Bottom), confirming translated axes are
not clamped.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/ui/win32_snap.rs`:
- Around line 1057-1079: Add a unit test alongside
active_edges_preserve_the_fixed_sides_of_a_corner_drag covering a proposed
rectangle whose horizontal axis only translates while the vertical axis changes.
Assert that active_edges_ignore_an_axis_that_only_translates yields horizontal:
None and vertical: Some(VerticalEdge::Bottom), confirming translated axes are
not clamped.
ℹ️ Review info⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c1b5ea84-5871-4f87-ae70-296c062b24da
📥 Commits
Reviewing files that changed from the base of the PR and between 576a36c and 50b9144.
📒 Files selected for processing (6)
.github/workflows/fuzz.yml
hooks/pre-commit
scripts/build-windows.ps1
src/ui/win32_snap.rs
src/ui/window.rs
tests/packaging_metadata.rs
🚧 Files skipped from review as they are similar to previous changes (4)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary
HTMAXBUTTONhandling for Windows 11 Snap Layouts while preserving maximize/restore clicksRoot cause and impact
GTK 4.22 performs CSD resize drags with pointer capture and repeated
SetWindowPoscalls. The earlier workaround cappedMINMAXINFO.max_track_sizeto one monitor's work area, preventing a restored window from reaching the full width of a high-DPI portrait monitor. Removing that cap restored width but allowed the manually dragged outer surface, including GTK's transparent CSD shadow, into the taskbar strip.The new subclass preserves GTK's tracking limits, adjusts only maximized bounds in
WM_GETMINMAXINFO, and constrains only the active edge during GTK's capturedWM_WINDOWPOSCHANGINGresize loop. It does not forceWS_THICKFRAMEor other frame styles.Validation
.\scripts\build-windows.ps1 -Fmt.\scripts\build-windows.ps1 -Check.\scripts\build-windows.ps1 -Test— 20 library, 1,707 application, and 26 packaging tests passed.\scripts\build-windows.ps1 -Clippy— strict debug/all-target and release passesgit diff --checkSummary by CodeRabbit
New Features
Changes
Bug Fixes