Hook up the buttons, and othe UX nits #229

zadjii-msft · 2024-12-14T16:07:50Z

targets #224

A big collection of UX nits.

The command buttons at the bottom work now.
The context menu works with enter now
ctrl+enter will do the secondary command now
ctrl+k will open the context menu now
Lists will do a better job of starting with a selected item, so that you can immediately start arrowing through the list
Lists will also do a better job of keeping at least one item selected as the list changes.
I manually dispatch a CurrentPageViewModel.Filter = FilterBox.Text when the filterbox backspaces to a single character. IIRC there's a bug in the debouncer upstream, but I had no patience for that
I reworded some things and changed some icons.
- "Execute shell commands" felt so "I am a 1337 haxor", when "sir, this is a run dialog"
- "Switch between open windows" is tooooooooo lllllllllllooooooooooooonnnnnnngggggggggg

…ists

But enter on the context menu weirdly doesn't work. Somewhere the Command on the context item is set to null. Not sure why

…mic-lists

(cherry picked from commit a0fa016)

…mic-lists

…ts-pr

* Handle command results * move the cursor to the end of the search when going back * clear the search on going home * show apps on the main page * fix HeroImages for details (cherry picked from commit 87f513a)

(cherry picked from commit f324e60)

michael-hawker

Lots to process here, think I got a handle on some of it to comment on and try out. Will need to go through the PR intro notes again.

Think this is a good follow-up from my comment on the other PR, now that the Command Results can be processed, should we have the default app running result be dismiss over GoHome?

src/modules/cmdpal/Exts/Microsoft.CmdPal.Ext.Shell/Properties/Resources.resx

michael-hawker · 2024-12-16T20:13:38Z

src/modules/cmdpal/Microsoft.CmdPal.UI/Controls/ActionBar.xaml.cs

+
+    [System.Diagnostics.CodeAnalysis.SuppressMessage("CodeQuality", "IDE0051:Remove unused private members", Justification = "VS has a tendency to delete XAML bound methods over-agressively")]
+    private void PrimaryButton_Tapped(object sender, TappedRoutedEventArgs e) =>
+        WeakReferenceMessenger.Default.Send<ActivateSelectedListItemMessage>();


It was suggested that the WCT could have Behavior helpers for this for simple messages like this. Though not sure if records can be constructed in XAML either (would be nice). Ties into improvements Behaviors need anyway for directly supporting specific events like Tapped for AOT support anyway, but anyway, just wanted to leave this here for posterity of we could clean these up in the future to make it easier.

michael-hawker · 2024-12-16T20:14:11Z

src/modules/cmdpal/Microsoft.CmdPal.UI/Controls/ActionBar.xaml.cs

        }
    }
+
+    [System.Diagnostics.CodeAnalysis.SuppressMessage("CodeQuality", "IDE0051:Remove unused private members", Justification = "VS has a tendency to delete XAML bound methods over-agressively")]


I haven't seen this elsewhere, so I'm not sure what's going on with the PT config that's causing this to be flagged. We should investigate later and follow-up as it's weird we need these.

Maybe it's just me configuring the editor code cleanup rules to aggressively? But it definitely constantly deletes any of the XAML bound methods on save. Only way to stop it is like this, or marking the methods as internal (which is not technically correct)

It's really just an annoyance, but I agree we should get to the bottom of it

michael-hawker · 2024-12-16T20:15:26Z

src/modules/cmdpal/Microsoft.CmdPal.UI/Controls/IconBox.cs

-                        {
-                            @this.Source = eventArgs.Value;
-                        }
+                        @this.Source = eventArgs.Value;


Ah, thanks, I know we discussed this last week, and I forgot to go back and fix it. 😅

Future note for if we pull this in Toolkit later, is we could have a FallbackSource property to display when the key lookup returns null here. I don't think we have a scenario for this currently, but just a thought.

src/modules/cmdpal/Microsoft.CmdPal.UI/ExtViews/IconCacheService.cs

michael-hawker · 2024-12-16T20:20:05Z

src/modules/cmdpal/Microsoft.CmdPal.UI/Controls/SearchBar.xaml.cs

+            {
+                CurrentPageViewModel.Filter = FilterBox.Text;
+            }
+        }
    }

    private void FilterBox_TextChanged(object sender, TextChangedEventArgs e)


I thought the PR comment mentioned shortcutting the debouncer somewhere, where am I missing seeing that in the changes?

The debouncer issue you saw is mostly likely the bug that I detected and fixed as part of the improvements I made (including tests) there: CommunityToolkit/Windows#569 - we've just been waiting on some other stuff to push a new update to NuGet out (hopefully in a day or two). Otherwise, you could pull from the MainLatest feed like we're doing for Labs.

