Port All Apps plugin again #409

stefansjfw · 2025-02-06T10:05:23Z

MISSING:

Support arguments passing
~~Live app detection (newly installed apps, updates, etc..)~~
~~Localization~~
~~Cleanup~~

Argument passing requires conversion to DynamicListPage and is low priority. Let's merge this first and I'll work on this as a follow up

Closes #76

stefansjfw · 2025-02-12T10:53:01Z

@zadjii-msft FYI Ready for review

zadjii-msft

I'm inclined to just sign off on this since it's 99% just porting remaining code that I had commented out or ignored back in June, but I want to give it a test run before we merge it in

that being said - this looks awesome and I'm excited to see everything hopefully finally have the right subtitles again

zadjii-msft · 2025-02-12T12:19:29Z

src/modules/cmdpal/Exts/Microsoft.CmdPal.Ext.Apps/AllAppsPage.cs

-
-                    // ExePath = app.FullPath,
+                    IsPackaged = true,
+                    Commands = app.Commands,


📝: UWPs just use app.Commands, Win32's need app.GetCommands()?

I swear this was aligned! At some point at least! :) GetCommands() introduced in UWP apps as well.

zadjii-msft · 2025-02-12T12:22:11Z

src/modules/cmdpal/Exts/Microsoft.CmdPal.Ext.Apps/AppListItem.cs

        };

-        if (string.IsNullOrEmpty(app.UserModelId))
+        if (!_app.IsPackaged)


nit: we can collapse these two branches down since they're the same now

hah. of course :)

src/modules/cmdpal/Exts/Microsoft.CmdPal.Ext.Apps/AppListItem.cs

zadjii-msft · 2025-02-12T12:26:20Z

src/modules/cmdpal/exts/Microsoft.CmdPal.Ext.Apps/Commands/OpenInConsoleCommand.cs

+    public OpenInConsoleCommand(string target)
+    {
+        Name = Resources.open_path_in_console;
+        Icon = new IconInfo("\ue838");


nit: we can probably save some memory by having a single static instance of this IconInfo for all OpenInConsoleCommand's, but who knows if that's significant

zadjii-msft · 2025-02-12T12:28:51Z

src/modules/cmdpal/exts/Microsoft.CmdPal.Ext.Apps/Commands/RunAsUserCommand.cs

+
+    public RunAsUserCommand(string target, string parentDir)
+    {
+        Name = "Run as different user";


does this one need to get localized too?

ooh, nice catch. there are few more. fixed

zadjii-msft · 2025-02-12T12:29:55Z

src/modules/cmdpal/exts/Microsoft.CmdPal.Ext.Apps/Commands/RunAsUserCommand.cs

+
+namespace Microsoft.CmdPal.Ext.Apps.Commands;
+
+// NOTE this is pretty close to what we'd put in the SDK


nit: this comment can probably go away now - I think it's been copy/pasted a bunch of times since the original hackathon for this project 10 months ago

zadjii-msft · 2025-02-12T12:39:08Z

src/modules/cmdpal/exts/Microsoft.CmdPal.Ext.Apps/Utils/ShellCommand.cs

+        OtherUser,
+    }
+
+    public static ProcessStartInfo GetProcessStartInfo(string target, string parentDir, string programArguments, RunAsType runAs = RunAsType.None)


ha you know, I think I already pilfered a bunch of this into ShellHelpers in the toolkit. I should file a future todo to just de-dupe all the calls that do this everywhere

stefansjfw · 2025-02-12T13:50:07Z

fixed all. But do not merge yet! I see some issues with icons for Packaged apps.. working on it

stefansjfw · 2025-02-12T14:05:10Z

All good. Fixed packaged apps icon load. Give it a test end let me know how it goes :)

lauren-ciha · 2025-02-12T17:50:07Z

I forget if it was mentioned already, but there's a misalignment on the app subtitles. For example,

opens Google Chrome (as expected), but has a Visual Studio subtitle.

zadjii-msft · 2025-02-12T19:03:01Z

I forget if it was mentioned already, but there's a misalignment on the app subtitles.

uhg then that's definitely the same list virtualization bug we were seeing with the icons. At least that wasn't unique to the hack job for apps I had before, but yikes we need to fix this.

oh my god wait I think I have an idea. I have a theory - it might be that we're never PropertyChanged'ing the items when we fetch their properties - it was something I noticed in #388. I've got a branch, gimme a bit to go build this with that branch merged

zadjii-msft · 2025-02-12T19:19:18Z

Okay no, it wasn't that.

I think there might be two problems, and I'm not sure they're separate yet.

Problem the first: The icon loading is definitely fucky. I think this probably comes down to the async loading for random access stream icons. I'm betting that the async-ness of loading them, mixed with the list virtualization, causes the icons to get loaded weird. Just now, I had the event viewer icon show up on the "web search" command on the main page, when I started typing "chro".
Problem the second: the subtitles still not lining up with the app. This I don't know what's up yet.

zadjii-msft · 2025-02-12T19:46:57Z

Commenting out the stream icons from the apps at least doesn't leave everything exploded. It leaves almost everything without an icon, but I'm guessing that this is the cause of the wrong icons.
That being said, commenting out the icons doesn't fix the broken subtitles. That's still messed up. It also seems... stable. Like, if I go to tall apps, filter for like, "firefox", see that it's got the wrong subtitle, then go try to search for firefox on the root view, it'll have the same wrong subtitle. Problem is that it's not stable across reboots, so you can't just always set a breakpoint on like, "Title==Firefox" and see what's up -sometimes firefox will still be right. This is tricky

