Conversation
There was a problem hiding this comment.
Pull request overview
This pull request updates the Microsoft account login dialog to improve the user experience by adding QR code login support and refactoring the authentication flow with a cleaner state-based architecture.
Changes:
- Added QR code generation capability using the nayuki QR code library for device-based authentication
- Refactored login flow using a sealed interface pattern with distinct states (Init, StartAuthorizationCodeLogin, WaitForOpenBrowser, StartDeviceCodeLogin, WaitForScanQrCode, LoginFailed)
- Simplified UI layout by consolidating authentication methods into a single streamlined interface with dynamic state transitions
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| gradle/libs.versions.toml | Added nayuki QR code generator library dependency (version 1.8.0) |
| HMCL/build.gradle.kts | Imported nayuki-qrcodegen library for QR code generation |
| HMCLCore/src/main/java/org/jackhuang/hmcl/util/StringUtils.java | Added XML attribute escaping utility function |
| HMCL/src/main/java/org/jackhuang/hmcl/util/QrCodeUtils.java | New utility class for converting QR codes to SVG path format |
| HMCL/src/main/java/org/jackhuang/hmcl/ui/construct/JFXHyperlink.java | Added constructor accepting text and external link |
| HMCL/src/main/java/org/jackhuang/hmcl/ui/construct/HintPane.java | Added overload for setSegment with custom hyperlink action |
| HMCL/src/main/java/org/jackhuang/hmcl/ui/account/MicrosoftAccountLoginPane.java | Major refactor with state-based architecture and QR code support |
| HMCL/src/main/java/org/jackhuang/hmcl/ui/account/CreateAccountPane.java | Updated to use simplified MicrosoftAccountLoginPane |
| HMCL/src/main/resources/assets/lang/*.properties | Updated localization strings for new login flow |
| HMCL/src/main/resources/assets/css/root.css | Updated CSS for new dialog layout and code box styling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
HMCLCore/src/main/java/org/jackhuang/hmcl/util/StringUtils.java
Outdated
Show resolved
Hide resolved
HMCL/src/main/java/org/jackhuang/hmcl/ui/account/MicrosoftAccountLoginPane.java
Show resolved
Hide resolved
HMCL/src/main/java/org/jackhuang/hmcl/ui/account/MicrosoftAccountLoginPane.java
Outdated
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| deviceHint.setSegment(i18n("account.methods.microsoft.methods.device.hint", | ||
| StringUtils.escapeXmlAttribute(wait.verificationUri()), | ||
| wait.verificationUri(), | ||
| wait.userCode() |
There was a problem hiding this comment.
The userCode parameter is not being escaped before being passed to the i18n function, which then formats it into an HTML string. If the userCode contains special XML/HTML characters like "&", "<", ">", etc., they will not be properly escaped and could break the HTML rendering or potentially cause security issues. Consider escaping the userCode parameter similar to how verificationUri is escaped.
| wait.userCode() | |
| StringUtils.escapeXmlAttribute(wait.userCode()) |
|
|
||
| rootContainer.getChildren().addAll(deviceHint, new Group(qrCode), codeBox); | ||
| } else if (currentStep instanceof Step.LoginFailed failed) { | ||
| loginButtonSpinner.setLoading(true); |
There was a problem hiding this comment.
In the LoginFailed state, the spinner is set to loading (true) when it should likely be set to not loading (false). When a login has failed, the user should be able to retry or cancel, so the login button should not be in a loading state. This is inconsistent with other error handling patterns where the spinner is stopped after an error occurs.
| loginButtonSpinner.setLoading(true); | |
| loginButtonSpinner.setLoading(false); |
| } else if (currentStep instanceof Step.LoginFailed failed) { | ||
| loginButtonSpinner.setLoading(true); | ||
|
|
||
| HintPane errHintPane = new HintPane(MessageDialogPane.MessageType.ERROR); | ||
| errHintPane.setText(failed.message()); | ||
| rootContainer.getChildren().add(errHintPane); | ||
| } |
There was a problem hiding this comment.
When the login process fails, users may want to retry the login. However, there is no button action set for btnLogin in the LoginFailed state, which means clicking the login button won't trigger any retry logic. Consider adding a retry mechanism by setting btnLogin.setOnAction to restart the login flow (e.g., transitioning back to Step.Init or Step.StartAuthorizationCodeLogin).
| ******************************************************************************/ | ||
|
|
||
| .microsoft-login-dialog:bodyonly { | ||
| -fx-padding: 0px 0px 0px 0px; |
There was a problem hiding this comment.
The CSS padding value "0px 0px 0px 0px" is unnecessarily verbose. It can be simplified to just "0" or "0px" which is more concise and follows common CSS best practices. All four values being the same can be represented with a single value.
| -fx-padding: 0px 0px 0px 0px; | |
| -fx-padding: 0; |
2 participants
No description provided.