sync: from linuxdeepin/dde-session-shell #448
Open
Conversation
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
Synchronize source files from linuxdeepin/dde-session-shell. Source-pull-request: linuxdeepin/dde-session-shell#30
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: deepin-ci-robot The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Reviewer's GuideThis PR brings upstream synchronizations to dde-session-shell, adding extensible plugin support (including a new PasswordExtendLoginType), a SignalBridge for extra-info propagation, a new gesture-login plugin suite, a MFASequenceControl for sequencing multi-factor auth, and UI/workflow improvements across authentication widgets and session management. Sequence diagram for extra info propagation via SignalBridgesequenceDiagram
participant AuthPassword
participant AssistLoginWidget
participant SignalBridge
participant LockWorker
participant DeepinAuthFramework
AuthPassword->>AssistLoginWidget: User triggers action (e.g., password + extra info)
AssistLoginWidget->>SignalBridge: emit requestSendExtraInfo(info)
SignalBridge->>LockWorker: requestSendExtraInfo(account, authType, info)
LockWorker->>DeepinAuthFramework: sendExtraInfo(account, authType, info)
DeepinAuthFramework-->>LockWorker: (DBus call to SetExtraInfo)
ER diagram for plugin types and relationshipserDiagram
PLUGIN_BASE ||--o{ LOGIN_PLUGIN : provides
PLUGIN_BASE ||--o{ TRAY_PLUGIN : provides
LOGIN_PLUGIN ||--o{ ASSIST_LOGIN_WIDGET : used_by
LOGIN_PLUGIN ||--o{ PASSWORD_EXTEND_LOGIN_TYPE : subtype
LOGIN_PLUGIN ||--o{ GESTURE_LOGIN_PLUGIN : subtype
PLUGIN_MANAGER ||--o{ LOGIN_PLUGIN : manages
PLUGIN_MANAGER ||--o{ TRAY_PLUGIN : manages
Class diagram for new and updated authentication plugin typesclassDiagram
class PluginBase {
+ModuleType type()
}
class LoginPlugin {
+authStateChanged(type, state, prompt)
+readyToAuth()
+updateConfig()
+notifyAuthFactorsChanged(authFactors)
}
class PluginManager {
+getFullManagedLoginPlugin()
+getAssistloginPlugin()
+getFirstLoginPlugin(type)
+getLoginPlugin(authType)
+broadcastAuthFactors(authFactors)
+getProperties(messageObj)
+parseMessage(message)
}
class AssistLoginWidget {
+setModule(LoginPlugin*)
+setAuthState(state, result)
+pluginType()
+readyToAuth()
+updateVisible()
+extraInfo()
}
class AuthCustom {
+setModule(LoginPlugin*)
+setAuthState(state, result)
+setLimitsInfo(limitsInfo)
+notifyAuthState(authType, state)
+updateUnlockPrompt()
}
class AuthPassword {
+assistLoginWidget()
+onReadyToAuthChanged(ready)
}
PluginBase <|-- LoginPlugin
PluginManager o-- LoginPlugin
AssistLoginWidget o-- LoginPlugin
AuthCustom o-- LoginPlugin
AuthPassword o-- AssistLoginWidget
Class diagram for new gesture login plugin componentsclassDiagram
class WayPointModel {
+instance()
+currentPoints()
+isGestureEnabled()
+getToken()
+setUserName(name)
+setCurrentMode(mode)
+onPathArrived(index)
+onUserInputDone()
+clearPath()
}
class GestureDialog {
+GestureDialog(ResetPatternController*, QWidget*)
+onInputFinished()
}
class Manager {
+start()
+exit(retCode)
+free()
}
class ResetPatternController
class ModuleWidget
class GesturePannel
GestureDialog o-- ResetPatternController
GestureDialog o-- ModuleWidget
ModuleWidget o-- GesturePannel
Manager o-- GestureDialog
Class diagram for MFASequenceControl and related MFAWidget changesclassDiagram
class MFASequenceControl {
+instance()
+setAuthType(type)
+onAuthStatusChanged(type, state, message)
+insertIsolateAuthWidget(type, widget)
+currentAuthWidget()
+currentAuthType()
}
class MFAWidget {
+setAuthType(type)
+setAuthState(type, state, message)
+initGestureAuth()
+getAuthWidget()
}
MFAWidget o-- MFASequenceControl
Class diagram for updated SessionBaseModel and FullScreenBackgroundclassDiagram
class SessionBaseModel {
+setShutdownMode(is_black)
+shutdownkModeChanged(bool)
+m_enableShutdownBlackWidget: bool
}
class FullScreenBackground {
+m_shutdownBlackWidget: ShutdownBlackWidget*
}
SessionBaseModel <|-- FullScreenBackground
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @deepin-ci-robot - I've reviewed your changes and found some issues that need to be addressed.
- This PR merges a huge upstream sync along with a new gesture-login plugin; consider breaking it into smaller, focused PRs (core sync vs. new plugin) to simplify review and reduce integration risk.
- There are many hard-coded numbers for timings and layout spacings—defining these as named constants would improve readability and make future adjustments easier.
- The logic for sending extraInfo/readyToAuth signals is duplicated across several widgets—extracting a shared helper or base class for plugin communication could reduce code duplication and ensure consistent behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- This PR merges a huge upstream sync along with a new gesture-login plugin; consider breaking it into smaller, focused PRs (core sync vs. new plugin) to simplify review and reduce integration risk.
- There are many hard-coded numbers for timings and layout spacings—defining these as named constants would improve readability and make future adjustments easier.
- The logic for sending extraInfo/readyToAuth signals is duplicated across several widgets—extracting a shared helper or base class for plugin communication could reduce code duplication and ensure consistent behavior.
## Individual Comments
### Comment 1
<location> `src/session-widgets/auth_password.cpp:151` </location>
<code_context>
}
+ // 密码框下面增加一个认证界面
+ auto extendPlugin = PluginManager::instance()->getFirstLoginPlugin(dss::module::BaseModuleInterface::PasswordExtendLoginType);
+ if (extendPlugin) {
+ m_assistLoginWidget = new AssistLoginWidget(this);
</code_context>
<issue_to_address>
Potential for multiple AssistLoginWidget instances may cause confusion.
Assigning both plugin widgets to m_assistLoginWidget can cause ambiguity and incorrect signal/slot connections. Use separate member variables or a container to clearly distinguish between plugin instances.
</issue_to_address>
### Comment 2
<location> `src/session-widgets/auth_password.cpp:171` </location>
<code_context>
{
AuthModule::initConnections();
+
+ auto blockEditSig = [this]() -> bool {
+ return m_assistLoginWidget
+ && m_assistLoginWidget->isVisible()
</code_context>
<issue_to_address>
blockEditSig lambda may not distinguish between multiple assist login widgets.
Currently, m_assistLoginWidget only references the last created widget, which may cause blockEditSig to behave incorrectly when multiple assist login widgets exist. Please update the logic to handle multiple widgets appropriately.
</issue_to_address>
### Comment 3
<location> `src/session-widgets/auth_password.cpp:205` </location>
<code_context>
- connect(m_lineEdit, &DLineEditEx::returnPressed, this, [this] {
- if (!m_lineEdit->lineEdit()->isReadOnly()) // 避免用户在验证的时候反复点击
- emit requestAuthenticate();
+ connect(m_lineEdit, &DLineEditEx::returnPressed, this, [this, blockEditSig] {
+ if (!m_lineEdit->lineEdit()->isReadOnly()) { // 避免用户在验证的时候反复点击
+ if (!blockEditSig()) {
</code_context>
<issue_to_address>
Setting focus proxy to m_assistLoginWidget may not work if multiple widgets exist.
setFocusProxy(m_assistLoginWidget) will only apply to the most recently assigned widget, which may cause unexpected focus behavior. Please ensure focus is managed correctly for all assist login widgets.
</issue_to_address>
### Comment 4
<location> `src/session-widgets/assist_login_widget.cpp:56` </location>
<code_context>
+dss::module::BaseModuleInterface::ModuleType AssistLoginWidget::pluginType() const
+{
+ if (m_module)
+ return m_module->type();
+
</code_context>
<issue_to_address>
pluginType() returns LoginType by default, which may mask errors.
Returning LoginType when m_module is null may hide initialization issues. Consider returning a sentinel value to better detect misconfigurations.
Suggested implementation:
```cpp
dss::module::BaseModuleInterface::ModuleType AssistLoginWidget::pluginType() const
{
if (m_module)
return m_module->type();
return DSS_PLUGIN_TYPE::Unknown;
```
If `Unknown` is not already a value in the `ModuleType` enum, you will need to add it to the enum definition in the appropriate header file (likely `base_module_interface.h` or similar):
```cpp
enum class ModuleType {
LoginType,
// ... other types ...
Unknown
};
```
</issue_to_address>
### Comment 5
<location> `src/session-widgets/assist_login_widget.cpp:129` </location>
<code_context>
+ qCDebug(DDE_SHELL) << Q_FUNC_INFO << m_currentAuthData.result;
+ const QString &account = callbackData.account;
+ const QString &token = callbackData.token;
+ if (DSS_PLUGIN_TYPE::PasswordExtendLoginType == pluginType()) {
+ qCInfo(DDE_SHELL) << "Request send extra info, result: " << callbackData.result;
+ m_extraInfo = token;
</code_context>
<issue_to_address>
Overloading setAuthData for PasswordExtendLoginType may cause confusion.
Document the unique handling for PasswordExtendLoginType in setAuthData, and verify that other plugin types do not need similar logic.
</issue_to_address>
### Comment 6
<location> `src/widgets/dlineeditex.cpp:160` </location>
<code_context>
if ((watched == this || watched == this->lineEdit())
&& event->type() == QEvent::InputMethodQuery) {
return true;
+ } else if ((watched == this || watched == this->lineEdit())
+ && event->type() == QEvent::KeyPress
+ && m_enableTableKeyEvent) {
</code_context>
<issue_to_address>
Tab key triggers returnPressed, which may be unexpected for users.
Confirm that overriding Tab to emit returnPressed is intentional and does not hinder keyboard navigation or accessibility.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if (keyEvent->key() == Qt::Key_Tab
&& keyEvent->modifiers() == Qt::NoModifier
&& !this->text().isEmpty()) {
emit this->returnPressed();
return true;
}
=======
// Do not emit returnPressed() on Tab to preserve standard keyboard navigation and accessibility.
if (keyEvent->key() == Qt::Key_Tab
&& keyEvent->modifiers() == Qt::NoModifier) {
// Allow normal Tab key processing (focus navigation)
return false;
}
if (keyEvent->key() == Qt::Key_Return
&& keyEvent->modifiers() == Qt::NoModifier
&& !this->text().isEmpty()) {
emit this->returnPressed();
return true;
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 7
<location> `src/session-widgets/auth_face.cpp:76` </location>
<code_context>
*/
void AuthFace::setAuthState(const AuthCommon::AuthState state, const QString &result)
{
+ static bool startFilterTimer = false;
m_state = state;
switch (state) {
</code_context>
<issue_to_address>
Using a static variable for startFilterTimer may cause issues with multiple AuthFace instances.
A static variable here means all AuthFace instances share the same startFilterTimer, which can lead to incorrect behavior. Use a member variable to ensure each instance manages its own state.
</issue_to_address>
### Comment 8
<location> `src/widgets/shutdown_black_widget.cpp:69` </location>
<code_context>
+ setVisible(false);
+ setWindowFlags(Qt::WindowStaysOnTopHint | Qt::X11BypassWindowManagerHint);
+
+ QCursor cursor(Qt::BlankCursor);
+ this->setCursor(cursor);
+
+ installEventFilter(this);
</code_context>
<issue_to_address>
Cursor handling may not restore original cursor state.
Currently, the cursor is only restored in setBlackMode and not in debug mode, which may cause issues if the widget is shown or hidden multiple times. Please ensure the original cursor is always restored, regardless of build type.
Suggested implementation:
```cpp
ShutdownBlackWidget::ShutdownBlackWidget(QWidget *parent)
: QWidget(parent)
, m_cursor(cursor())
{
signal(SIGTERM, handleSIGTERM);
setObjectName(QStringLiteral("ShutdownBlackWidget"));
setAccessibleName(QStringLiteral("ShutdownBlackWidget"));
setVisible(false);
setWindowFlags(Qt::WindowStaysOnTopHint | Qt::X11BypassWindowManagerHint);
// Store the original cursor in m_cursor (already done in initializer list)
// Do not set blank cursor here; do it only when showing the widget
installEventFilter(this);
}
```
```cpp
void ShutdownBlackWidget::setBlackMode(const bool isBlackMode)
{
qInfo() << "ShutdownBlackWidget setBlackMode : " << isBlackMode;
setVisible(isBlackMode);
if (isBlackMode) {
// Set blank cursor when showing the widget
QCursor blankCursor(Qt::BlankCursor);
this->setCursor(blankCursor);
show();
} else {
// Always restore the original cursor when hiding the widget
this->setCursor(m_cursor);
hide();
}
#ifndef QT_DEBUG
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -146,6 +147,16 @@ void AuthPassword::initUI() | |||
| m_lineEdit->show(); | |||
| } | |||
|
|
|||
| // 密码框下面增加一个认证界面 | |||
| auto extendPlugin = PluginManager::instance()->getFirstLoginPlugin(dss::module::BaseModuleInterface::PasswordExtendLoginType); | |||
There was a problem hiding this comment.
issue (bug_risk): Potential for multiple AssistLoginWidget instances may cause confusion.
Assigning both plugin widgets to m_assistLoginWidget can cause ambiguity and incorrect signal/slot connections. Use separate member variables or a container to clearly distinguish between plugin instances.
| @@ -156,20 +167,27 @@ | |||
| void AuthPassword::initConnections() | |||
| { | |||
| AuthModule::initConnections(); | |||
|
|
|||
| auto blockEditSig = [this]() -> bool { | |||
There was a problem hiding this comment.
issue (bug_risk): blockEditSig lambda may not distinguish between multiple assist login widgets.
Currently, m_assistLoginWidget only references the last created widget, which may cause blockEditSig to behave incorrectly when multiple assist login widgets exist. Please update the logic to handle multiple widgets appropriately.
| connect(m_lineEdit, &DLineEditEx::returnPressed, this, [this] { | ||
| if (!m_lineEdit->lineEdit()->isReadOnly()) // 避免用户在验证的时候反复点击 | ||
| emit requestAuthenticate(); | ||
| connect(m_lineEdit, &DLineEditEx::returnPressed, this, [this, blockEditSig] { |
There was a problem hiding this comment.
issue (bug_risk): Setting focus proxy to m_assistLoginWidget may not work if multiple widgets exist.
setFocusProxy(m_assistLoginWidget) will only apply to the most recently assigned widget, which may cause unexpected focus behavior. Please ensure focus is managed correctly for all assist login widgets.
| } | ||
| } | ||
|
|
||
| dss::module::BaseModuleInterface::ModuleType AssistLoginWidget::pluginType() const | ||
| { | ||
| if (m_module) |
There was a problem hiding this comment.
suggestion (bug_risk): pluginType() returns LoginType by default, which may mask errors.
Returning LoginType when m_module is null may hide initialization issues. Consider returning a sentinel value to better detect misconfigurations.
Suggested implementation:
dss::module::BaseModuleInterface::ModuleType AssistLoginWidget::pluginType() const
{
if (m_module)
return m_module->type();
return DSS_PLUGIN_TYPE::Unknown;
If Unknown is not already a value in the ModuleType enum, you will need to add it to the enum definition in the appropriate header file (likely base_module_interface.h or similar):
enum class ModuleType {
LoginType,
// ... other types ...
Unknown
};| qCDebug(DDE_SHELL) << Q_FUNC_INFO << m_currentAuthData.result; | ||
| const QString &account = callbackData.account; | ||
| const QString &token = callbackData.token; | ||
| if (DSS_PLUGIN_TYPE::PasswordExtendLoginType == pluginType()) { |
There was a problem hiding this comment.
question: Overloading setAuthData for PasswordExtendLoginType may cause confusion.
Document the unique handling for PasswordExtendLoginType in setAuthData, and verify that other plugin types do not need similar logic.
Comment on lines
+164
to
+169
| if (keyEvent->key() == Qt::Key_Tab | ||
| && keyEvent->modifiers() == Qt::NoModifier | ||
| && !this->text().isEmpty()) { | ||
| emit this->returnPressed(); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
suggestion: Tab key triggers returnPressed, which may be unexpected for users.
Confirm that overriding Tab to emit returnPressed is intentional and does not hinder keyboard navigation or accessibility.
Suggested change
| if (keyEvent->key() == Qt::Key_Tab | |
| && keyEvent->modifiers() == Qt::NoModifier | |
| && !this->text().isEmpty()) { | |
| emit this->returnPressed(); | |
| return true; | |
| } | |
| // Do not emit returnPressed() on Tab to preserve standard keyboard navigation and accessibility. | |
| if (keyEvent->key() == Qt::Key_Tab | |
| && keyEvent->modifiers() == Qt::NoModifier) { | |
| // Allow normal Tab key processing (focus navigation) | |
| return false; | |
| } | |
| if (keyEvent->key() == Qt::Key_Return | |
| && keyEvent->modifiers() == Qt::NoModifier | |
| && !this->text().isEmpty()) { | |
| emit this->returnPressed(); | |
| return true; | |
| } |
| @@ -68,6 +73,7 @@ void AuthFace::reset() | |||
| */ | |||
| void AuthFace::setAuthState(const AuthCommon::AuthState state, const QString &result) | |||
| { | |||
| static bool startFilterTimer = false; | |||
There was a problem hiding this comment.
issue (bug_risk): Using a static variable for startFilterTimer may cause issues with multiple AuthFace instances.
A static variable here means all AuthFace instances share the same startFilterTimer, which can lead to incorrect behavior. Use a member variable to ensure each instance manages its own state.
Comment on lines
+69
to
+70
| QCursor cursor(Qt::BlankCursor); | ||
| this->setCursor(cursor); |
There was a problem hiding this comment.
suggestion (bug_risk): Cursor handling may not restore original cursor state.
Currently, the cursor is only restored in setBlackMode and not in debug mode, which may cause issues if the widget is shown or hidden multiple times. Please ensure the original cursor is always restored, regardless of build type.
Suggested implementation:
ShutdownBlackWidget::ShutdownBlackWidget(QWidget *parent)
: QWidget(parent)
, m_cursor(cursor())
{
signal(SIGTERM, handleSIGTERM);
setObjectName(QStringLiteral("ShutdownBlackWidget"));
setAccessibleName(QStringLiteral("ShutdownBlackWidget"));
setVisible(false);
setWindowFlags(Qt::WindowStaysOnTopHint | Qt::X11BypassWindowManagerHint);
// Store the original cursor in m_cursor (already done in initializer list)
// Do not set blank cursor here; do it only when showing the widget
installEventFilter(this);
}
void ShutdownBlackWidget::setBlackMode(const bool isBlackMode)
{
qInfo() << "ShutdownBlackWidget setBlackMode : " << isBlackMode;
setVisible(isBlackMode);
if (isBlackMode) {
// Set blank cursor when showing the widget
QCursor blankCursor(Qt::BlankCursor);
this->setCursor(blankCursor);
show();
} else {
// Always restore the original cursor when hiding the widget
this->setCursor(m_cursor);
hide();
}
#ifndef QT_DEBUG
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Synchronize source files from linuxdeepin/dde-session-shell.
Source-pull-request: linuxdeepin/dde-session-shell#30
Summary by Sourcery
Sync source from linuxdeepin/dde-session-shell, integrating new authentication workflows and plugins
New Features:
Bug Fixes:
Enhancements:
Build: