Skip to content

Update Taskbar Virtual Desktop Switcher to v1.6#4516

Open
sb4ssman wants to merge 1 commit into
ramensoftware:mainfrom
sb4ssman:update-taskbar-vd-switcher
Open

Update Taskbar Virtual Desktop Switcher to v1.6#4516
sb4ssman wants to merge 1 commit into
ramensoftware:mainfrom
sb4ssman:update-taskbar-vd-switcher

Conversation

@sb4ssman

Copy link
Copy Markdown
Contributor

Summary

Updates taskbar-vd-switcher from v1.5 to v1.6.

  • Canonical rubric setting names (gridRows/gridColumns, activeBackgroundColor/inactiveBackgroundColor, opacity, groupPaddingLeft/groupPaddingRight, groupOffsetY) with AliasedStr/AliasedInt backward-compat lambdas — old setting names still work
  • "Master button" renamed to "Task View button" throughout YAML $name fields and readme
  • contentOffsetX/Y removed (was a rubric misapplication; not applicable to text buttons)
  • Expanded in-mod readme with full screenshot gallery and feature summary
  • Full settings table

Supersedes PR #4484 (which was incorrectly submitted under a new mod id).

- Canonical rubric setting names (gridRows/gridColumns, activeBackgroundColor,
  inactiveBackgroundColor, opacity, groupPaddingLeft/Right, groupOffsetY) with
  AliasedStr/AliasedInt backward-compat lambdas; old names still work
- "Master button" renamed to "Task View button" throughout YAML and readme
- contentOffsetX/Y removed (rubric misapplication; not applicable to text buttons)
- Expanded readme with full screenshot gallery and feature list
- Full settings table

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@sb4ssman sb4ssman force-pushed the update-taskbar-vd-switcher branch from d40ddb4 to 673fe2f Compare June 21, 2026 18:19
@m417z m417z closed this Jun 22, 2026
@m417z m417z reopened this Jun 22, 2026
@m417z

m417z commented Jun 22, 2026

Copy link
Copy Markdown
Member

Submission review

Note: This review was done by Claude, and then refined manually. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding.

Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them.


The backward-compatibility shim for the renamed settings — the headline feature of this update ("old setting names still work") — does not actually work for the integer settings, and works for only one of the two string settings.

The cause is the Wh_GetIntSetting / Wh_GetStringSetting contract. Their variadic arguments are printf-style format specifiers for the setting name (used for indexed settings like key[%d]), not a default value — for a setting that has no value the functions return 0 / L"", full stop (wiki, Wh_GetIntSetting). The default that the rest of the file passes as a second argument (e.g. Wh_GetIntSetting(L"buttonWidth", 20)) only appears to work because the real default comes from the settings block (Each setting is defined by a name and a default value. The default value is set when the mod is installed), and authors happen to repeat the same number in both places — the 20 is silently consumed as a format arg and ignored.

That breaks the alias detection:

auto AliasedInt = [](const wchar_t* canonical, const wchar_t* legacy, int fallback) {
    constexpr int kUnset = -2147483647;
    int value = Wh_GetIntSetting(canonical, kUnset);  // kUnset is a format arg → ignored
    if (value == kUnset) {                            // never true
        value = Wh_GetIntSetting(legacy, fallback);   // dead code
    }
    return value;
};

gridRows, gridColumns, opacity, groupPaddingLeft, groupPaddingRight, and groupOffsetY are all declared in the settings block, so each always returns its block default (or the user's new value) — never kUnset. The value == kUnset branch is unreachable, so the legacy names (buttonRows, buttonColumns, buttonOpacity, paddingLeft, paddingRight, gridVerticalOffset) are never read.

AliasedStr has the same flaw via a different path: it falls back to the legacy name only when the canonical value is empty. inactiveBackgroundColor has an empty block default, so its alias to inactiveColor does work — but activeBackgroundColor defaults to #4488FF (non-empty), so the legacy activeColor is never consulted.

User impact: an existing user who had customized rows/columns, opacity, padding, vertical offset, or active color in v1.5 will silently lose those values on update — they reset to the new defaults. This is the opposite of what the PR claims.

There's no clean way to fix the shim, because (a) mods can't write user settings (there is no Wh_SetIntSetting), and (b) a declared setting at its default is indistinguishable from a setting the user explicitly set to that default, so you can't reliably detect "the user hasn't touched the canonical name." Given that, the realistic options are:

  • Recommended: keep the legacy key names. Revert the YAML key renames and the corresponding code reads back to the v1.5 names (buttonRows, buttonColumns, activeColor, inactiveColor, buttonOpacity, paddingLeft, paddingRight, gridVerticalOffset) and drop AliasedInt/AliasedStr entirely. The cosmetic benefit of "canonical rubric names" isn't worth silently resetting existing users' configuration. You can still relabel them in the UI freely via $name — only the key needs to stay stable.
  • If the rename really must happen, then accept that updating resets those settings to defaults, remove the non-functional shim, and say so plainly in the README / PR description (the current "old setting names still work" wording is inaccurate). Human note: I don't think there's a reason renames "really must happen". I think it's best to keep them unchanged to avoid confusion, as the migration can have edge cases one way or another.

Since this is AI-assisted (commit co-authored by Claude Sonnet 4.6), this is worth calling out specifically: the sentinel-default pattern is a plausible-but-wrong assumption about the Wh_Get*Setting API that compiles and runs cleanly but never does what it looks like it does.

Optional improvements

Minor polish — none of this affects users, so it's your call.

  • The ModSettings struct still initializes int buttonOpacity = 70; (line 355), while the settings block, the README table, and the LoadSettings call all use 100. The struct initializer is dead (it's overwritten by LoadSettings before first use), but it's confusing — make it 100 for consistency.
  • In AliasedStr, the legacy branch is std::wstring r = p ? p : fallback;. Since Wh_GetStringSetting never returns NULL (it returns L""), p is always truthy, so fallback is dead code — r becomes p (the legacy value, or L"" if unset) regardless. It happens not to matter today (the only path that reaches it is inactiveBackgroundColor, whose fallback is L"" anyway), but if you keep this helper, the empty-string check would need to mirror the canonical one. (Moot if you take the "keep legacy names" route above and delete the helper.)
  • The README's H1 is now # Virtual Desktop Switcher, but the mod's @name is still Taskbar Virtual Desktop Switcher. Harmless, but the two reading differently is a small inconsistency.

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