Merge pull request #2886 from socksprox/fix/launcher-forget-offline-name

🤖 Agent: read the diff at 5096b4110: launcher offline names: Forget also deletes the name's skin and cape files, async skin picks bail if the name was forgotten meanwhile, account rows load-mutate-save, and "New name" rolls a fresh PlayerNNNNN into every game home. Tests included. verify and launcher green; no harness reaches launcher code. Queued for the merger, which waits for this head's parity run.

docs/launcher-commands.md

@@ -149,7 +149,7 @@ MCP: `account_offline`.
 
 ## `horizon-launcher account forget-offline`
 
-Forget one of your offline names.
+Forget one of your offline names; its skin and cape go with it.
 
     horizon-launcher account forget-offline <NAME>
 

launcher/README.md

@@ -136,8 +136,9 @@ The opening, then Play ([clip](docs/opening.webm)):
   made up (`horizon-player.txt`). Picking one plays as it from the next Play. Offline names play
   only while an account that owns the game is signed in ([Microsoft accounts](#microsoft-accounts));
   signing the last one out keeps them, unplayable until one is back. Settings > Account
-  adds offline names (3 to 16 letters, digits or `_`; the game gets `--name`) and forgets them;
-  the Console's Share log takes them out of the log too. `horizon-launcher account offline
+  adds offline names (3 to 16 letters, digits or `_`; the game gets `--name`) and forgets them,
+  skin and cape files included, and rolls the game's made-up name for a new one; the Console's
+  Share log takes the names out of the log too. `horizon-launcher account offline
   [NAME]`, `account select <name>` (an account or an offline name), `account forget-offline
   <name>`.
 - **Accent and Home background**: on top of Dark, Light or System, Settings > Launcher picks an

launcher/src/account.rs

@@ -907,14 +907,16 @@ impl Accounts {
         Ok(name.to_string())
     }
 
-    /// Forgets an offline name; if it was playing, the game's own name plays.
-    pub fn remove_offline(&mut self, name: &str) -> Result<(), String> {
+    /// Forgets an offline name: its look and its skin and cape files (`skins`) go with it; if
+    /// it was playing, the game's own name plays.
+    pub fn remove_offline(&mut self, name: &str, skins_dir: &Path) -> Result<(), String> {
         let before = self.offline.len();
         self.offline.retain(|n| !n.eq_ignore_ascii_case(name));
         self.looks.remove(&name.to_ascii_lowercase());
         if self.offline.len() == before {
             return Err(format!("No offline name {name}."));
         }
+        crate::skins::forget(skins_dir, name);
         if self
             .offline_selected
             .as_ref()

launcher/src/account/tests.rs

@@ -627,9 +627,26 @@ fn offline_names_switch_with_accounts() {
     assert!(a.play_as(&store, Player::Microsoft("zzz".into())).is_err());
     a.play_as(&store, Player::Offline("Builder_1".into()))
         .unwrap();
-    a.remove_offline("BUILDER_1").unwrap();
+    // Forgetting drops the look and the name's skin and cape files too.
+    let dir = std::env::temp_dir().join(format!("horizon-accounts-{}", std::process::id()));
+    let _ = std::fs::remove_dir_all(&dir);
+    std::fs::create_dir_all(&dir).unwrap();
+    let (skin, cape) = crate::skins::files(&dir, "builder_1");
+    std::fs::write(&skin, b"skin").unwrap();
+    std::fs::write(&cape, b"cape").unwrap();
+    a.looks.insert(
+        "builder_1".into(),
+        crate::skins::Look {
+            skin: true,
+            cape: true,
+            ..crate::skins::Look::default()
+        },
+    );
+    a.remove_offline("BUILDER_1", &dir).unwrap();
     assert_eq!(a.player(), Player::GameName);
-    assert!(a.remove_offline("Builder_1").is_err());
+    assert!(a.looks.is_empty() && !skin.exists() && !cape.exists());
+    let _ = std::fs::remove_dir_all(&dir);
+    assert!(a.remove_offline("Builder_1", &dir).is_err());
     // An accounts.json from before offline names reads as it was, and writes nothing new.
     let old: Accounts = serde_json::from_str(r#"{"accounts":[],"selected":null}"#).unwrap();
     assert_eq!(old.player(), Player::GameName);

launcher/src/cli.rs

@@ -103,7 +103,7 @@ enum AccountCmd {
     /// as NAME (kept to switch back to; 3 to 16 letters, digits or _), or as the name the game
     /// made up
     Offline { name: Option<String> },
-    /// Forget one of your offline names
+    /// Forget one of your offline names; its skin and cape go with it
     ForgetOffline { name: String },
     /// Show or set an offline name's skin and cape: a 64x64 (or 64x32) skin PNG, a 64x32 cape
     /// PNG, the model. Kept by the launcher and given to the game when that name plays
@@ -1580,7 +1580,7 @@ fn account_command(action: AccountCmd, as_json: bool) -> Result<(), String> {
             }
         }
         AccountCmd::ForgetOffline { name } => {
-            accounts.remove_offline(&name)?;
+            accounts.remove_offline(&name, &horizon_launcher::skins::dir())?;
             accounts.save()?;
             println!("Forgot the offline name {name}.");
         }

launcher/src/instances.rs

@@ -491,6 +491,34 @@ fn follow_player(game_home: &Path, home: &Path) {
     }
 }
 
+/// A fresh made-up name everywhere the game keeps one: `horizon-player.txt` is per game
+/// folder (`PLAYER_FILES`), so the same new name goes to the default home and every
+/// instance's — the player stays one player in its worlds. The file's `name:` line and the
+/// `Player` + five digits scheme are horizon-client's (`offline_name.rs`).
+pub fn new_player_name() -> Result<String, String> {
+    new_player_name_in(&backend::game_home(), &backend::data_dir())
+}
+
+fn new_player_name_in(game_home: &Path, data: &Path) -> Result<String, String> {
+    let nanos = std::time::SystemTime::now()
+        .duration_since(std::time::UNIX_EPOCH)
+        .map_or(0, |d| d.as_nanos() as u64);
+    let name = format!(
+        "Player{:05}",
+        (nanos ^ u64::from(std::process::id()).rotate_left(32)) % 100_000
+    );
+    for instance in list_in(game_home, data) {
+        if instance.home.is_dir() {
+            fs::write(
+                instance.home.join("horizon-player.txt"),
+                format!("name:{name}\n"),
+            )
+            .map_err(|e| format!("{}: {e}", instance.home.display()))?;
+        }
+    }
+    Ok(name)
+}
+
 #[cfg(test)]
 #[path = "instances/tests.rs"]
 mod tests;

launcher/src/instances/tests.rs

@@ -67,6 +67,33 @@ fn player_home(home: &Path) {
     );
 }
 
+#[test]
+fn a_new_player_name_replaces_the_kept_one_everywhere() {
+    let root = scratch("newname");
+    let (home, data) = (root.join("horizon"), root.join("launcher"));
+    player_home(&home);
+    // An own instance that has the old name, and one with no file yet.
+    put(
+        &home.join("instances/one/horizon-player.txt"),
+        "name:Player123\n",
+    );
+    fs::create_dir_all(home.join("instances/two")).unwrap();
+    let name = new_player_name_in(&home, &data).unwrap();
+    assert_ne!(name, "Player123");
+    assert!(name.starts_with("Player") && name.len() == 11);
+    let kept = |home: &Path| fs::read_to_string(home.join("horizon-player.txt")).unwrap();
+    for h in [
+        &home,
+        &home.join("instances/one"),
+        &home.join("instances/two"),
+        &home.join("ports/small-pack/home"),
+        &home.join("horizon-packs/cozy/home"),
+    ] {
+        assert_eq!(kept(h), format!("name:{name}\n"), "{}", h.display());
+    }
+    let _ = fs::remove_dir_all(&root);
+}
+
 #[test]
 fn existing_worlds_stay_where_they_are_and_every_folder_becomes_an_instance() {
     let root = scratch("migrate");

launcher/src/main.rs

@@ -501,8 +501,10 @@ impl Launcher {
     }
 
     fn go(&mut self, page: Page) {
-        // The game may have made up a name since: read it again (a small file, once a switch).
+        // The game may have made up a name since, and the CLI or another window may have
+        // changed the accounts: read them again (small files, once a switch).
         self.offline_name = offline_name();
+        self.accounts = Accounts::load();
         if page != self.page {
             self.page = page;
             self.switches += 1;
@@ -1791,6 +1793,10 @@ impl Launcher {
             )
             .on_click(cx.listener(|this, _: &ClickEvent, _, cx| {
                 this.account_menu = !this.account_menu;
+                if this.account_menu {
+                    // Who can play may have changed elsewhere since the window opened.
+                    this.accounts = Accounts::load();
+                }
                 cx.notify();
             }));
         let account = div()
@@ -3400,8 +3406,11 @@ impl Launcher {
                 actions = actions.child(
                     button(format!("offline-{i}"), "Play offline", Kind::Ghost, &t).on_click(
                         cx.listener(|this, _: &ClickEvent, _, cx| {
-                            this.accounts.selected = None;
-                            let _ = this.accounts.save();
+                            let mut accounts = Accounts::load();
+                            accounts.selected = None;
+                            if accounts.save().is_ok() {
+                                this.accounts = accounts;
+                            }
                             cx.notify();
                         }),
                     ),
@@ -3411,8 +3420,7 @@ impl Launcher {
                 actions = actions.child(
                     button(format!("select-{i}"), "Play as", Kind::Secondary, &t).on_click(
                         cx.listener(move |this, _: &ClickEvent, _, cx| {
-                            this.accounts.selected = Some(uuid.clone());
-                            let _ = this.accounts.save();
+                            this.play_as(account::Player::Microsoft(uuid.clone()));
                             cx.notify();
                         }),
                     ),
@@ -3421,12 +3429,15 @@ impl Launcher {
             actions = actions.child(
                 button(format!("sign-out-{i}"), "Sign out", Kind::Ghost, &t).on_click(cx.listener(
                     move |this, _: &ClickEvent, _, cx| {
-                        let store = account::store();
-                        if let Err(e) = this.accounts.remove(store, &uuid) {
-                            this.sign_in_state = Some(SignInState::Done(Err(e)));
+                        let mut accounts = Accounts::load();
+                        match accounts
+                            .remove(account::store(), &uuid)
+                            .and_then(|()| accounts.save())
+                        {
+                            Ok(()) => this.accounts = accounts,
+                            Err(e) => this.sign_in_state = Some(SignInState::Done(Err(e))),
                         }
-                        let _ = this.accounts.save();
-                        this.owner = Some(this.accounts.has_owner(store));
+                        this.owner = Some(this.accounts.has_owner(account::store()));
                         cx.notify();
                     },
                 )),

launcher/src/skin_page.rs

@@ -38,6 +38,14 @@ impl Launcher {
             return Ok(());
         };
         let mut accounts = Accounts::load();
+        // The name may have been forgotten while a file dialog was open: keep nothing for it.
+        if !accounts
+            .offline
+            .iter()
+            .any(|n| n.eq_ignore_ascii_case(&name))
+        {
+            return Ok(());
+        }
         let mut look = accounts.look(&name);
         change(&mut look);
         accounts.looks.insert(name.to_ascii_lowercase(), look);
@@ -75,6 +83,14 @@ impl Launcher {
         let Some(name) = self.skin.as_ref().map(|p| p.name.clone()) else {
             return;
         };
+        // The name may have been forgotten while the file dialog was open.
+        if !Accounts::load()
+            .offline
+            .iter()
+            .any(|n| n.eq_ignore_ascii_case(&name))
+        {
+            return;
+        }
         let done = skins::keep(&skins::dir(), &name, &path, cape).and_then(|()| {
             self.save_look(|l| match cape {
                 false => l.skin = true,

launcher/src/skins.rs

@@ -157,6 +157,13 @@ pub fn keep(dir: &Path, name: &str, file: &Path, cape: bool) -> Result<(), Strin
     std::fs::write(&to, bytes).map_err(|e| format!("{}: {e}", to.display()))
 }
 
+/// Drops a forgotten name's files (`Accounts::remove_offline`); missing files are fine.
+pub fn forget(dir: &Path, name: &str) {
+    let (skin, cape) = files(dir, name);
+    let _ = std::fs::remove_file(skin);
+    let _ = std::fs::remove_file(cape);
+}
+
 /// The variables the game gets for `name`'s look.
 pub fn env(dir: &Path, name: &str, look: &Look) -> Vec<(&'static str, String)> {
     let (skin, cape) = files(dir, name);

launcher/src/skins/tests.rs

@@ -102,3 +102,23 @@ fn a_kept_look_reaches_the_game_only_with_its_files() {
     );
     std::fs::remove_dir_all(&root).unwrap();
 }
+
+#[test]
+fn forgetting_a_name_drops_its_files() {
+    let root = std::env::temp_dir().join(format!("horizon-skins-forget-{}", std::process::id()));
+    let _ = std::fs::remove_dir_all(&root);
+    std::fs::create_dir_all(&root).unwrap();
+    let (src, cape_src) = (root.join("alex.png"), root.join("cape.png"));
+    std::fs::write(&src, png(&mapped(64))).unwrap();
+    std::fs::write(&cape_src, png(&mapped(32))).unwrap();
+    let dir = root.join("skins");
+    keep(&dir, "Alex", &src, false).unwrap();
+    keep(&dir, "Alex", &cape_src, true).unwrap();
+    let (skin, cape) = files(&dir, "Alex");
+    assert!(skin.is_file() && cape.is_file());
+    forget(&dir, "ALEX");
+    assert!(!skin.exists() && !cape.exists());
+    // A name that never had files is forgotten all the same.
+    forget(&dir, "Alex");
+    std::fs::remove_dir_all(&root).unwrap();
+}

launcher/src/switch_page.rs

@@ -185,7 +185,11 @@ impl Launcher {
                 button(format!("offline-forget-{i}"), "Forget", Kind::Ghost, &t).on_click(
                     cx.listener(move |this, _: &ClickEvent, _, cx| {
                         let mut accounts = Accounts::load();
-                        if accounts.remove_offline(&forget).is_ok() && accounts.save().is_ok() {
+                        if accounts
+                            .remove_offline(&forget, &horizon_launcher::skins::dir())
+                            .is_ok()
+                            && accounts.save().is_ok()
+                        {
                             this.accounts = accounts;
                         }
                         cx.notify();
@@ -210,6 +214,37 @@ impl Launcher {
             ));
         }
         rows = rows.children(self.skin_panel(cx));
+        // The name the game made up can't be forgotten — it's what an offline player is —
+        // only rolled for a new one.
+        if let Some(made_up) = self.offline_name.clone() {
+            let plays = now == Player::GameName;
+            n += 1;
+            rows = rows.child(index_row(
+                n,
+                made_up,
+                Some(
+                    div()
+                        .child(tr(if plays {
+                            "Offline name · the game's own · plays"
+                        } else {
+                            "Offline name · the game's own"
+                        }))
+                        .into_any_element(),
+                ),
+                Some(
+                    button("game-name-new", "New name", Kind::Ghost, &t)
+                        .on_click(cx.listener(|this, _: &ClickEvent, _, cx| {
+                            match instances::new_player_name() {
+                                Ok(name) => this.offline_name = Some(name),
+                                Err(e) => this.sign_in_state = Some(SignInState::Done(Err(e))),
+                            }
+                            cx.notify();
+                        }))
+                        .into_any_element(),
+                ),
+                &t,
+            ));
+        }
         n += 1;
         let add = div()
             .flex()