I just saw the comment in the PR and see that I missed it literally right above here in the Backspace key handle. Yeah, I'm 99% sure this is the issue with the debouncer I fixed. If you hit Escape vs. backspace it should be fine as it's a different codepath.

src/modules/cmdpal/Microsoft.CmdPal.UI/ShellPage.xaml.cs

michael-hawker · 2024-12-16T20:25:17Z

src/modules/cmdpal/Microsoft.CmdPal.UI/Controls/ActionBar.xaml

Somewhere in here seeing the shortcut getting trimmed:

woah what

@niels9001 any ideas?

Yeah, we might have a few hardcoded heights. Will tackle that in a separate PR once this gets in.

This was originally based off `dev/migrie/f/ux-nits`, which is #229 * Adds support for our `win+ctrl+.` global hotkey again. * Makes our window a transient toolwindow. It doesn't show up in alt+tab. It can only be summoned with the hotkey * Unless you're debugging, because that's an absolute chore * Manually makes Quit visible as a command. Quit is usually a fallback command, but I implemented that _right after this commit_. However, I _believe_ the fallback commands need #224 to merge, whereas this subset of deltas didn't exactly Closes #136

zadjii-msft · 2024-12-16T21:57:31Z

should we have the default app running result be dismiss over GoHome?

oh my god that's why it acts like that! It definitely should be dismiss 🤦

done in 0b00cdc

…Resources.resx Co-authored-by: Michael Hawker MSFT (XAML Llama) <[email protected]>

…sft/powertoys into dev/migrie/f/ux-nits-pr

michael-hawker

Small final clean-up comment

michael-hawker · 2024-12-17T19:47:44Z

src/modules/cmdpal/Microsoft.CmdPal.UI/Controls/SearchBar.xaml.cs

    }

+    public void ClearSearch() => this.FilterBox.Text = string.Empty;


This doesn't have to be public anymore, eh? Should we just move the line into the Receive body for GoHome?

joadoumie · 2024-12-17T19:22:08Z

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

@@ -62,6 +62,6 @@ internal async Task Launch()
    public override CommandResult Invoke()
    {
        _ = Launch();
-        return CommandResult.GoHome();
+        return CommandResult.Dismiss();


nit: should we clean up the file name -- 'AppAction' made sense in Wox land as Wox has actions, but since we are aiming for command palette vibes, probably should be AppCommand.cs

We need to make a pass over a lot more than just this file - definitely better to do in a separate PR

joadoumie · 2024-12-17T19:22:59Z

src/modules/cmdpal/Exts/Microsoft.CmdPal.Ext.WindowsServices/Pages/ServicesListPage.cs


 namespace Microsoft.CmdPal.Ext.WindowsServices;

 internal sealed partial class ServicesListPage : DynamicListPage
 {
    public ServicesListPage()
    {
-        Icon = new(string.Empty);
+        Icon = new("%windir%\\system32\\filemgmt.dll");


I don't know why this line gives me a lot of joy.

joadoumie · 2024-12-17T19:44:47Z

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Messages/ActivateSecondaryCommandMessage.cs

+/// <summary>
+/// Used to perform a list item's secondary command when the user presses ctrl+enter in the search box
+/// </summary>
+public record ActivateSecondaryCommandMessage


Is this just a to do?

joadoumie · 2024-12-17T19:45:32Z

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Messages/OpenContextMenuMessage.cs

+/// Used to perform a list item's secondary command when the user presses ctrl+enter in the search box
+/// </summary>
+public record OpenContextMenuMessage
+{


What are the purposes of all the messages? Looks like all the bodies are empty -- what do we use these for?

oh I guess this is referred to elsewhere -- not sure exactly what for

@joadoumie these represent the objects of the messages themselves that get passed to the MVVM Toolkit Messenger we use to do communication across components/layers of the application. Many of them don't have extra parameters, like this one, and are just a signal that some event has happened. In this case, it's a request to open the context menu. It's coming from the keyboard handlers in the searchbox (iirc) in order to trigger opening the menu in the ActionBar as they're two independent UI controls now.

joadoumie · 2024-12-17T19:49:22Z

src/modules/cmdpal/Microsoft.CmdPal.UI/Controls/SearchBar.xaml.cs

            @this.FilterBox.Text = page.Filter;
+            @this.FilterBox.Select(@this.FilterBox.Text.Length, 0);


what is the @ decorator for?

this is a keyword in C# normally, but this is a static method as it's the callback from the dependency property where we don't have the specific instance of our class, that's passed in as the first parameter, so we do a cast on line 46 and make our own reference to "this". But since it's a keyword, we use the @ declarator to tell C# that we really want to use this as our variable name, basically it's a C# syntax feature to let you use keywords like this, class, etc... as variable names.

It's not that common, and probably not the best to use in many cases, but here is kind of a special case as we really are trying to refer to our own class here, we're just in a static method.

However, it's really just temporary, as when the DependencyProperty source generators come out of Toolkit Labs, then the DP declaration here can all be replaced with a nice source generator which will call a method for us on our class instance, so we'll already have our regular context and won't need this at all. 😊

github-actions · 2024-12-18T12:23:01Z

@check-spelling-bot Report

🔴 Please review

See the 📂 files view, the 📜action log, or 📝 job summary for details.

Unrecognized words (169)

ACVS
adaptivecards
affordances
agressive
agressively
Aliias
Allman
apicontract
APPICONREFERENCE
APPMODEL
APPPUBLISHER
asynchonously
AVery
BNDBk
bodgy
builttoroam
BYEXENAME
Cantarell
cdn
channelurl
charater
CLSIDs
cmdpal
commandpalette
contractversion
cpcontrols
CREATETHREAD
CREATEWINDOW
cswin
Ctl
CVS
DDEAPPLICATION
DDECOMMAND
DDEIFEXEC
DDETOPIC
defaultlib
DEFAULTTOFOLDER
DEFAULTTOSTAR
DELEGATEEXECUTE
devpal
dic
dragable
Droid
DROPTARGET
Errrors
ESPN
extensionsdk
favourites
filemgmt
Fira
flaticon
footgunning
fpvm
Fqc
FRIENDLYAPPNAME
FRIENDLYDOCNAME
gifv
Griese
gsl
Hande
harcoded
HKC
homies
HSSH
icf
idk
iextn
IGNOREBASECLASS
IGo
imgflip
imgur
implementaions
implmenting
inbox
infotip
INLINEPREFIX
intederminate
INVALIDCALL
INVALIDINDEX
keyboardtester
kinda
libraryincludes
localappdata
LTk
LUQ
lvm
MBM
mdpvm
meme
memicmp
Microwaved
minwindef
mobileredirect
mtu
murmurhash
MVVMTK
NGQt
nicksnettravels
NOFIXUPS
nonspace
NOOPEN
NOREMAPCLSID
NOTRUNCATE
NOUSERSETTINGS
NTZm
nullability
nullness
OWMt
OWRj
paintin
Playbadge
Poc
Podcasts
Pokedex
Puser
QUICKTIP
QXZ
raisee
Rasterization
realease
reblogs
refelct
refering
REGISTERCLASSEX
REMAPRUNDLL
renamve
rsdk
searchtext
seearch
seperators
SFBS
sharepoint
SHELLEXTENSION
SHELLNEWVALUE
shich
shurg
somee
sooon
sourced
sourcedoc
Spongebob
spongebot
stdcpp
throughs
TIcon
TILEINFO
tlc
tthe
ucrt
ucrtd
unistalls
videourl
vkey
VMs
vulnerbilities
wasdk
WCT
Wholegrain
wic
WUX
xclip
xdoc
XJSNh
ycombinator
youtube
YTM
zadjii
ZXk
ZXNs

These words are not needed and should be removed

accctrl aclapi appdata Appium appmodel atlbase atlcom atlfile atlstr BODGY bootstrapper caniuse ceq cguid Cmds cne codicon comdef commandline commctrl commdlg comutil consts CRSEL crx dcommon dcomp DCs desktopwindowxamlsource devpkey dxgidebug dxgiformat emmintrin Emoji endpointvolume evntrace exdisp Functiondiscoverykeys guiddef hinstance hstring Infotip Intelli junja Knownfolders lmcons LONGLONG lpt LTRB mfapi mfidl mfobjects mftransform Minimatch mmdeviceapi mmsystem msedge msiquery newdev nodoc notlike Objbase objidl pathcch Pnp Preinstalled processthreadsapi propkey propvarutil redistributable Renamer reparse restrictederrorinfo roadmap ruleset runtimes shellapi shellscalingapi shldisp shlobj stl strsafe strutil subquery SWC tailwindcss tapp thumbcache tlhelp Toolset touchpad Tsd uninstantiated uniquifier Unknwn unregistering urlmon USERDATA Uxtheme verrsrc VKey wcautil WIC wincodec Wincodecsdk windef windowsapp windowsx winerror winevt winsdkver winternl wsl wtsapi

Some files were automatically ignored 🙈

These sample patterns would exclude them:

^\Qsrc/modules/cmdpal/doc/initial-sdk-spec/list-elements-mock-002.pdn\E$
^\Qsrc/modules/cmdpal/Exts/PokedexExtension/Pages/PokedexExtensionPage.cs\E$

You should consider adding them to:

.github/actions/spell-check/excludes.txt

File matching is via Perl regular expressions.

To check these files, more of their words need to be in the dictionary than not. You can use patterns.txt to exclude portions, add items to the dictionary (e.g. by adding them to allow.txt), or fix typos.

To accept these unrecognized words as correct, update file exclusions, and remove the previously acknowledged and now absent words, you could run the following commands

... in a clone of the [email protected]:zadjii-msft/PowerToys.git repository
on the dev/migrie/f/ux-nits-pr branch (ℹ️ how do I use this?):

curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/v0.0.24/apply.pl' |
perl - 'https://github.com/zadjii-msft/PowerToys/actions/runs/12392808294/attempts/1'

OR

To have the bot accept them for you, comment in the PR quoting the following line:
@check-spelling-bot apply updates.

Forbidden patterns 🙅 (7)

In order to address this, you could change the content to not match the forbidden patterns (comments before forbidden patterns may help explain why they're forbidden), add patterns for acceptable instances, or adjust the forbidden patterns themselves.

These forbidden patterns matched content:

Should be `cannot` (or `can't`)

See https://www.grammarly.com/blog/cannot-or-can-not/

Don't use can not when you mean cannot. The only time you're likely to see can not written as separate words is when the word can happens to precede some other phrase that happens to start with not.
Can't is a contraction of cannot, and it's best suited for informal writing.
In formal writing and where contractions are frowned upon, use cannot.
It is possible to write can not, but you generally find it only as part of some other construction, such as not only . . . but also.

if you encounter such a case, add a pattern for that case to patterns.txt.

\b[Cc]an not\b

In English, duplicated words are generally mistakes

There are a few exceptions (e.g. "that that").
If the highlighted doubled word pair is in:

code, write a pattern to mask it.
prose, have someone read the English before you dismiss this error.

\s([A-Z]{3,}|[A-Z][a-z]{2,}|[a-z]{3,})\s\g{-1}\s

Do not use `(click) here` links

For more information, see:

(?i)(?:>|\[)(?:(?:click |)here|this(?=\]\([^\)]+:/)|link|(?:read |)more(?!</value))(?:</|\]\()

Should only be one of `a`, `an`, or `the`

\b(?:(?:an?|the)\s+){2,}\b

Should be `into`

when not phrasal and when in order to would be wrong:
https://thewritepractice.com/into-vs-in-to/

\sin to\s(?!if\b)

Should be `its`

\bit's(?= own\b)

Should be `prepopulate`

[Pp]re[- ]populate

Pattern suggestions ✂️ (2)

You could add these patterns to .github/actions/spell-check/patterns.txt:

# Automatically suggested patterns

# hit-count: 1 file-count: 1
# Amazon
\bamazon\.com/[-\w]+/(?:dp/[0-9A-Z]+|)

# hit-count: 1 file-count: 1
# imgur
\bimgur\.com/[^.]+

Errors (5)

See the 📂 files view, the 📜action log, or 📝 job summary for details.

❌ Errors	Count
⚠️ binary-file	1
ℹ️ candidate-pattern	2
❌ check-file-path	669
❌ forbidden-pattern	14
⚠️ noisy-file	1

See ❌ Event descriptions for more information.

If the flagged items are 🤯 false positives

If items relate to a ...

binary file (or some other file you wouldn't want to check at all).

Please add a file path to the excludes.txt file matching the containing file.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).
well-formed pattern.

If you can write a pattern that would match it,
try adding it to the patterns.txt file.

Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

Note that patterns can't match multiline strings.

zadjii added 18 commits December 13, 2024 11:48

rudimentary support for dynamic lists

5de29f9

that was important

0012c16

damn it's done? Just like that?

642d98d

actually just renaming and moving files around

09b63d6

The dumbest possible 'but it works'

275b0af

show a loading bar while we're loading up

9bf4d2e

progressively filter the results we already had

ad9635c

add a couple commands that missed TRA

2016bcb

Merge branch 'dev/migrie/f/main-apps' into dev/migrie/f/TRA-dynamic-l…

113a6c6

…ists

Wire up the buttons, and ctrl+k

2cc5399

But enter on the context menu weirdly doesn't work. Somewhere the Command on the context item is set to null. Not sure why

Merge remote-tracking branch 'origin/main' into dev/migrie/f/TRA-dyna…

2d032b1

…mic-lists

Merge branch 'dev/migrie/f/TRA-dynamic-lists' into dev/migrie/f/ux-nits

038ac0a

Yea that was a bit of a footgun huh

bd6140d

Add window icons to WW. Fix an IconBox bug.

c96c93b

marginally better at selecting the first element

a3f4d55

mostly notes

1a3c740

iconography and string touch-ups

3f9c9b8

yea missed that that's obvious

cd9dbc5

zadjii-msft mentioned this pull request Dec 14, 2024

Migrate UI to Proper MVVM Framework #73

Closed

stash: couple stability improvements

a407eb5

zadjii mentioned this pull request Dec 15, 2024

Add global hotkey again #232

Merged

zadjii added 4 commits December 15, 2024 12:00

Oh IEnumerable is so much better

c91adc2

ANCESTOR: ItemsChanged might come in on a BG thread, watch out

956c798

(cherry picked from commit a0fa016)

Merge branch 'main' into dev/migrie/f/TRA-dynamic-lists

abbaefb

Stop the .Count()

34282b3

Base automatically changed from dev/migrie/f/TRA-dynamic-lists to main December 16, 2024 17:38

zadjii added 4 commits December 16, 2024 11:40

Merge remote-tracking branch 'origin/main' into dev/migrie/f/TRA-dyna…

b217728

…mic-lists

Merge branch 'dev/migrie/f/TRA-dynamic-lists' into dev/migrie/f/ux-ni…

b9d8b7e

…ts-pr

Another collection of nits:

70773b0

* Handle command results * move the cursor to the end of the search when going back * clear the search on going home * show apps on the main page * fix HeroImages for details (cherry picked from commit 87f513a)

"Cherry-pick" from 0b789ad, dev/migrie/f/stash-forms-should-submit

cd57e30

(cherry picked from commit f324e60)

michael-hawker reviewed Dec 16, 2024

View reviewed changes

zadjii-msft mentioned this pull request Dec 16, 2024

If an extension crashes while you're on an L2 page, then navigate back, we crash (READ: make an IconViewModel) #235

Closed

zadjii and others added 5 commits December 16, 2024 15:58

Merge remote-tracking branch 'origin/main' into dev/migrie/f/ux-nits-pr

a8bbc70

Update src/modules/cmdpal/Exts/Microsoft.CmdPal.Ext.Shell/Properties/…

8ff54d4

…Resources.resx Co-authored-by: Michael Hawker MSFT (XAML Llama) <[email protected]>

Dismiss after launching an app

0b00cdc

Merge branch 'dev/migrie/f/ux-nits-pr' of https://github.com/zadjii-m…

7d98ac7

…sft/powertoys into dev/migrie/f/ux-nits-pr

messages

8f21516

zadjii-msft mentioned this pull request Dec 17, 2024

Add support for aliases #236

Merged

michael-hawker approved these changes Dec 17, 2024

View reviewed changes

joadoumie approved these changes Dec 17, 2024

View reviewed changes

zadjii added 2 commits December 18, 2024 06:13

make private

08f7977

Merge remote-tracking branch 'origin/main' into dev/migrie/f/ux-nits-pr

f56693c

zadjii merged commit 9cd5621 into main Dec 18, 2024
3 of 4 checks passed

zadjii deleted the dev/migrie/f/ux-nits-pr branch December 18, 2024 12:20

		}

		public void ClearSearch() => this.FilterBox.Text = string.Empty;

		@this.FilterBox.Text = page.Filter;
		@this.FilterBox.Select(@this.FilterBox.Text.Length, 0);

Hook up the buttons, and othe UX nits #229

Hook up the buttons, and othe UX nits #229

Uh oh!

Conversation

zadjii-msft commented Dec 14, 2024

Uh oh!

michael-hawker left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

zadjii-msft commented Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michael-hawker left a comment

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!

Uh oh!

github-actions bot commented Dec 18, 2024

@check-spelling-bot Report

🔴 Please review

See the 📂 files view, the 📜action log, or 📝 job summary for details.

Should be cannot (or can't)

In English, duplicated words are generally mistakes

Do not use (click) here links

Should only be one of a, an, or the

Should be into

Should be its

Should be prepopulate

See the 📂 files view, the 📜action log, or 📝 job summary for details.

Uh oh!

Uh oh!

zadjii-msft commented Dec 16, 2024 •

edited

Loading

Should be `cannot` (or `can't`)

Do not use `(click) here` links

Should only be one of `a`, `an`, or `the`

Should be `into`

Should be `its`

Should be `prepopulate`