feat: support start sessions other than treeland#558
feat: support start sessions other than treeland#558zccrs merged 1 commit intolinuxdeepin:masterfrom
Conversation
|
Hi @apr3vau. Thanks for your PR. I'm waiting for a linuxdeepin member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Reviewer's GuideIntroduces a session‐selection button in the lockscreen UI to choose non-Treeland sessions, enhances the session list popup with styling and a scrollbar, and updates SessionModel to include all executable sessions and match last session by exec identifier. Sequence diagram for session selection interaction in lockscreensequenceDiagram
actor User
participant "Session Selection Button"
participant "SessionList Popup"
participant SessionModel
User->>"Session Selection Button": Click
"Session Selection Button"->>"SessionList Popup": open()
User->>"SessionList Popup": Select session
"SessionList Popup"->>SessionModel: updateCurrentSession(index)
"SessionList Popup"->>"SessionList Popup": close()
Class diagram for updated SessionModel and SessionListclassDiagram
class SessionModel {
+populate(type, dirPaths)
-displayNames: QStringList
-sessions: QList<SessionInfo*>
-lastIndex: int
}
class SessionInfo {
+displayName()
+exec()
+fileName()
+isSingleMode()
}
SessionModel "1" *-- "many" SessionInfo
class SessionList {
+open()
+close()
+updateCurrentSession(index)
+currentIndex: int
}
SessionList o-- SessionModel
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Use QML anchors or layouts instead of manual x/y offsets for positioning the sessionList popup to ensure it adapts to different screen sizes and resolutions.
- Bind the sessionItem label to GreeterModel.currentSession so that the button text updates immediately when a new session is selected.
- Replace the hardcoded Popup height in SessionList.qml with implicitHeight or calculate it based on the model count to support dynamic content sizing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Use QML anchors or layouts instead of manual x/y offsets for positioning the sessionList popup to ensure it adapts to different screen sizes and resolutions.
- Bind the sessionItem label to GreeterModel.currentSession so that the button text updates immediately when a new session is selected.
- Replace the hardcoded Popup height in SessionList.qml with implicitHeight or calculate it based on the model count to support dynamic content sizing.
## Individual Comments
### Comment 1
<location> `src/plugins/lockscreen/qml/SessionList.qml:39` </location>
<code_context>
clip: true
model: SessionModel
+
+ ScrollBar.vertical: ScrollBar {
+ id: verticalScrollBar
+ anchors.right: parent.right
+ anchors.top: parent.top
+ anchors.bottom: parent.bottom
+ policy: ScrollBar.AlwaysOn
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Always-on vertical scrollbar may impact visual clarity.
Using 'ScrollBar.AlwaysOn' may unnecessarily clutter the UI if the content fits. Consider 'ScrollBar.AsNeeded' unless always-on is required by design.
```suggestion
policy: ScrollBar.AsNeeded
```
</issue_to_address>
### Comment 2
<location> `src/plugins/lockscreen/qml/SessionList.qml:44` </location>
<code_context>
+
delegate: Item {
required property string name
+ required property int index
width: parent.width
height: 60
</code_context>
<issue_to_address>
**suggestion:** Passing index to delegate may introduce maintenance complexity.
Using ListView's built-in 'index' property within the delegate is preferable, as it reduces the risk of errors if the model changes or the delegate is reused.
Suggested implementation:
```
```
```
updateCurrentSession(index)
```
The built-in `index` property is automatically available in ListView delegates, so you can use it directly without declaring it as a required property or passing it from the model. If the model or delegate is reused elsewhere, ensure that any references to `index` are using the built-in property and not expecting it from the model.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull Request Overview
This PR enables treeland to function as a display manager greeter by adding session selection capabilities, allowing users to choose and launch different desktop environments beyond just treeland.
- Adds a session selection button to the lock screen control panel for choosing available desktop sessions
- Fixes SessionModel to include all executable sessions instead of only single-mode sessions
- Introduces UI improvements including a scrollable session list popup and proper session switching logic
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/plugins/lockscreen/qml/SessionList.qml | Enhanced popup with scrollbar support and improved session selection handling |
| src/plugins/lockscreen/qml/ControlAction.qml | Added session selection button with popup integration |
| src/greeter/sessionmodel.cpp | Removed single-mode restriction and fixed last session lookup logic |
| src/greeter/greeterproxy.h | Added isLoggedIn property declaration |
| src/greeter/greeterproxy.cpp | Implemented isLoggedIn property with proper state management |
| misc/wayland-sessions/treeland-user.desktop.in | Updated display name for clarity |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
fa3147d to
91c0a18
Compare
|
TAG Bot New tag: 0.7.1 |
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
TAG Bot New tag: 0.7.2 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: apr3vau, zccrs The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
TAG Bot New tag: 0.7.3 |
This commit fixed the SessionModel and SessionList, and enabled session selection by adding a button to ControlAction.qml. Thus treeland can act as a greeter of ddm, guide the start of other desktop environment.
This commit fixed the SessionModel and SessionList, and enabled session selection by adding a button to ControlAction.qml. Thus treeland can act as a greeter of ddm, guide the start of other desktop environment.
Summary by Sourcery
Enable treeland to serve as a greeter for the display manager by adding UI and model support for selecting and launching non-treeland sessions
New Features:
Bug Fixes:
Enhancements: