-
Notifications
You must be signed in to change notification settings - Fork 50
feat(account): support changing textures for all types of account (3rd+microsoft) #1137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR extends texture (skin/cape) management capabilities from offline accounts to all account types (3rd-party and Microsoft accounts), unifying the interface for managing player appearances across different authentication methods.
Key changes:
- Renamed commands from
update_player_skin_offline_*toupdate_player_texture_*to reflect broader account type support - Introduced a
TextureOperationtrait for polymorphic texture handling across different player types - Added API integration for uploading/deleting skins and capes to Microsoft and AuthLib servers
- Unified the frontend modal interface by removing the view-only
ViewSkinModalin favor of a singleManageSkinModal
Reviewed Changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/services/account.ts |
Renamed service methods to be player-type agnostic |
src/locales/zh-Hans.json |
Updated Chinese translations for texture operations and removed view-only modal strings |
src/locales/en.json |
Updated English translations for texture operations and removed view-only modal strings |
src/components/player-menu.tsx |
Unified modal usage to single ManageSkinModal for all player types |
src/components/modals/view-skin-modal.tsx |
Deleted view-only modal component |
src/components/modals/manage-skin-modal.tsx |
Enhanced to support all player types with Microsoft cape upload warning |
src-tauri/src/lib.rs |
Updated command registration with renamed handlers |
src-tauri/src/account/models.rs |
Changed method to immutable reference and added new error type |
src-tauri/src/account/helpers/texture.rs |
Introduced TextureOperation trait and helper functions |
src-tauri/src/account/helpers/offline/mod.rs |
Extracted login function to separate module |
src-tauri/src/account/helpers/offline/login.rs |
New module containing offline login logic |
src-tauri/src/account/helpers/mod.rs |
Renamed skin helper module to texture |
src-tauri/src/account/helpers/microsoft/texture.rs |
New implementation of TextureOperation for Microsoft accounts |
src-tauri/src/account/helpers/microsoft/oauth.rs |
Refactored to use TextureOperation trait |
src-tauri/src/account/helpers/microsoft/mod.rs |
Added texture module |
src-tauri/src/account/helpers/authlib_injector/texture.rs |
New implementation of TextureOperation for 3rd-party accounts |
src-tauri/src/account/helpers/authlib_injector/mod.rs |
Added texture module |
src-tauri/src/account/helpers/authlib_injector/common.rs |
Refactored to use TextureOperation trait |
src-tauri/src/account/commands.rs |
Rewrote texture update commands to support all player types |
src-tauri/Cargo.toml |
Added multipart feature for HTTP uploads |
src-tauri/Cargo.lock |
Added mime_guess dependency for multipart support |
Comments suppressed due to low confidence (1)
src-tauri/src/account/helpers/texture.rs:71
- The error mapping to
TextureFormatIncorrectis misleading here. The error occurs during file path retrieval, not texture format validation. Consider using a more appropriate error type or creating a new one likeResourceNotFound.
| pub fn get_player_by_id(&self, id: String) -> Option<&PlayerInfo> { | ||
| self.players.iter().find(|player| player.id == id) |
Copilot
AI
Nov 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The id parameter should be &str instead of String to avoid unnecessary allocations. The method only needs to read the ID for comparison, not take ownership.
| pub fn get_player_by_id(&self, id: String) -> Option<&PlayerInfo> { | |
| self.players.iter().find(|player| player.id == id) | |
| pub fn get_player_by_id(&self, id: &str) -> Option<&PlayerInfo> { | |
| self.players.iter().find(|player| player.id.as_str() == id) |
| }) => { | ||
| let playerId = player.id; | ||
| let skin = player.textures.find((t) => t.textureType === TextureType.Skin); | ||
| let cape = player.textures.find((t) => t.textureType === TextureType.Cape); |
Copilot
AI
Nov 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use const instead of let for these variables since they are never reassigned. This makes the code's intent clearer and prevents accidental reassignment.
| let cape = player.textures.find((t) => t.textureType === TextureType.Cape); | |
| const cape = player.textures.find((t) => t.textureType === TextureType.Cape); |
| async fn upload_cape( | ||
| _app: &AppHandle, | ||
| _player: &PlayerInfo, | ||
| _file_path: &Path, | ||
| ) -> SJMCLResult<MinecraftProfile> { | ||
| Err(AccountError::NoTextureApi.into()) | ||
| } |
Copilot
AI
Nov 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment explaining why cape upload is not supported for Microsoft accounts (e.g., 'Microsoft API does not provide a public endpoint for cape uploads').
| match player.player_type { | ||
| PlayerType::Offline => {} | ||
| PlayerType::ThirdParty => { | ||
| let _ = AuthlibInjectorTextureOperation::delete_cape(&app, &player).await; |
Copilot
AI
Nov 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silently ignoring cape deletion errors with let _ could hide important issues. Consider logging the error or adding a comment explaining why failures are acceptable in this context.
| .await?; | ||
| } | ||
| PlayerType::Microsoft => { | ||
| let _ = MicrosoftTextureOperation::delete_cape(&app, &player).await; |
Copilot
AI
Nov 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silently ignoring cape deletion errors with let _ could hide important issues. Consider logging the error or adding a comment explaining why failures are acceptable in this context.
Checklist
This PR is a ..
Related Issues
Description
Additional Context