zadjii-msft · 2025-02-12T19:56:57Z

oh it's so obvious (in retrospect)

ShellLinkHelper is stored as a static singleton. It's used to retrieve the target path for a Lnk, then get the description out of it. Problem is, we enumerate the apps in parallel. So one app might RetreieveTargetPath, then before we get the description out of it - the next app comes along and stomps it

zadjii-msft · 2025-02-12T20:09:07Z

okay affb26f fixes the subtitles I think

zadjii-msft · 2025-02-12T20:21:54Z

and 51b162a fixes the icon loading.

Thanks for your patience ☺️

Feel free to just merge those two in here.

The only other thing that I'd want to double check is that it feels like the main page no longer reports the initial app load as part of its IsLoading. In the past it would have the loading bar spin while it was doing both the extension and the app load in the background, but now I think doesnt. Need to double check that (probably tomorrow morning)

lauren-ciha · 2025-02-12T21:50:10Z

When I tried dev/migrie/f/apps-and-propchanged, the icons and subtitles worked as expected.

I think I figured out why Access was opening when I clicked on the All Apps command in the main list page. I had turned double clicking off and accidentally clicked "All apps" twice. The first click opened All Apps and the second click opened Access before the All Apps page loaded. This bug only happens occasionally. Most times, a double click just opened the All Apps page.

It took a few tries (the app opens at the 35 second mark). The top left icon also had some interesting glitching as I was clicking back and forth:

stefansjfw · 2025-02-13T08:41:00Z

Feel free to just merge those two in here.

Just did

zadjii-msft · 2025-02-13T17:30:29Z

Mmk so I pushed a commit just now that should have the main page also show the loading state of the apps again. Definitely takes a bit longer to settle now, but that's mostly because there are so many apps we're enumerating now.

We can work on porting that settings file to a JsonSettingsManager like we have for the other extensions later. This is close enough 👍

Port All Apps plugin again

f724334

stefansjfw marked this pull request as draft February 6, 2025 10:16

stefansjfw added 6 commits February 6, 2025 21:23

Live app reload - install/uninstall/change

cc64229

Merge remote-tracking branch 'origin/main' into stefan/report_apps

73dd91d

Localization

7f10542

Use AppCache as a single source of truth

0717384

minor cleanup

e5f44e5

Fix icon fetch

e6b53f7

stefansjfw marked this pull request as ready for review February 12, 2025 10:52

stefansjfw changed the title ~~[WIP] Port All Apps plugin again~~ Port All Apps plugin again Feb 12, 2025

zadjii-msft reviewed Feb 12, 2025

View reviewed changes

stefansjfw added 2 commits February 12, 2025 14:50

Address PR comments

0b676d5

Fix packaged app icon load

2dec4bd

Improve load time

28940f7

zadjii added 2 commits February 13, 2025 09:38

Fix the async icon loading

adb4055

This fixes the subtitles

ac60bf2

Make sure the main page still shows loading when apps are loading

1ae3be4

zadjii-msft approved these changes Feb 13, 2025

View reviewed changes

zadjii-msft merged commit 38a7f18 into main Feb 13, 2025
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port All Apps plugin again #409

Port All Apps plugin again #409

stefansjfw commented Feb 6, 2025 •

edited by zadjii-msft

Loading

stefansjfw commented Feb 12, 2025

zadjii-msft left a comment

zadjii-msft Feb 12, 2025

stefansjfw Feb 12, 2025

zadjii-msft Feb 12, 2025

stefansjfw Feb 12, 2025

zadjii-msft Feb 12, 2025

stefansjfw Feb 12, 2025

zadjii-msft Feb 12, 2025

stefansjfw Feb 12, 2025

zadjii-msft Feb 12, 2025

zadjii-msft Feb 12, 2025

stefansjfw commented Feb 12, 2025

stefansjfw commented Feb 12, 2025

lauren-ciha commented Feb 12, 2025

zadjii-msft commented Feb 12, 2025

zadjii-msft commented Feb 12, 2025

zadjii-msft commented Feb 12, 2025

zadjii-msft commented Feb 12, 2025

zadjii-msft commented Feb 12, 2025 •

edited

Loading

zadjii-msft commented Feb 12, 2025 •

edited

Loading

lauren-ciha commented Feb 12, 2025 •

edited

Loading

stefansjfw commented Feb 13, 2025

zadjii-msft commented Feb 13, 2025


		namespace Microsoft.CmdPal.Ext.Apps.Commands;

		// NOTE this is pretty close to what we'd put in the SDK

Port All Apps plugin again #409

Port All Apps plugin again #409

Conversation

stefansjfw commented Feb 6, 2025 • edited by zadjii-msft Loading

stefansjfw commented Feb 12, 2025

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefansjfw commented Feb 12, 2025

stefansjfw commented Feb 12, 2025

lauren-ciha commented Feb 12, 2025

zadjii-msft commented Feb 12, 2025

zadjii-msft commented Feb 12, 2025

zadjii-msft commented Feb 12, 2025

zadjii-msft commented Feb 12, 2025

zadjii-msft commented Feb 12, 2025 • edited Loading

zadjii-msft commented Feb 12, 2025 • edited Loading

lauren-ciha commented Feb 12, 2025 • edited Loading

stefansjfw commented Feb 13, 2025

zadjii-msft commented Feb 13, 2025

stefansjfw commented Feb 6, 2025 •

edited by zadjii-msft

Loading

zadjii-msft commented Feb 12, 2025 •

edited

Loading

zadjii-msft commented Feb 12, 2025 •

edited

Loading

lauren-ciha commented Feb 12, 2025 •

edited

Loading