Skip to content

Commit 9cd3489

Browse files
talesamtalesam
committed
🔨 refactor: refactor: comprehensive code review + 35 improvements across security, a11y, architecture & UX
Security: - Fix command injection: shell=True → subprocess.run(list) in CommandExecutor - Fix zip path traversal in import (validate member paths) - Fix shell script quoting (big-webapps, big-webapps-exec) - Replace fragile manual JSON generation with _json_escape() + printf - Convert browser_exec to bash array (big-webapps-exec) - Add cmp -s guard for icon copy on every launch - Add flock advisory locking for Wayland .desktop race condition - Replace sed chain with case statement for browser name mapping Architecture: - Create webapp_service.py: centralized business logic layer (CRUD, export/import) - Create browser_registry.py: single source of truth for browser ID/name/pattern mapping - Create favicon_picker.py: extracted FlowBox icon selector widget - Eliminate get_json.sh chain → direct big-webapps json + enrich_webapps_with_icons() - Simplify application.py (-120L): all business logic moved to WebAppService - Simplify main_window.py: all CRUD delegated to service layer - Split webapp_dialog.py setup_ui() monolith → 5 builder methods - Remove dead find_all_widget_types() method → use self.name_row directly - State management: find_webapp() uses app_file (desktop filename) as stable ID Accessibility (Orca screen reader): - Add accessible labels to all interactive elements (buttons, entries, switches, dropdown) - Add accessible descriptions to FlowBox favicon items - Category headings: AccessibleRole.HEADING + focusable (Orca h-key nav) - Destructive toasts: HIGH priority (assertive for screen readers) - Empty state: focusable AdwStatusPage with grab_focus() - Loading overlay: accessible description for screen readers - Delete button: destructive-action CSS class (color + icon shape = dual indicator) - Dynamic focus order: URL for new webapps, Name for editing UX: - Progressive disclosure: profile settings in AdwExpanderRow (collapsed by default) - Save feedback: loading overlay + background thread + GLib.idle_add callback - URL real-time validation: urlparse check + suffix icon (success/error) + CSS classes - Welcome dialog: rename "Show dialog on startup" → "Don't show this again" (inverted logic) - Remove-all: replace double-confirm dialog with single text-confirm entry - FaviconPicker: .favicon-selected CSS class for visual selection feedback - Delete confirmation: include URL and browser info for disambiguation Code Quality: - Migrate get_app_icon_url.py from GTK3 to GTK4 - Reduce cyclomatic complexity: 6 functions refactored (avg CC 54→5) - Add type hints to all function signatures - Fix unused variables (prefix _ for GTK callback params) - Fix BROWSER_ICONS_PATH relative → absolute path - Fix name_label.set_ellipsize(True) → Pango.EllipsizeMode.END - Fix _open_folder infinite recursion - Replace print() with structured logging (logging module) - Consistent UUID-based webapp file IDs - ruff lint: 0 errors, all checks passed - bash -n: all shell scripts valid
1 parent 9c04ebe commit 9cd3489

17 files changed

Lines changed: 1107 additions & 945 deletions

‎PLANNING.md‎

Lines changed: 83 additions & 44 deletions
Large diffs are not rendered by default.

‎biglinux-webapps/usr/bin/big-webapps‎

Lines changed: 33 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ if [[ $command == "json" ]]; then
5151
one_file="$1"
5252

5353
# Remove
54-
elif [ $command == "remove" ]; then
54+
elif [ "$command" == "remove" ]; then
5555
if [ "$#" = "0" ]; then
5656
display_help
5757
exit 1
@@ -61,8 +61,8 @@ elif [ $command == "remove" ]; then
6161
browser="$2"
6262
profile="$3"
6363

64-
if [ -f $filename ]; then
65-
rm $filename
64+
if [ -f "$filename" ]; then
65+
rm "$filename"
6666

6767
echo "WebApp successfully removed!"
6868
exit 0
@@ -72,7 +72,7 @@ elif [ $command == "remove" ]; then
7272
fi
7373

