fix: term opening on Linux #845
Merged
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
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR refactors the Linux terminal launch script to improve auto-detection by expanding the supported terminal list, preferring xterm as a fallback, and consolidating case logic, and also updates the CI release workflow by bumping the Flutter version, adjusting apt dependencies, removing manual AppImage tool download, and standardizing AppImage artifact naming. Sequence diagram for improved Linux terminal auto-detection and launchsequenceDiagram
participant App as Application
participant Script as launch_terminal.sh
participant Terminal as Terminal Emulator
App->>Script: Request to launch terminal
Script->>Script: Check provided terminal argument
alt Terminal not provided or is x-terminal-emulator
Script->>Script: Iterate through preferred terminal list
Script->>Script: Select first available terminal (incl. xterm fallback)
end
Script->>Terminal: Launch terminal with appropriate arguments
Terminal-->>App: Terminal window opens
Class diagram for updated terminal launch handling in shell scriptclassDiagram
class TerminalLauncher {
+terminal: String
+launch(args)
+autoDetectTerminal()
+supportedTerminals: [kitty, alacritty, gnome-terminal, gnome-console, konsole, xfce4-terminal, terminator, tilix, wezterm, foot, xterm]
}
TerminalLauncher : autoDetectTerminal() selects first available terminal
TerminalLauncher : launch(args) executes terminal with correct flags
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @lollipopkit - I've reviewed your changes - here's some feedback:
- Make sure the invocation flags for the newly added terminals (mate-terminal, qterminal, lxterminal, deepin-terminal) are correct, as some of these may not support the "-e" syntax and could require different options.
- Consider consolidating the repeated exec logic for terminals with the same flag patterns (e.g. all those using "-e") to simplify and reduce duplication in the case statement.
- Double-check that the CI pipeline still produces and uploads the correctly named AppImage, since the separate rename step was removed and the extension was changed to ".AppImage".
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Make sure the invocation flags for the newly added terminals (mate-terminal, qterminal, lxterminal, deepin-terminal) are correct, as some of these may not support the "-e" syntax and could require different options.
- Consider consolidating the repeated exec logic for terminals with the same flag patterns (e.g. all those using "-e") to simplify and reduce duplication in the case statement.
- Double-check that the CI pipeline still produces and uploads the correctly named AppImage, since the separate rename step was removed and the extension was changed to ".AppImage".
## Individual Comments
### Comment 1
<location> `lib/view/widget/server_func_btns.dart:342` </location>
<code_context>
exec "\$TERMINAL" "\$@"
;;
- urxvt|rxvt-unicode)
+ urxvt|rxvt-unicode|xterm|mate-terminal|qterminal|lxterminal|deepin-terminal)
exec "\$TERMINAL" -e "\$@"
;;
</code_context>
<issue_to_address>
Grouping many terminals under the same exec pattern may miss terminal-specific options.
Some terminals in this group may support unique options or behave differently. Please confirm that '-e' is compatible with each, or note any exceptions.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
urxvt|rxvt-unicode|xterm|mate-terminal|qterminal|lxterminal|deepin-terminal)
exec "\$TERMINAL" -e "\$@"
;;
=======
urxvt|rxvt-unicode)
exec "\$TERMINAL" -e "\$@"
;;
xterm)
exec "\$TERMINAL" -e "\$@"
;;
mate-terminal)
# mate-terminal supports -- but not -e for custom commands
exec "\$TERMINAL" -- "\$@"
;;
qterminal)
exec "\$TERMINAL" -e "\$@"
;;
lxterminal)
exec "\$TERMINAL" -e "\$@"
;;
deepin-terminal)
# deepin-terminal supports -e, but may require quotes around the command
exec "\$TERMINAL" -e "\$@"
;;
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `.github/workflows/release.yml:61` </location>
<code_context>
sudo apt update
# Basic
- sudo apt install -y clang cmake ninja-build pkg-config libgtk-3-dev libvulkan-dev desktop-file-utils wget
+ sudo apt install -y clang cmake ninja-build pkg-config libgtk-3-dev mesa-utils libvulkan-dev desktop-file-utils wget
# App Specific
- sudo apt install -y libgstreamer1.0-dev libgstreamer-plugins-base1.0-dev libunwind-dev
</code_context>
<issue_to_address>
Adding mesa-utils may be unnecessary for build environments.
mesa-utils is mainly for runtime diagnostics, not building. If it's not needed for the build, please remove it to streamline dependencies.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
sudo apt install -y clang cmake ninja-build pkg-config libgtk-3-dev mesa-utils libvulkan-dev desktop-file-utils wget
=======
sudo apt install -y clang cmake ninja-build pkg-config libgtk-3-dev libvulkan-dev desktop-file-utils wget
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
342
to
344
| urxvt|rxvt-unicode|xterm|mate-terminal|qterminal|lxterminal|deepin-terminal) | ||
| exec "\$TERMINAL" -e "\$@" | ||
| ;; |
There was a problem hiding this comment.
suggestion: Grouping many terminals under the same exec pattern may miss terminal-specific options.
Some terminals in this group may support unique options or behave differently. Please confirm that '-e' is compatible with each, or note any exceptions.
Suggested change
| urxvt|rxvt-unicode|xterm|mate-terminal|qterminal|lxterminal|deepin-terminal) | |
| exec "\$TERMINAL" -e "\$@" | |
| ;; | |
| urxvt|rxvt-unicode) | |
| exec "\$TERMINAL" -e "\$@" | |
| ;; | |
| xterm) | |
| exec "\$TERMINAL" -e "\$@" | |
| ;; | |
| mate-terminal) | |
| # mate-terminal supports -- but not -e for custom commands | |
| exec "\$TERMINAL" -- "\$@" | |
| ;; | |
| qterminal) | |
| exec "\$TERMINAL" -e "\$@" | |
| ;; | |
| lxterminal) | |
| exec "\$TERMINAL" -e "\$@" | |
| ;; | |
| deepin-terminal) | |
| # deepin-terminal supports -e, but may require quotes around the command | |
| exec "\$TERMINAL" -e "\$@" | |
| ;; |
Owner
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @lollipopkit - I've reviewed your changes - here's some feedback:
- To keep the shell launch script maintainable as you add more terminals, consider extracting the repeated
execpatterns into helper functions or a small lookup table instead of large inline case arms. - After removing the manual AppImage tool download in your CI, make sure
appimagetoolis still installed on the runner (for example viaapt install appimagetool) so the build step won’t fail.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- To keep the shell launch script maintainable as you add more terminals, consider extracting the repeated `exec` patterns into helper functions or a small lookup table instead of large inline case arms.
- After removing the manual AppImage tool download in your CI, make sure `appimagetool` is still installed on the runner (for example via `apt install appimagetool`) so the build step won’t fail.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Fixes #822
Summary by Sourcery
Improve Linux terminal launching logic by broadening supported terminals and streamlining the shell script, and update the release CI pipeline with updated Flutter version, dependencies, and artifact naming conventions
Bug Fixes:
Enhancements: