Fix generic taskbar/Activities icon on Linux: ship and install the .desktop launchers / fixes #1667

[asikarastallion]

Summary

Fixes the generic "gear" taskbar/Activities icon reported in #1667.

On GNOME/GTK3 the taskbar icon for a running window does not come from wx.Frame.SetIcon() at runtime. GNOME Shell matches the window's WM_CLASS against the StartupWMClass of an installed .desktop file and paints that file's Icon= — see wxWidgets/wxWidgets#24034.

MAVProxy had .desktop files in the repo, but nothing installed them (not even pip install), they carried no StartupWMClass, and the ReadMe pointed at ~/.icon, which GTK never searches. So there was nothing for GNOME to match, and the windows fell back to a generic icon.

Changes

Shipping and installing the launchers — the part that actually fixes #1667

• Move shortcuts/ → MAVProxy/data/shortcuts/, so it lives inside the package, with data/shortcuts/* added to package_data and MAVProxy.tools added to packages (plus MAVProxy/tools/__init__.py).

  Without this, neither the helper nor the files it installs existed in a pip install: a wheel built from the previous head had a MAVProxy/tools/ holding only graphs/*.xml, and no .desktop or .png anywhere in the archive. The script degraded quietly rather than erroring, so the feature was simply inert for pip users — which is who the ReadMe addresses.

• MAVProxy/tools/install_desktop_icons.py — new opt-in helper:

  python3 -m MAVProxy.tools.install_desktop_icons
  

  It copies the .desktop files to $XDG_DATA_HOME/applications and the icons to $XDG_DATA_HOME/icons/hicolor/256x256/apps (the standard freedesktop icon theme location; the ~/.icon path the old ReadMe documented is never searched), then best-effort refreshes the desktop and icon caches. It resolves the packaged files with importlib.resources.files('MAVProxy') rather than a path walked up from __file__, honours $XDG_DATA_HOME (ignoring a relative value, as the XDG basedir spec requires), refuses to run on win32/cygwin/darwin instead of scattering files, returns real exit codes, and skips any icon whose PNG header is not 256×256.

  It is not run automatically by pip install, since pip cannot reliably write to XDG user directories — especially into a venv, as in the original report.

• StartupWMClass on all three launchers. The values are the WM_CLASS instance names wxWidgets derives from each script's name, probed against a live wxPython/GTK3 window opened by a script of that name:

  launcher	WM_CLASS	StartupWMClass
  mavproxy.desktop	"mavproxy.py", "Mavproxy"	mavproxy.py
  mavexplorer.desktop	"MAVExplorer.py", "Mavexplorer"	MAVExplorer.py
  mavpicviewer.desktop	"mavpicviewer.py", "Mavpicviewer"	mavpicviewer.py

• MAVProxy/data/shortcuts/ReadMe.md: corrected install instructions, points at the helper, documents why an installed .desktop file is needed at all, and notes that GNOME Shell may need a restart (Alt+F2 → r on X11, log out/in on Wayland) to pick up a newly-installed .desktop file.

• MAVProxy/data/shortcuts/mavpicviewer.png: was 245×256 while being installed into the 256x256 theme directory. Padded symmetrically with transparency, so the artwork is unchanged; re-encoding also took it from 124KB to 104KB.

Window icons

• MAVProxy/modules/lib/icon.py: add a "WP" icon (dashed route with numbered waypoint markers) to SimpleIcon.
• MAVProxy/modules/mavproxy_misseditor/missionEditorFrame.py: MissionEditorFrame never called SetIcon(), unlike ConsoleFrame and the Map window. It now sets the "WP" icon using the same try/except pattern wxconsole_ui.py uses, with the icon import inside the try so matplotlib stays optional — this file already defers its own matplotlib import in height_profile_pushed() for that reason.

What this does and does not achieve

MAVProxy's console, map and mission editor windows all run in multiprocessing.Process children forked from mavproxy.py, so they inherit its WM_CLASS — a forked child's window reports the identical WM_CLASS(STRING) = "mavproxy.py", "Mavproxy". GNOME therefore groups all of them under mavproxy.desktop and paints Icon=mavproxy for every one.

So the fix for the reported gear icon is installing the launcher with a matching StartupWMClass and putting the icons in the theme directory GTK actually searches — not the per-window SetIcon() calls. Those are still worth having, since _NET_WM_ICON is used elsewhere (window lists, other desktops such as KDE), but they are not what changes the GNOME taskbar. The ReadMe states this explicitly so a user reading it isn't misled.

Verification

• WM_CLASS values probed with xprop against live wxPython/GTK3 windows, and against a multiprocessing.Process child to confirm the forked-window behaviour above.
• Wheel built and installed into a clean venv, then python3 -m MAVProxy.tools.install_desktop_icons run from outside the checkout, so the source tree could not satisfy the import: the module resolves to site-packages, and all six files install under $XDG_DATA_HOME.
• scripts/run_flake8.py MAVProxy passes with a plain pip install flake8, matching the workflow. install_desktop_icons.py carries an AP_FLAKE8_CLEAN marker so it is actually checked. The marker is deliberately not added to icon.py or missionEditorFrame.py, which have substantial pre-existing violations (405 in missionEditorFrame.py) — that would be a separate cleanup.
• Tested on Ubuntu 24.04 / GNOME (X11): before, the windows showed the generic gear icon in Alt-Tab; after running the installer and restarting GNOME Shell, they showed the MAVProxy icon.

Known, pre-existing, deliberately not touched here

desktop-file-validate fails on all three launchers' Exec= lines (reserved ;, ' and ~ outside quotes, an unescaped $, and %F splitting filenames containing spaces). Those lines are byte-identical to the ones at this PR's base — this PR only adds a StartupWMClass line to each file — and fixing them changes launch behaviour, so they belong in their own change. Likewise, importlib.resources.files() needs Python 3.9 while setup.py declares no python_requires; the floor is not introduced here, since MAVProxy/tools/MAVExplorer.py already calls the same API.

---

AI assistance disclosure: this contribution was developed with the assistance of Claude (Anthropic), including Claude Code — investigating the root cause, drafting the changes, and preparing this description. I reviewed the diff, understand the change, and ran the verification described above on my own machine.

Fixes #1667

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[tridge]

Deprecated — see below for the updated review.

Previous review

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.
Full report: https://uav.tridgell.net/DevCallReviews/2026_09_01_AIReview/devcall_pr_reviews.html#prMAVProxy-1710

Reviewed at head 6dc1e356a1. REQUEST CHANGES — but the icon work itself is right; this is about packaging.

Good news first, because it's the part most likely to be silently wrong and it isn't: StartupWMClass=mavproxy.py is correct. Probing a real wxPython/GTK3 window from a script named mavproxy.py gives WM_CLASS(STRING) = "mavproxy.py", "Mavproxy", and gnome-shell matches StartupWMClass against res_class then res_name, so it hits on the instance name. The icon.py and missionEditorFrame.py changes also follow the existing project pattern exactly.

The blocker: the command your ReadMe tells users to run doesn't exist in a pip install. setup.py's packages=[...] doesn't list MAVProxy.tools, and no package_data glob covers tools/*.py. I built and installed this head into a clean venv: site-packages/MAVProxy/tools/ contains only graphs/, and import MAVProxy.tools.install_desktop_icons raises ModuleNotFoundError. It works only from a git checkout, which isn't who the ReadMe addresses.

And even if it were importable, shortcuts/ isn't shipped either. SHORTCUTS_DIR resolves three dirnames up from MAVProxy/tools/ — the repo root — which under a pip install is site-packages/shortcuts. There's no MANIFEST.in at this head and shortcuts/ sits outside the MAVProxy package, so no .desktop or .png is installed anywhere in the venv. The script degrades quietly ("shortcuts directory not found") rather than erroring, so the feature is simply inert for pip users.

The fix is small: move shortcuts/ under the package (e.g. MAVProxy/data/shortcuts/), add it to package_data, add MAVProxy.tools to packages, and resolve SHORTCUTS_DIR relative to the package rather than the repo root.

Two things that affect what this actually achieves. The Mission Editor icon won't appear in the GNOME taskbar regardless: console, map and mission editor all run as multiprocessing.Process children forked from mavproxy.py, and I probed a forked child's window — identical WM_CLASS — so GNOME groups them all under mavproxy.desktop and paints Icon=mavproxy. The per-window SetIcon still shows where _NET_WM_ICON is used, so the change isn't wrong; it just doesn't do what the title implies for that window. And mavexplorer.desktop/mavpicviewer.desktop didn't get a StartupWMClass line, so the claim about MAVExplorer is only half-delivered. It's also possible the real fix for #1667 is your ReadMe correction from ~/.icon to ~/.local/share/icons/hicolor/256x256/apps, since res_class Mavproxy already lowercases onto mavproxy.desktop via GNOME's existing fallback.

Worth knowing: no CI ran on this PR at all. check-runs for the head is total_count: 0, while sibling PRs have 2. Both workflows trigger on bare [push, pull_request] with no path filters, so it isn't path filtering — they were never approved for the fork branch. Running flake8 locally against the head passes, though note run_flake8.py only checks files carrying an AP_FLAKE8_CLEAN marker and the new files have none.

Minor: the installer hardcodes ~/.local/share rather than honouring $XDG_DATA_HOME; it has no platform guard, so running it on Windows would scatter files; mavpicviewer.png is 245×256 but installed into the 256x256 directory; and the new top-level import makes the mission editor a hard consumer of matplotlib, which it wasn't before.

[asikarastallion]

Thanks — the packaging blocker was exactly right, and I reproduced it before changing anything. Building the previous head (6dc1e35) into a wheel gave a MAVProxy/tools/ containing only graphs/*.xml, and zero shortcuts entries anywhere in the archive. The feature was inert for precisely the users the ReadMe addresses.

Pushed as four commits on top of 6dc1e35.

1. Packaging — c570fc2

Done the way you laid out:

• shortcuts/ → MAVProxy/data/shortcuts/, with data/shortcuts/* added to package_data
• MAVProxy.tools added to packages, plus an __init__.py
• SHORTCUTS_DIR replaced with importlib.resources.files('MAVProxy').joinpath('data').joinpath('shortcuts') rather than a path walked up from __file__. That also stays correct under the zip_safe=True this setup declares, and matches how MAVExplorer.py already reaches MAVProxy.tools.graphs.

Verified against a wheel installed into a clean venv, run from outside the checkout so the source tree could not satisfy the import:

$ .../cleanvenv/bin/python -c "import MAVProxy.tools.install_desktop_icons as m; print(m.__file__)"
.../cleanvenv/lib/python3.12/site-packages/MAVProxy/tools/install_desktop_icons.py

$ XDG_DATA_HOME=/tmp/x .../cleanvenv/bin/python -m MAVProxy.tools.install_desktop_icons
Installed /tmp/x/applications/mavexplorer.desktop
Installed /tmp/x/icons/hicolor/256x256/apps/mavexplorer.png
... 6 files, exit 0

2. MAVExplorer / MAVPicViewer — d4c45e4

You're right that this was half-delivered; both launchers now carry the key. I probed the values the same way you did, against a real wxPython/GTK3 window opened by a script of each name. mavproxy.py reproduced your result exactly, which is my control that the method matches yours:

script	WM_CLASS	StartupWMClass
mavproxy.py	"mavproxy.py", "Mavproxy"	mavproxy.py
MAVExplorer.py	"MAVExplorer.py", "Mavexplorer"	MAVExplorer.py
mavpicviewer.py	"mavpicviewer.py", "Mavpicviewer"	mavpicviewer.py

3. Icon size — c8b4c8e

mavpicviewer.png padded symmetrically to a real 256×256 (artwork unchanged; re-encoding also took it from 124KB to 104KB). The installer now reads each PNG's IHDR and skips anything that isn't 256×256, so a wrongly-sized icon can't silently land in a directory claiming that size again.

4. matplotlib — c8e1ba9

Good catch. missionEditorFrame.py already defers its own matplotlib import in height_profile_pushed() and says why in a comment, so the module-scope icon import was working against the file's own intent. Moved inside the existing try:. Confirmed 'matplotlib' in sys.modules is False after importing the module.

On the forked children

Confirmed independently — a multiprocessing.Process child forked from a script named mavproxy.py reports the identical WM_CLASS(STRING) = "mavproxy.py", "Mavproxy". So GNOME does group console, map and mission editor under mavproxy.desktop and paints Icon=mavproxy for all of them.

I've retitled the PR accordingly, and the ReadMe now states this explicitly rather than implying the per-window SetIcon() drives the taskbar. The SetIcon() calls stay because _NET_WM_ICON is still used elsewhere (window lists, other desktops), but they are not what fixes #1667 — as you suggested, the ~/.icon → ~/.local/share/icons/hicolor/256x256/apps correction, plus actually installing the launcher, is.

Minor points

• $XDG_DATA_HOME is honoured, with relative values ignored per the XDG basedir spec.
• Platform guard: returns non-zero on win32/cygwin/darwin instead of scattering files.
• The script now returns real exit codes rather than degrading quietly.

flake8

Added AP_FLAKE8_CLEAN to install_desktop_icons.py; scripts/run_flake8.py MAVProxy picks it up and passes with a plain pip install flake8, matching the workflow. I left the marker off icon.py and missionEditorFrame.py — both carry substantial pre-existing violations (405 in missionEditorFrame.py), so marking them would be a separate cleanup. CI still appears not to be approved to run on this fork branch.

One thing I did not touch

desktop-file-validate reports pre-existing Exec= quoting errors in all three launchers, including the untouched mavproxy.desktop (reserved ;, ' and ~ outside quotes). It predates this PR and fixing it changes launch behaviour, so it felt out of scope — happy to do it here or separately if you'd prefer.

The PR description above still describes the original shortcuts/… layout; say the word and I'll refresh it to match.

AI assistance disclosure, as before: Claude Code helped investigate and prepare these changes; I reviewed the diff and ran the verification above on my own machine.

[tridge]

Deprecated — see below for the updated review.

Previous review

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_04_2335/devcall_pr_reviews.html#prMAVProxy-1710

Re-reviewed at head c8e1ba9978; my earlier comment above is superseded. Both packaging blockers are fixed and everything else from last round with them — APPROVE.

I re-ran the same test that produced the blockers rather than reading setup.py: build the head, pip install --no-deps . into a clean venv, look at what's actually there.

• MAVProxy.tools is in packages and MAVProxy/tools/__init__.py exists, so import MAVProxy.tools.install_desktop_icons works from a venv.
• shortcuts/ moved inside the package to MAVProxy/data/shortcuts/ with data/shortcuts/* in package_data, old top-level directory gone. All seven files land in site-packages/MAVProxy/data/shortcuts/. A cross-check reproduced this from both a wheel and a release-style sdist.
• The path resolution is better than what I suggested. shortcuts_dir() uses importlib.resources.files('MAVProxy')..., and run from a neutral cwd it resolves into site-packages, not the source tree — I checked that specifically, since running it from inside a checkout gives the flattering answer either way.
• StartupWMClass is now on all three .desktop files. A cross-check probed live wxPython 4.2.4/GTK3 windows and confirmed the values match what each program produces (mavproxy.py, MAVExplorer.py, mavpicviewer.py).
• $XDG_DATA_HOME honoured including the spec's ignore-if-relative rule, platform guard present, all three PNGs now 256×256 (mavpicviewer.png was 245×256 going into the 256x256 directory), and matplotlib is no longer a hard dependency of the mission editor — the icon import moved inside __init__ in a try/except with a comment saying why.
• The taskbar limitation is documented rather than implied. That was the one point last round the change couldn't fix, only explain, and the new ReadMe explains it where a user will find it.

Two things still open, neither yours to fix

No CI has run on this PR at all. check-runs for the head still reports total_count: 0 while sibling PRs have 2. Both workflows trigger on bare [push, pull_request] with no path filters, so it's fork-branch approval rather than filtering. Worth a maintainer clicking approve before merge, since nothing here has been machine-checked.

The .desktop files fail desktop-file-validate — but they already did. I ran it: all three error on reserved characters in Exec= (single quotes, ~, an unescaped $ in the two that set PATH). A cross-check also traced %F splitting a filename containing spaces into extra bash -c arguments, and noted "$PATH:~/.local/bin" appends a literal tilde because it's inside double quotes. All real and worth fixing — but the Exec= lines are byte-identical to the ones at this PR's base, which only added a StartupWMClass line to each file. Its own change, not a condition on this one. (A cold pass called it blocking; I'm not carrying that across, for that reason.)

A cross-check also flagged that importlib.resources.files() needs Python 3.9 while MAVProxy declares no python_requires. The API requirement is right, but this PR doesn't introduce the floor — MAVProxy/tools/MAVExplorer.py:390 already calls it. The project is 3.9+ in practice and would benefit from declaring it, again separately.

Cosmetic: mavexplorer.desktop and mavproxy.desktop are mode 100755 in git while mavpicviewer.desktop is 100644. The installer chmods what it copies, so it only matters for the documented manual "copy to Desktop" route.

[asikarastallion]

Two follow-ups since your re-review at c8e1ba9, both non-functional:

• 007bbb0 sets mavpicviewer.desktop to mode 100755, matching the other two launchers — the cosmetic point you raised. Mode only; the file's bytes are unchanged.
• Refreshed the PR description, which still described the old top-level shortcuts/ layout. It now also records the two items you flagged as out of scope — the pre-existing Exec= quoting and the undeclared python_requires — so they aren't lost when this is read later.

Head is now 007bbb0. No code changed, so your verification at c8e1ba9 still holds for all of it.

CI still reports check-runs: total_count 0 on the new head, so the fork-branch approval does still look like it's needed before this can merge.

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-09-04)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Full report: https://uav.tridgell.net/DevCallReviews/2026_09_05/devcall_pr_reviews.html#prMAVProxy-1710

Re-reviewed at head 007bbb0f48; my earlier comment above is superseded. APPROVE, unchanged.

The entire delta since the last review is one mode bit: every blob under MAVProxy/data/shortcuts/ is byte-identical, and the only difference is mavpicviewer.desktop going 100644 → 100755 — the cosmetic inconsistency raised last round, and nothing else.

A cross-check pass added a useful qualification: it has no effect on helper-based installation, because install_desktop_icons.py writes every desktop file as 0755 regardless. It matters only for the documented manual "copy to Desktop" route, which is where it was raised. Both passes agree the APPROVE stands.

The two things noted last round as open but not yours to fix are unchanged: no CI has ever run on this PR (check-runs total_count is still 0 while sibling PRs have 2, because the workflows trigger on bare [push, pull_request] and the fork branch has never been approved), and the Exec= lines still fail desktop-file-validate on reserved characters — byte-identical to the PR's base, so pre-existing.

Worth a maintainer approving the workflow run before merge, since nothing here has ever been machine-checked.

[AP-Review]

Deprecated — see below for the updated review.

Previous review (2026-09-17)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Full report: https://uav.tridgell.net/DevCallReviews/2026_09_17_AIReview/devcall_pr_reviews.html#prMAVProxy-1710

Re-reviewed at head 56547f36ab; my earlier comment above is superseded. (That comment was posted under a different account, so this bot cannot mark it deprecated in place — please read this one as replacing it.)

Good news first: CI has now run, and it's green. That was the one blocking item from the last round. This head is a pure rebase — confirmed by blob SHA on all ten files and by git range-diff marking all eight commits = — so your own content is unchanged, but the rebase triggered the workflows: build and build (3.10) both pass. I re-ran the full verification anyway rather than carrying it forward, since the rebase brought in a new setup.py.

Verdict: COMMENT. One thing worth a decision before merge.

I got something wrong last round — please ignore it

I claimed the launchers' export PATH="$PATH:~/.local/bin" line is a no-op because the ~ is inside double quotes and never expands. That's wrong. Bash expands a literal tilde in PATH during command lookup:

$ HOME=/tmp/x bash -c 'export PATH="/usr/bin:~/.local/bin"; command -v fakeprog.py'
/tmp/x/.local/bin/fakeprog.py

The PATH extension does exactly what it's meant to. Sorry for the noise — a second reviewer caught it and I verified the correction directly.

The one open question

ISSUE — MAVProxy/data/shortcuts/mavpicviewer.desktop:5 — this installs a menu entry for a program that won't start in a wheel install. Exec= runs mavpicviewer.py, which is installed as a bare script into bin/, but its module-scope imports are sibling imports (mavpicviewer.py:14 import mavpicviewer_image, :15 import mavpicviewer_mosaic) and MAVProxy.tools.mavpicviewer isn't in setup.py's packages, so those modules ship nowhere:

$ <cleanvenv>/bin/python <cleanvenv>/bin/mavpicviewer.py --help
  File ".../bin/mavpicviewer.py", line 14, in <module>
    import mavpicviewer_image
ModuleNotFoundError: No module named 'mavpicviewer_image'

Same in the released PyPI wheel. The contrast confirms the diagnosis: MAVExplorer.py's one bare sibling import (import mavflightview) resolves fine because mavflightview.py is in scripts= and lands in the same bin/ — so that launcher is genuinely OK. It's specifically mavpicviewer.

The root cause is pre-existing and not yours — before this PR, shortcuts/mavpicviewer.desktop sat in a top-level directory that no artifact shipped, and the released wheel contains no .desktop at all. But this PR is what makes it user-visible: someone runs pip install MAVProxy, follows your ReadMe, sees "MAVPicViewer" in the GNOME app grid, clicks it, and the terminal flashes a ModuleNotFoundError and closes. (Scope correction from a second reviewer: this is the wheel/sdist path — a source checkout works fine, so "every pip install" as I'd have put it is too broad.)

Two ways out, both small: add MAVProxy.tools.mavpicviewer to packages and make those two imports absolute, or leave that one launcher out until mavpicviewer is packaged. Either is fine; shipping it as-is is the only thing I'd push back on.

Does it fix #1667? Yes

The mechanism is confirmed, and I specifically re-checked the part most likely to have rotted: Python 3.14 changed the default Linux multiprocessing start method from fork to forkserver, and every window in #1667 is a multiprocessing child — so if the start method changed WM_CLASS, StartupWMClass=mavproxy.py would stop matching and this PR would silently stop working. It doesn't. On Python 3.14.4 / wxPython 4.3.1 / GTK3, a script named mavproxy.py creating live wx windows in children reports WM_CLASS = ("mavproxy.py", "Mavproxy") under fork, forkserver and spawn — I checked via GDK, and a second reviewer confirmed with an actual xprop read on a mapped window. GNOME Shell matches that instance name against StartupWMClass first, which your launchers now set.

Install paths, precisely: pip install + running the helper works end to end (verified in a clean venv, running the installer from / so the source tree couldn't satisfy the import); pip install alone changes nothing, which is the correct call since pip can't write XDG dirs from a venv, and you document it; distro packagers can use the sdist copies; macOS/Windows are guarded with no files written.

Notes

• MAVProxy/tools/install_desktop_icons.py:100-104 — gtk-update-icon-cache fails on every fresh user icon dir (No theme index file., rc=1, because index.theme comes from the system hicolor-icon-theme package) and check=False swallows it while the script prints "Done." Not a functional bug — GTK resolves the icons without a cache, confirmed. But since the ReadMe already warns "if it doesn't appear right away", either dropping the call or surfacing its failure would stop a genuinely failed refresh being indistinguishable from a no-op.
• MAVProxy/data/shortcuts/ReadMe.md:6 is the only mention of the installer anywhere in the tree — nothing in README.md or docs/. After pip install that file lives in site-packages, so the user in Taskbar icons not present on Ubuntu #1667, who hit this running sim_vehicle.py from a venv, has no way to discover the fix exists. One line in the repo README or the wiki would close it.
• install_desktop_icons.py:63-66 returns 1 on win32/cygwin/darwin for a correct no-op, so a packaging step running it unconditionally on macOS sees a failure. Noting that you've explicitly documented this as intentional in the thread, so treat it as a caller contract rather than a defect.
• Exec= quoting (pre-existing, unchanged from your base, carried forward at your request so it isn't lost): desktop-file-validate 0.28 gives four errors per launcher. The filename bug is real — confirmed through GIO with the unchanged Exec text, log with spaces.bin is split and the app receives only the fragment ending in log, because %F sits inside the single-quoted bash -c script.

Everything you fixed in the two earlier rounds I re-verified empirically at this head rather than by reading the diff, and all of it holds: MAVProxy.tools in packages, shortcuts/ inside the package and shipped, StartupWMClass on all three launchers, mavpicviewer.png a true 256×256, matplotlib no longer a hard dep, $XDG_DATA_HOME honoured, platform guard, real exit codes. Wheel and sdist both contain all seven files under MAVProxy/data/shortcuts/; there's no MANIFEST.in and none is needed, since setuptools carries package_data into the sdist. The data_files venv-prefix trap doesn't apply here because you used package_data plus a manual helper — nothing lands in <venv>/share/applications. The new MAVProxy/tools/__init__.py causes no regression: importlib.resources.files("MAVProxy.tools.graphs") still resolves and finds its 4 XML files.

[asikarastallion]

AP-Review identified that the newly packaged MAVPicViewer desktop launcher could not start from a wheel installation because mavpicviewer.py was installed without its sibling helper modules. I reproduced the failure from a clean wheel install.

Fixed in 28b1986c: MAVProxy.tools.mavpicviewer is now packaged, and its intra-package imports use package-qualified names. I rebuilt and inspected both the wheel and sdist, confirmed that all MAVPicViewer modules are present, and verified clean installed-package module resolution from site-packages.

The desktop icon installer remains unchanged and still installs all three launchers and icons successfully with an isolated XDG_DATA_HOME. The new CI workflow runs are currently awaiting maintainer approval.

[AP-Review]

Deprecated — see below for the updated review.

Previous review (2026-09-19)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.
Verdict: COMMENT

Full report: followups/2026_09_20_0112

Re-reviewed at head 28b1986cf7; my earlier comment above is superseded.

Your fix landed — the one open issue from last round is resolved, and I verified it by building and installing rather than by reading setup.py. Verdict stays COMMENT.

RESOLVED — the MAVPicViewer packaging issue

You fixed it both ways round, which is what makes it work: setup.py:116 adds MAVProxy.tools.mavpicviewer to packages, the new __init__.py makes it a real package, and the three sibling imports became absolute.

Evidence — wheel contents at each head:

	old head 56547f36ab	this head 28b1986cf7
MAVProxy/tools/mavpicviewer/*.py	(absent)	all six modules present
mavproxy-…data/scripts/mavpicviewer.py	present	present

And the launcher itself, installed into a clean venv and run from / so the source tree could not satisfy the import:

old head  → ModuleNotFoundError: No module named 'mavpicviewer_image'
this head → (that error is gone)

I also checked the things your commit didn't explicitly name and they're all fine: mavpicviewer_settings and mavpicviewer_shared are reachable too, the sdist is fixed as well as the wheel, there are no remaining bare sibling imports anywhere in the tree, and importlib.resources.files("MAVProxy.tools.graphs") still resolves its 4 XML files. I specifically checked whether mavpicviewer.py being both a scripts= entry and a package module causes shadowing or a double import — it doesn't; helper identities and shared-object pickle compatibility survive.

One new thing, which only became visible because the fix landed

ISSUE — the menu entry still doesn't start after a plain pip install MAVProxy. The packaging error is gone, so the import now runs one step further and stops on the next module-scope dependency:

this head, wheel installed with exactly its declared dependencies:
  ModuleNotFoundError: No module named 'piexif'
…and after adding piexif:
  ModuleNotFoundError: No module named 'wx'

mavpicviewer_image.py:15 imports piexif at module scope and mavpicviewer_mosaic.py:14 imports wx.lib.scrolledpanel at module scope, but both piexif and wxpython are in the recommended extra only. (opencv-python is in the base requirements via MAP3D_MARKER, so cv2 is fine.)

This is adjacent to your change rather than caused by it — those imports predate it. But it means the user story from the original finding still ends the same way: install, click MAVPicViewer in the app grid, terminal flashes a ModuleNotFoundError and closes.

Worth noting mavpicviewer_image.py already guards its own wx imports behind mp_util.has_wxpython while mavpicviewer_mosaic.py doesn't, so the two files are inconsistent. Either guard them the way _image already does, or document MAVProxy[recommended] as the requirement for that launcher. Your call which — I'm not going to push back on shipping it either way now that the packaging is right.

NOTE — CI no longer reports at this head. Last round's headline was that the rebase had finally triggered the workflows and both jobs were green. gh pr checks 1710 now says no checks reported on the fix/taskbar-icons-1667 branch, and statusCheckRollup is empty. So that green result no longer covers the current head — probably just needs a maintainer to approve the run.

The four notes from last round — unchanged, none blocking

Your commit didn't touch any of these, and I re-verified rather than assuming:

• install_desktop_icons.py:102 — gtk-update-icon-cache on a fresh user icon dir still returns rc 1 (No theme index file.) while the installer returns 0. Still not a functional bug; GTK resolves the icons without a cache.
• ReadMe discoverability — MAVProxy/data/shortcuts/ReadMe.md and the root README.md are byte-identical to the previous head, so the installer is still only documented in a file that lands in site-packages.
• install_desktop_icons.py:65 returning 1 on win32/darwin — accepted, not re-raising. You've explained in the thread that this is intentional, and I'm treating it as a caller contract.
• Exec= quoting / %F — all launcher files byte-identical to the previous head; re-reproduced through GIO at this head, image with spaces.jpg still arrives as just the fragment ending in /image. Pre-existing, carried forward at your request so it isn't lost.

[asikarastallion]

Friendly ping: the current head (28b1986) has been ready since Sep 19, but the Windows Build and test Python cleanliness workflow runs are still awaiting maintainer approval. When convenient, could someone approve the workflow runs so CI can proceed? Thanks!

[peterbarker]

@asikarastallion I've marked the workflows to run, but the fact that tridge force-pushed and then you force-pushed means I'm unlikely to merge this one myself.

[tridge]

@asikarastallion apologies, needs rebase

[AP-Review]

Deprecated — see below for the updated review.

Previous review (2026-10-04)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.
Verdict: COMMENT

Reviewed at head 696d476558.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/mavproxy/1710/1.html#prMAVProxy-1710

Re-reviewed at 696d4765, the rebase onto current master. Nothing you wrote changed in the rebase: 7 of 9 commits are identical, and the other two only move the Mission Editor icon block, which still sits correctly after CreateStatusBar(). The mission-editor tests pass. Verdict: COMMENT. Nothing blocking.

Still fixed: the MAVPicViewer packaging. The rebuilt wheel and sdist ship all six helper modules, and the installed launcher loads them from site-packages.

Two small documentation points, worth doing in this PR:

• The MAVPicViewer launcher still exits with ModuleNotFoundError when the recommended extra isn't installed. piexif (https://github.com/ArduPilot/MAVProxy/pull/1710/files#diff-ce2bb599e9f1df84e8093e0ad36894eb5fd8ff1a733407a58f9dac28b7fa8585R15) and wx.lib.scrolledpanel (https://github.com/ArduPilot/MAVProxy/pull/1710/files#diff-133ac95e3d8fb411918c3d3469b8c2484e470428ebe85892193bc478e84e5dbfR14) are imported at module scope. That predates your change. I'd correct last round's suggestion: guarding the imports alone won't make the viewer run, because it creates wx and MPImage objects unconditionally. A line in the shortcuts ReadMe saying this launcher needs pip install MAVProxy[recommended] (or wxPython + piexif) is the simplest fix.
• The launchers find the tools through the desktop session's PATH, and mavproxy.desktop doesn't even append ~/.local/bin (https://github.com/ArduPilot/MAVProxy/pull/1710/files#diff-eceb9d8a9868573db63df371b2829a6d306455526e3e75891238ccf0d6482a80R5). So if MAVProxy is in a plain venv, which your ReadMe mentions, the menu entry fails with command not found (exit 127). The fix is a note saying the scripts must be on the session PATH, e.g. a pip --user/pipx install.

Not for this PR, carried forward so it isn't lost: %F inside the quoted bash -c still cuts filenames at the first space. A real GIO launch of log with spaces.bin delivered only .../log (https://github.com/ArduPilot/MAVProxy/pull/1710/files#diff-daf22fbe64fbfb8a93ca2d437130f6d896d50b37e7f056ed71f5ecc39cf621deR5). This predates the PR, as you noted. The gtk-update-icon-cache exit 1 on a fresh icon directory is fine as best effort. The root README not mentioning the installer is optional (the installer's docstring does). The win32/darwin return 1 stays accepted.

CI: the snapshot shows no checks for 696d4765, so the earlier green runs don't cover the rebased head. I didn't query live CI. Live GNOME taskbar behaviour and a fresh install with exactly the declared dependencies weren't tested either.

[AP-Review]

Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting.
Verdict: COMMENT

Reviewed at head 6e4310d06d.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/mavproxy/1710/2.html#prMAVProxy-1710

Re-reviewed at 6e4310d0. Since my last comment at 696d4765 you've added one docs commit, and the earlier nine commits are unchanged. Thanks, the ReadMe now covers both points I raised: the desktop-session PATH and the recommended extra for MAVPicViewer. The packaging fix still holds. Nothing blocking.

One small wording point on the new text:

• The ReadMe now suggests pipx install MAVProxy (https://github.com/ArduPilot/MAVProxy/pull/1710/files#diff-c9c90e6e9a33727b137c0462fabe472bb74c340b6d68260e105f46c8372cc5b9R24), but its two commands both run the terminal's plain python3: the installer at https://github.com/ArduPilot/MAVProxy/pull/1710/files#diff-c9c90e6e9a33727b137c0462fabe472bb74c340b6d68260e105f46c8372cc5b9R6 and the extras install at https://github.com/ArduPilot/MAVProxy/pull/1710/files#diff-c9c90e6e9a33727b137c0462fabe472bb74c340b6d68260e105f46c8372cc5b9R32. pipx keeps MAVProxy in its own venv. With a pipx-only install, python3 -m MAVProxy.tools.install_desktop_icons fails with No module named MAVProxy, and python3 -m pip install 'MAVProxy[recommended]' never reaches the venv that mavpicviewer.py runs from, so the viewer still stops on piexif/wx. Running the helper with the pipx venv's Python works (all six files installed). Either add pipx-specific commands (e.g. pipx install 'MAVProxy[recommended]', and run the helper with the Python MAVProxy is installed in) or drop the pipx suggestion. On recent Ubuntu/Debian, system pip also refuses both line 32 and pip install --user MAVProxy with externally-managed-environment.

Not for this PR, as before: %F inside the quoted bash -c (https://github.com/ArduPilot/MAVProxy/pull/1710/files#diff-daf22fbe64fbfb8a93ca2d437130f6d896d50b37e7f056ed71f5ecc39cf621deR5) predates your change.

Not tested: CI (the snapshot shows no checks for 6e4310d0), a full pipx install 'MAVProxy[recommended]' with a working viewer, and live GNOME taskbar behaviour.