7474
# Remove
75-
elif [ $command = "remove-with-folder" ]; then
75+
elif [ "$command" = "remove-with-folder" ]; then
7676
if [ "$#" = "0" ]; then
7777
display_help
7878
exit 1
@@ -82,11 +82,11 @@ elif [ $command = "remove-with-folder" ]; then
8282
browser="$2"
8383
profile="$3"
8484

85-
if [ -f $filename ]; then
86-
rm $filename
85+
if [ -f "$filename" ]; then
86+
rm "$filename"
8787
# Verify if $browser variable not null and $profile variable not null and folder $HOME/.bigwebapps/$browser/$profile exists
88-
if [[ -n $browser ]] && [[ -n $profile ]] && [[ -d $HOME/.bigwebapps/$browser/$profile ]]; then
89-
rm -rf $HOME/.bigwebapps/$browser/$profile
88+
if [[ -n "$browser" ]] && [[ -n "$profile" ]] && [[ -d "$HOME/.bigwebapps/$browser/$profile" ]]; then
89+
rm -rf "$HOME/.bigwebapps/$browser/$profile"
9090
else
9191
if [[ "$browser" == "firefox" || "$browser" == "librewolf" || "$browser" == "flatpak-firefox" || "$browser" == "flatpak-librewolf" ]]; then
9292
# Firefox and Librewolf always remove the profile folder
@@ -126,17 +126,20 @@ fi
126126
if [[ "$browser" == "__viewer__" ]]; then
127127
short_browser="viewer"
128128
else
129-
short_browser=$(sed 's/.*[Cc]hrom.*/chrome/
130-
s/.*[Bb]rave.*/brave/
131-
s/.*[Ee]dge.*/msedge/
132-
s/.*[Vv]ivaldi.*/vivaldi/' <<< "$browser")
129+
case "$browser" in
130+
*[Cc]hrom*) short_browser="chrome" ;;
131+
*[Bb]rave*) short_browser="brave" ;;
132+
*[Ee]dge*) short_browser="msedge" ;;
133+
*[Vv]ivaldi*) short_browser="vivaldi" ;;
134+
*) short_browser="$browser" ;;
135+
esac
133136
fi
134137

135138
# Adjust the class by replacing "/" with "__" and removing https
136139
class=$(sed 's|https://||;s|http://||g;s|/|__|g' <<< "$url")
137140

138141
# Define the filename in Wayland compatible format
139-
filename="$short_browser-$(sed 's|https://||;s|http://||;s|?.*||g;s|/|__|g' <<< $url)-Default.desktop"
142+
filename="$short_browser-$(sed 's|https://||;s|http://||;s|?.*||g;s|/|__|g' <<< "$url")-Default.desktop"
140143

141144
# Keep first occurrence of __ and replace all others with _
142145
filename=$(sed 's/__/\n/g;s/\n/__/;s/\n/_/g' <<< "$filename")
@@ -209,7 +212,7 @@ elif [ "$command" = "verify" ]; then
209212
exit 1
210213
fi
211214

