Update shortcut removal to rely on menuinst.toml by lrandersson · Pull Request #1 · lrandersson/menuinst

lrandersson · 2026-05-14T21:36:13Z

Description

Use recorded paths from menuinst.toml for removal instead of recomputing from JSON. Falls back to computed paths for legacy installs.

`api.py`

Added get_recorded_paths(), delete_paths()
remove() now checks TOML first, falls back to computed paths. I wasn't sure if it's totally safe to remove the "old code", I was thinking that there might be a scenario where packages are installed, then menuinst is updated to a version with menuinst.toml, but the shortcuts installed before the update are not tracked in the .toml file.
install() records MenuItem paths (not Menu paths) to TOML

Platform files (`win.py`, `osx.py`, `linux.py`)

Split remove() into delete_paths() + cleanup_side_effects(). When using TOML paths, api.remove() handles file deletion directly (it has the TOML data). But platform-specific cleanup (registry, LaunchServices, MIME etc.) still needs the MenuItem objects. Separating these two lets api.py delete files from TOML data while platforms handle their own side-effects.
Original remove() preserved (calls both)

Removal Flow

To simplify the review, this is the removal flow:

api.remove()
 ├─ Read TOML paths for source
 │   ├─ Found → delete_paths(toml_paths)
 │   └─ Not found → fallback to old approach: menu_item.delete_paths()
 ├─ menu_item.cleanup_side_effects() (registry, LaunchServices, MIME)
 ├─ menu.remove() (directory cleanup)
 └─ remove_shortcut_records() from TOML

Checklist - did you ...

Add a file to the news directory (using the template) for the next release's release notes?
Add / update necessary tests?
Add / update outdated documentation?

marcoesters · 2026-05-15T17:31:43Z

+    """Get recorded shortcut paths for a source from menuinst.toml."""
+    data = read_menuinst_toml(prefix)
+    shortcuts = data.get("shortcuts", [])
+    return [s["path"] for s in shortcuts if s.get("source") == source and "path" in s]


Should they be sources be converted into Path objects to not depend on string shenanigans? Or maybe even use Path.same_file()?

I think the idea is good but here we are unfortunately comparing filename strings (basename) and not the actual path on disk so unfortunately Path.samefile is not applicable.

Maybe not samefile, but converting the strings into Path objects for comparison may be more robust.

Yeah, so source here is just a filename (a basename), there is no path involved so normalization of slashes is not necessary, so we would have the same:

# Current "foo.json" == "foo.json" # With Path Path("foo.json") == Path("foo.json")

I wasn't able to find any indication that source is an actual path.

marcoesters · 2026-05-15T17:37:51Z

Before I review this in detail, I have a more general question: you use the API-level removal function to remove the shortcuts if loaded from TOML. Have you considered to instead just load the paths from the TOML file? Maybe as an optional input parameter for _paths() or as an optional input parameter for the __init__ methods?

That way, we don't have to touch any of the existing removal methods and keep everything located inside the classes.

lrandersson · 2026-05-15T18:47:54Z

Before I review this in detail, I have a more general question: you use the API-level removal function to remove the shortcuts if loaded from TOML. Have you considered to instead just load the paths from the TOML file? Maybe as an optional input parameter for _paths() or as an optional input parameter for the __init__ methods?

That way, we don't have to touch any of the existing removal methods and keep everything located inside the classes.

I did consider that approach (or something very similar) but the reason why I decided not to go with that is because:

The __init__ functions would need TOML knowledge (MenuItem constructor would need to know whether to use computed or TOML paths, adding some kind of coupling between platform classes and the TOML format which seems risky).
Right now api.py is the "coordinator" that knows about TOML.

The platform classes know how to create or delete shortcuts at computed locations.
I couldn't find any nice way to handle the fallback if we pass TOML data to to the MenuItem class, and we don't get a nice separation of concerns. And I think it would already become more complex than what it already is.

marcoesters · 2026-05-15T20:01:28Z

+    """Get recorded shortcut paths for a source from menuinst.toml."""
+    data = read_menuinst_toml(prefix)
+    shortcuts = data.get("shortcuts", [])
+    return [s["path"] for s in shortcuts if s.get("source") == source and "path" in s]


Suggested change

return [s["path"] for s in shortcuts if s.get("source") == source and "path" in s]

return [Path(s["path"]) for s in shortcuts if s.get("source") == source and "path" in s]

Since the delete_paths function returns list[Path], we might as well convert to Path early.

This also requires us to update delete_paths() which expects list[str], I'm fine with changing both. Let me know what you think.

Yes, changing both is good. I think we should use Path objects as much as possible.

See 1b871e4

marcoesters · 2026-05-15T20:04:13Z


    # Record shortcuts to menuinst.toml
+    # Only record MenuItem paths, not Menu paths (.directory files, etc.)
+    # The Menu's remove() method handles its own cleanup logic


Menu can be just as affected by drifts as MenuItem. I think Menu should be included in the record. That would favor a solution where a menus and menu items are built from the TOML file.

The function Menu.remove() has conditional logic on all platforms, as an example:

Linux checks if shortcuts remain,

Windows checks if folder is empty,

macOS is a no-op.
Recording Menu paths in the TOML file wouldn't capture this, which is why the state of the file system must be checked at removal time.

Correct. Imagine you have a path drift where the computed value does not correspond to the original value anymore. On Windows, e.g., you'd remove the Start Menu entries, but you'd leave the folder behind since Menu still relies on the computed value.

Hmm. I think for this PR the core issue is resolved, and I do agree that orphaned menu folders can still remain. I'm concerned this will also increase the complexity and scope of this PR but let me know what you think and I can look into it.

My concern is that if we decide on this solution here, but then find out that it doesn't work for Menu, that we will have to refactor in the end. Let me think about this some more.

marcoesters · 2026-05-15T20:20:09Z

+
+    def test_remove_uses_recorded_paths(self, tmp_path, monkeypatch):
+        """Test that remove() deletes files at recorded TOML paths, not computed paths."""
+        monkeypatch.delenv("MENUINST_DISTRIBUTION_NAME", raising=False)


Since this placeholder is not used in the metadata of this test, you don't need to delete that variable.

Fixed 1b871e4

marcoesters · 2026-05-15T20:22:47Z

+        )
+
+        # Verifying no exception is raised
+        remove(str(json_file), target_prefix=str(tmp_path), base_prefix=str(tmp_path))


There should be an assert statement here that checks whether the file really was removed.

The test currently only verifies that remove() doesn't crash when there are no TOML entries, it doesn't actually test file removal because install() was never called (no shortcut exists to remove). In order to add what you proposed I need update the test to call install() first, let me know if you still think the assert is necessary.

I'm drawing a blank of where that exception would be raised at the moment, can you job my memory?

There is no explicit exception, but the idea was to test that remove "works" in the sense that no unexpected error is raised.

I think an actual remove operation would be a more convincing test.

lrandersson added 6 commits May 14, 2026 17:12

add get_recorded_paths() to retrieve TOML shortcut paths

e6a5d38

Add delete_paths() to api.py

0320ec0

Refactor remove functions

9d29114

Update api.remove() to use TOML paths

228250d

Remove TODO and pre-commit fix

138585c

fix bug

2cb3753

lrandersson marked this pull request as ready for review May 15, 2026 13:33

temporarily enable test for fork

e685b6a

lrandersson self-assigned this May 15, 2026

Improve tests

2368861

lrandersson mentioned this pull request May 15, 2026

Add support for menuinst.toml conda/menuinst#477

Open

3 tasks

marcoesters reviewed May 15, 2026

View reviewed changes

lrandersson requested a review from marcoesters May 15, 2026 18:48

marcoesters reviewed May 15, 2026

View reviewed changes

Update test, change return type

1b871e4

	return [s["path"] for s in shortcuts if s.get("source") == source and "path" in s]
	return [Path(s["path"]) for s in shortcuts if s.get("source") == source and "path" in s]

Conversation

lrandersson commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

api.py

Platform files (win.py, osx.py, linux.py)

Removal Flow

Checklist - did you ...

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcoesters commented May 15, 2026

Uh oh!

lrandersson commented May 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lrandersson commented May 14, 2026 •

edited

Loading

`api.py`

Platform files (`win.py`, `osx.py`, `linux.py`)