-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Added new kde-plasma-mobile environment for trixie #8769
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?
Added new kde-plasma-mobile environment for trixie #8769
Conversation
WalkthroughAdds a new KDE Plasma Mobile environment under config/desktop/trixie: an architectures file ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… for touchscreen by default; changed armbian to only write armbian sddm config if doesn't exist;
919340b
to
998c1fe
Compare
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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
config/desktop/trixie/environments/kde-plasma-mobile/debian/postinst (1)
1-3
: Remove commented-out lightdm configuration.These commented lines appear to be remnants from copying the script from another desktop environment. Since KDE Plasma Mobile uses SDDM, these lightdm-related comments should be removed to avoid confusion.
Apply this diff to remove the commented lines:
-# overwrite stock lightdm greeter configuration - #if [ -d /etc/armbian/lightdm ]; then cp -R /etc/armbian/lightdm /etc/; fi -#if [ -f /etc/lightdm/slick-greeter.conf ]; then sed -i 's/armbian03-Dre0x-Minum-dark-blurred-3840x2160.jpg/armbian-4k-black-psycho-gauss.jpg/g' /etc/lightdm/slick-greeter.conf; fi - # Disable Pulseaudio timer scheduling which does not work with sndhdmi driverconfig/desktop/trixie/environments/kde-plasma-mobile/armbian/create_desktop_package.sh (1)
17-28
: Clarify whether wallpaper script should be enabled.The entire wallpaper-setting script is commented out. If the default wallpaper should be set automatically for new users, this script needs to be uncommented and potentially integrated into the user session startup. Otherwise, if this is intentional, consider removing the commented code to reduce clutter.
Do you want this wallpaper script enabled, or should it be removed?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
config/desktop/trixie/environments/kde-plasma-mobile/architectures
(1 hunks)config/desktop/trixie/environments/kde-plasma-mobile/armbian/create_desktop_package.sh
(1 hunks)config/desktop/trixie/environments/kde-plasma-mobile/config_base/packages
(1 hunks)config/desktop/trixie/environments/kde-plasma-mobile/config_base/packages.uninstall
(1 hunks)config/desktop/trixie/environments/kde-plasma-mobile/debian/postinst
(1 hunks)config/desktop/trixie/environments/kde-plasma-mobile/support
(1 hunks)packages/bsp/common/usr/lib/armbian/armbian-firstlogin
(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-16T15:34:34.672Z
Learnt from: Grippy98
PR: armbian/build#8202
File: config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh:2-3
Timestamp: 2025-05-16T15:34:34.672Z
Learning: The shell scripts in the Armbian build system's desktop configuration files, such as config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh, rely on environment variables like $SRC and $destination that are expected to be set externally before the script is executed.
Applied to files:
config/desktop/trixie/environments/kde-plasma-mobile/armbian/create_desktop_package.sh
🧬 Code graph analysis (3)
config/desktop/trixie/environments/kde-plasma-mobile/armbian/create_desktop_package.sh (2)
lib/functions/artifacts/artifact-armbian-bsp-desktop.sh (1)
artifact_armbian-bsp-desktop_prepare_version
(20-62)lib/functions/artifacts/artifact-armbian-desktop.sh (1)
artifact_armbian-desktop_prepare_version
(20-68)
config/desktop/trixie/environments/kde-plasma-mobile/debian/postinst (1)
lib/functions/bsp/armbian-bsp-cli-deb.sh (3)
board_side_bsp_cli_postinst_finish
(448-471)board_side_bsp_cli_preinst
(360-415)board_side_bsp_cli_postrm
(417-423)
packages/bsp/common/usr/lib/armbian/armbian-firstlogin (1)
lib/functions/bsp/armbian-bsp-cli-deb.sh (1)
board_side_bsp_cli_postinst_finish
(448-471)
🔇 Additional comments (12)
config/desktop/trixie/environments/kde-plasma-mobile/architectures (1)
1-1
: LGTM!The architecture specification correctly lists arm64 and amd64, which are appropriate for KDE Plasma Mobile support.
packages/bsp/common/usr/lib/armbian/armbian-firstlogin (1)
957-965
: LGTM!The guarded write prevents overwriting an existing SDDM configuration that may have been created by environment-specific postinst scripts (such as the KDE Plasma Mobile postinst). This defensive approach ensures that environment-specific configurations take precedence.
config/desktop/trixie/environments/kde-plasma-mobile/debian/postinst (2)
6-6
: LGTM!The PulseAudio configuration correctly disables timer scheduling for sndhdmi driver compatibility.
9-10
: Verify systemctl --user execution context.The postinst script likely runs as root during package installation, but
systemctl --user
commands are intended for user sessions. Ensure this works correctly, as it may fail or have no effect if no user session is active during installation.Consider deferring user service management to first login or using a different mechanism to enable PulseAudio for the user session.
config/desktop/trixie/environments/kde-plasma-mobile/armbian/create_desktop_package.sh (4)
1-3
: LGTM!The SDDM theme installation correctly copies the plasma-chili theme to the appropriate destination directory. Based on learnings, the
$SRC
and$destination
variables are expected to be set externally before this script executes.
5-7
: LGTM!The desktop settings skeleton is correctly installed to /etc/skel for new user defaults.
9-11
: LGTM!Wallpaper installation is implemented correctly.
13-15
: LGTM!The Armbian logo for the login screen is installed correctly.
config/desktop/trixie/environments/kde-plasma-mobile/support (1)
1-1
: LGTM!The support level is correctly set to "csc" (community support) as indicated in the PR description.
config/desktop/trixie/environments/kde-plasma-mobile/config_base/packages (3)
7-11
: LGTM!The input method packages include both Qt6 virtual keyboard and Maliit framework. This provides flexibility for virtual keyboard support on mobile devices. The SDDM theme matches the configuration set in the postinst script.
12-19
: LGTM!The PulseAudio stack is comprehensive and includes Bluetooth support. The multiple control tools (pamix, pavucontrol, pavucontrol-qt) provide flexibility for different user preferences. Chromium is a suitable default browser choice.
1-3
: Verify package redundancyConfirm whether
plasma-mobile-full
,kde-full
, andtask-kde-desktop
overlap significantly and if all three are required to avoid unnecessary bloat.
config/desktop/trixie/environments/kde-plasma-mobile/debian/postinst
Outdated
Show resolved
Hide resolved
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.
Actionable comments posted: 1
🧹 Nitpick comments (3)
config/desktop/trixie/environments/kde-plasma-mobile/debian/postinst (3)
1-3
: Remove commented-out code.These commented-out lines serve no purpose in a new environment configuration and should be removed to keep the codebase clean.
6-6
: Minor formatting issue and consider idempotency.The double space in
-i /etc/pulse/default.pa
is unnecessary. Additionally, consider checking if the modification has already been applied to avoid duplicate parameters.Apply this diff to fix the formatting:
-if [ -f /etc/pulse/default.pa ]; then sed "s/load-module module-udev-detect$/& tsched=0/g" -i /etc/pulse/default.pa; fi +if [ -f /etc/pulse/default.pa ]; then sed "s/load-module module-udev-detect$/& tsched=0/g" -i /etc/pulse/default.pa; fiFor idempotency, you could modify the sed pattern to check if
tsched=0
is not already present:-if [ -f /etc/pulse/default.pa ]; then sed "s/load-module module-udev-detect$/& tsched=0/g" -i /etc/pulse/default.pa; fi +if [ -f /etc/pulse/default.pa ]; then sed "/tsched=/!s/load-module module-udev-detect$/& tsched=0/g" -i /etc/pulse/default.pa; fi
13-23
: Consider adding error handling and shebang.While the SDDM configuration logic is correct, the script lacks:
- A shebang line (e.g.,
#!/bin/bash
)- Error handling for file operations
Add a shebang at the top of the file:
#!/bin/bash set -e # Exit on errorAnd optionally add error checking:
-mkdir -p /etc/sddm.conf.d -cat <<- EOF > /etc/sddm.conf.d/armbian.conf +mkdir -p /etc/sddm.conf.d || { echo "Failed to create SDDM config directory"; exit 1; } +cat <<- EOF > /etc/sddm.conf.d/armbian.conf || { echo "Failed to write SDDM config"; exit 1; } [Theme] Current=breeze [General] InputMethod=qtvirtualkeyboard [X11] DisplayCommand=/usr/share/sddm/scripts/Xsetup [Wayland] CompositorCommand=weston --shell=kiosk # @TODO figure out how to rotate with weston and wayland EOFNote: Debian postinst scripts typically use
#!/bin/bash
or#!/bin/sh
and should handle errors gracefully.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
config/desktop/trixie/environments/kde-plasma-mobile/debian/postinst
(1 hunks)
config/desktop/trixie/environments/kde-plasma-mobile/debian/postinst
Outdated
Show resolved
Hide resolved
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.
we are trying to move away from this kind of scripting and define this packaging logic in APA instead.
Can you provide an example on what I can do to change this - since it looks like the existing environments still use the existing scripting form. Would the environment just call the armbian package instead? |
APA is enabled for Debian Sid ATM - for smoother transition. If we want Trixie kde plasma right now, this PR looks O.K., but keep in mind this code, the way desktops are made, will eventually be dropped. I think we already prepared desktops like XFCE and Gnome @leggewie ? |
Description
Added New Files
Modified Files
How Has This Been Tested?
Checklist:
Please delete options that are not relevant.