212-
if [ -f $filename_orig ]; then
215+
if [ -f "$filename_orig" ]; then
213216
echo "true"
214217
exit 1
215218
else
@@ -270,11 +273,11 @@ elif [ "$command" = "json" ]; then
270273
categories=${line#*Categories=}
271274
;;
272275
esac
273-
done <<<$(grep -ve 'Name=Software Render' -ve 'Exec=SoftwareRender ' $file | grep -m4 -e '^Name=' -e '^Exec=' -e '^Icon=' -e '^Categories=')
276+
done <<<"$(grep -ve 'Name=Software Render' -ve 'Exec=SoftwareRender ' "$file" | grep -m4 -e '^Name=' -e '^Exec=' -e '^Icon=' -e '^Categories=')"
274277

275278
# Print the JSON separator
276279
if [[ $num -gt 0 ]]; then
277-
echo ,
280+
printf ',\n'
278281
fi
279282

280283
# Determine app_mode from browser
@@ -284,20 +287,20 @@ elif [ "$command" = "json" ]; then
284287
app_mode="browser"
285288
fi
286289

287-
# Print the JSON object with escaped quotes
288-
read -d $'' ShowText <<EOF
289-
{
290-
"browser": "$browser",
291-
"app_file": "${file//\"/\\\\\"}",
292-
"app_name": "${name//\"/\\\\\"}",
293-
"app_url": "${url//\"/\\\\\"}",
294-
"app_icon": "$icon",
295-
"app_profile": "$profile",
296-
"app_categories": "$categories",
297-
"app_mode": "$app_mode"
298-
}
299-
EOF
300-
echo "$ShowText"
290+
# JSON-escape values: handle backslash, double-quote, control chars
291+
_json_escape() { printf '%s' "$1" | sed 's/\\/\\\\/g;s/"/\\"/g;s/\t/\\t/g'; }
292+
293+
# Print the JSON object
294+
printf ' {\n'
295+
printf ' "browser": "%s",\n' "$(_json_escape "$browser")"
296+
printf ' "app_file": "%s",\n' "$(_json_escape "$file")"
297+
printf ' "app_name": "%s",\n' "$(_json_escape "$name")"
298+
printf ' "app_url": "%s",\n' "$(_json_escape "$url")"
299+
printf ' "app_icon": "%s",\n' "$(_json_escape "$icon")"
300+
printf ' "app_profile": "%s",\n' "$(_json_escape "$profile")"
301+
printf ' "app_categories": "%s",\n' "$(_json_escape "$categories")"
302+
printf ' "app_mode": "%s"\n' "$app_mode"
303+
printf ' }'
301304
num+=1
302305
done
303306
# Print the JSON array end

‎biglinux-webapps/usr/bin/big-webapps-exec‎

Lines changed: 47 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -17,47 +17,51 @@ if ! [[ $url =~ http:|file:|https: ]]; then
1717
fi
1818

1919
if [[ $icon =~ / ]]; then
20-
cp "$icon" ~/.local/share/icons/
21-
sed -Ei 's|^Icon=.*/(.*)\..*|Icon=\1|g' "$HOME/.local/share/applications/$filename"
20+
# copy icon only if not already present
21+
icon_dest="$HOME/.local/share/icons/${icon##*/}"
22+
if [[ ! -f "$icon_dest" ]] || ! cmp -s "$icon" "$icon_dest"; then
23+
cp "$icon" "$icon_dest"
24+
sed -Ei 's|^Icon=.*/(.*)\..*/|Icon=\1|g' "$HOME/.local/share/applications/$filename"
25+
fi
2226
fi
2327

24-
# Flatpak name to flatpak file address
28+
# Flatpak name to flatpak file address — use arrays for safe word splitting
2529
case $browser in
2630
flatpak-brave)
27-
browser_exec="flatpak run com.brave.Browser"
31+
browser_exec=(flatpak run com.brave.Browser)
2832
;;
2933
flatpak-chrome)
30-
browser_exec="flatpak run com.google.Chrome"
34+
browser_exec=(flatpak run com.google.Chrome)
3135
;;
3236
flatpak-chrome-unstable)
33-
browser_exec="flatpak run com.google.ChromeDev"
37+
browser_exec=(flatpak run com.google.ChromeDev)
3438
;;
3539
flatpak-chromium)
36-
browser_exec="flatpak run org.chromium.Chromium"
40+
browser_exec=(flatpak run org.chromium.Chromium)
3741
;;
3842
flatpak-edge)
39-
browser_exec="flatpak run com.microsoft.Edge"
43+
browser_exec=(flatpak run com.microsoft.Edge)
4044
;;
4145
flatpak-ungoogled-chromium)
42-
browser_exec="flatpak run com.github.Eloston.UngoogledChromium"
46+
browser_exec=(flatpak run com.github.Eloston.UngoogledChromium)
4347
;;
4448
flatpak-firefox)
45-
browser_exec="flatpak run org.mozilla.firefox"
49+
browser_exec=(flatpak run org.mozilla.firefox)
4650
;;
4751
flatpak-librewolf)
48-
browser_exec="flatpak run io.gitlab.librewolf-community"
52+
browser_exec=(flatpak run io.gitlab.librewolf-community)
4953
;;
5054
*)
51-
browser_exec=$browser
55+
browser_exec=("$browser")
5256
;;
5357
esac
5458

5559

56-
mkdir -p ~/.bigwebapps/$browser
60+
mkdir -p "$HOME/.bigwebapps/$browser"
5761

5862
# Permission for flatpak browser
5963
if [[ $browser =~ flatpak ]]; then
60-
flatpak override --user --filesystem=$HOME/.bigwebapps/$browser ${browser_exec/flatpak run /}
64+
flatpak override --user --filesystem="$HOME/.bigwebapps/$browser" "${browser_exec[2]}"
6165
fi
6266

6367
if [[ $browser =~ (firefox|librewolf|flatpak-firefox) ]]; then
@@ -71,13 +75,13 @@ if [[ $browser =~ (firefox|librewolf|flatpak-firefox) ]]; then
7175
fi
7276

7377
# Execute the browser with the profile
74-
XAPP_FORCE_GTKWINDOW_ICON="${icon//*\/}" MOZ_APP_REMOTINGNAME="$class" exec $browser_exec --class="$class" --name="$filename" --profile "$HOME/.bigwebapps/$browser/$filename" --no-remote "$url"
78+
XAPP_FORCE_GTKWINDOW_ICON="${icon//*\/}" MOZ_APP_REMOTINGNAME="$class" exec "${browser_exec[@]}" --class="$class" --name="$filename" --profile "$HOME/.bigwebapps/$browser/$filename" --no-remote "$url"
7579

7680
fi
7781

7882
# If the big-webapp-version is set and using wayland, we change desktop file temporarily
79-
if echo $filename | grep -q '\-BigWebApp' && [[ $XDG_SESSION_TYPE == 'wayland' ]]; then
80-
filename_orig="$HOME/.local/share/applications/$(echo $filename | sed 's/-BigWebApp[0-9]*//g')"
83+
if grep -q '\-BigWebApp' <<< "$filename" && [[ $XDG_SESSION_TYPE == 'wayland' ]]; then
84+
filename_orig="$HOME/.local/share/applications/${filename//-BigWebApp[0-9]*/}.desktop"
8185
filename_orig_bkp="${filename_orig//.desktop/-bkp.desktop}-bkp"
8286
# Add folder path to filename
8387
filename="$HOME/.local/share/applications/$filename"
@@ -89,18 +93,32 @@ if echo $filename | grep -q '\-BigWebApp' && [[ $XDG_SESSION_TYPE == 'wayland' ]
8993

9094
# In wayland, we don't have option to set two different icons for same site
9195
# Because of that, we change the desktop file temporarily
92-
mv -f "$filename_orig" "$filename_orig_bkp"
93-
cp "$filename" "$filename_orig"
94-
# Wait to system detect updated icon
95-
sleep 2
96-
if [[ "$profile" == "Browser" ]]; then
97-
$browser_exec --no-default-browser-check --app="$url" &
96+
# lock → prevent race if two instances open simultaneously
97+
lockfile="${filename_orig}.lock"
98+
exec 9>"$lockfile"
99+
if flock -n 9; then
100+
mv -f "$filename_orig" "$filename_orig_bkp"
101+
cp "$filename" "$filename_orig"
102+
# Wait to system detect updated icon
103+
sleep 2
104+
if [[ "$profile" == "Browser" ]]; then
105+
"${browser_exec[@]}" --no-default-browser-check --app="$url" &
106+
else
107+
"${browser_exec[@]}" --no-default-browser-check --user-data-dir="$HOME/.bigwebapps/$browser/$profile" --app="$url" &
108+
fi
109+
110+
sleep 2
111+
mv -f "$filename_orig_bkp" "$filename_orig"
112+
flock -u 9
98113
else
99-
$browser_exec --no-default-browser-check --user-data-dir="$HOME/.bigwebapps/$browser/$profile" --app="$url" &
114+
# another instance holds lock → just launch without icon swap
115+
if [[ "$profile" == "Browser" ]]; then
116+
"${browser_exec[@]}" --no-default-browser-check --app="$url" &
117+
else
118+
"${browser_exec[@]}" --no-default-browser-check --user-data-dir="$HOME/.bigwebapps/$browser/$profile" --app="$url" &
119+
fi
100120
fi
101-
102-
sleep 2
103-
mv -f "$filename_orig_bkp" "$filename_orig"
121+
rm -f "$lockfile"
104122
else
105123

106124
# If have problem with the original file, we restore it
@@ -111,8 +129,8 @@ else
111129
fi
112130

113131
if [[ "$profile" == "Browser" ]]; then
114-
$browser_exec --no-default-browser-check --app="$url" &
132+
"${browser_exec[@]}" --no-default-browser-check --app="$url" &
115133
else
116-
$browser_exec --no-default-browser-check --user-data-dir="$HOME/.bigwebapps/$browser/$profile" --app="$url" &
134+
"${browser_exec[@]}" --no-default-browser-check --user-data-dir="$HOME/.bigwebapps/$browser/$profile" --app="$url" &
117135
fi
118136
fi

‎biglinux-webapps/usr/bin/big-webapps-viewer‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
CSD headerbar replicating GTK4/Adwaita style. Fullscreen auto-hide overlay.
44
"""
55

6-
APP_VERSION = "3.4.1"
6+
APP_VERSION = "3.4.2"
77

88
import argparse
99
import json

‎biglinux-webapps/usr/share/biglinux/webapps/get_app_icon_url.py‎

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,9 @@
33
import os
44
import json
55

6-
gi.require_version("Gtk", "3.0")
7-
from gi.repository import Gtk
6+
gi.require_version("Gtk", "4.0")
7+
gi.require_version("Gdk", "4.0")
8+
from gi.repository import Gtk, Gdk
89

910

1011
def get_icon_path(icon_name, icon_theme):
@@ -26,16 +27,25 @@ def get_icon_path(icon_name, icon_theme):
2627
if os.path.exists(path_with_ext):
2728
return path_with_ext
2829

29-
# Fall back to icon theme lookup
30+
# Fall back to icon theme lookup (GTK4 API)
3031
parts = icon_name.split("-")
3132
for end in range(len(parts), 0, -1):
3233
modified_icon_name = "-".join(parts[:end])
3334
for size in [64, 48, 128, 32, 256, 512, 24, 22, 16]:
34-
icon_info = icon_theme.lookup_icon(
35-
modified_icon_name, size, Gtk.IconLookupFlags.USE_BUILTIN
35+
paintable = icon_theme.lookup_icon(
36+
modified_icon_name,
37+
None,
38+
size,
39+
1,
40+
Gtk.TextDirection.NONE,
41+
Gtk.IconLookupFlags(0),
3642
)
37-
if icon_info:
38-
return icon_info.get_filename()
43+
if paintable:
44+
gfile = paintable.get_file()
45+
if gfile:
46+
path = gfile.get_path()
47+
if path:
48+
return path
3949

4050
return "Icon not found"
4151

@@ -44,7 +54,10 @@ def get_app_info_from_json(json_file):
4454
with open(json_file, "r") as file:
4555
apps_data = json.load(file)
4656

47-
icon_theme = Gtk.IconTheme.get_default()
57+
# GTK4 requires init + display for icon theme
58+
Gtk.init()
59+
display = Gdk.Display.get_default()
60+
icon_theme = Gtk.IconTheme.get_for_display(display)
4861
for app in apps_data:
4962
icon_name = app.get("app_icon", "")
5063
icon_path = get_icon_path(icon_name, icon_theme)

0 commit comments

Comments
 (0)