Add App Filtering Page and Session Validation (ref #633)#652
Add App Filtering Page and Session Validation (ref #633)#652galagyy wants to merge 24 commits intounchihugo:masterfrom
Conversation
- Add dedicated "App Filter" page - Add "App Filtering" menu card and navigation item - Add app filtering properties to settings storage - Add app filtering keys to en-US dictionary - Update main menu to use `GetActiveMediaSession()` over `GetFocusedSession()` - Update control click handlers to use `GetActiveMediaSession()`
- Update formatting from K&R to BSD (C# standard)
- Add taskbar & flyout refresh once a filter has been updated
github-actions
Bot
added
MainWindow / Media Flyout
|
A quick comment about the formatting, I come from a Java/C++ background so I had originally formatted it with braces on the same line. I changed it in one of the commits, but if there is still an issue with it let me know and I will try my best to fix it. Thanks! |
|
Found a small bug regarding pause/play, I will try fixing it today. |
- Update `PlayPause_Click()` in both `MainWindow.xaml.cs` and `TaskbarWidgetControl.xaml.cs` to use `TryTogglePlayPauseAsync()` over the previous keyboard approach to respect app filtering - Update UI logic to check `PlaybackStatus` over `Controls.IsPauseEnabled` to prevent desync from filters NOTE: ControlPlayPause opacity has been moved outside of the conditional due to making more sense there.
|
Fixed the bug above, I changed the old implementation that relied on manually sending a keyboard input. |
unchihugo
left a comment
unchihugo
left a comment
There was a problem hiding this comment.
Hi @galagyy, solid implementation so far! Just a few things I've noticed throughout that we should adjust before merging. I've noted it in the comments.
I'd like to discuss the implementation of the filter lists:
Currently, names in the filter lists aren't sanitized - we should have this feature utilize our MediaPlayerData class. Methods in here can find the sanitized title + icon for apps/media players, especially with the changes I've added myself in the Volume Flyout branch here.
Could you adapt the current code to use the GetMediaPlayerData() method in MediaPlayerData instead of ExtractAppName()? That would be great!
Once the current issues/questions are addressed, we can continue with the merge :)
| if (taskbarSession == null) | ||
| { | ||
| taskbarWindow?.UpdateUi("-", "-", null, GlobalSystemMediaTransportControlsSessionPlaybackStatus.Closed); | ||
| return; |
There was a problem hiding this comment.
I believe the early return here is justified unless I'm mistaken, and adding the else clause isn't necessary either. I think we should bring the structure back to a similar state of what it was before (early return if null, no else needed).
What happens now when focusedSession == null:
taskbarSession = GetActiveMediaSession(); // first check, null
update taskbar ui, but don't return
focusedSession = GetActiveMediaSession(); // a second time, null again
if (focusedSession == null) return; // returns after calling same method again
Additionally, variable taskbarSession is the same as focusedSession. We should keep the focusedSession naming!
|
|
||
| var thumbnail = BitmapHelper.GetThumbnail(songInfo.Thumbnail); | ||
| BitmapHelper.GetDominantColors(1); | ||
| taskbarWindow?.UpdateUi(songInfo.Title, songInfo.Artist, thumbnail, playbackInfo.PlaybackStatus, playbackInfo.Controls); |
There was a problem hiding this comment.
If I'm not mistaken, this change is unnecessary right? Correct me if I'm wrong!
There was a problem hiding this comment.
Yes, you are correct! I have gone ahead and fixed it.
| @@ -0,0 +1,141 @@ | |||
| // Copyright � 2024-2026 The FluentFlyout Authors | |||
There was a problem hiding this comment.
For some reason, the copyright symbol is a question mark here.
- Removed the `ExtractAppName()` function in favor of using the already-existing `getMediaPlayerData()` function
- Reintroduced early return statement - Renamed `taskbarSession` back to `focusedSession` - Removed unneeded else indentation
- Replaced malformed unicode with proper copyright unicode
|
Hello, thank you for the prompt review! I have addressed all the issues you pointed out earlier:
I also had noticed that all the other tabs have an image describing their function with this specifically standing out as one without; would you like me to try to find an image suitable for it? Thank you! |
unchihugo
left a comment
unchihugo
left a comment
There was a problem hiding this comment.
Hi, thanks for the quick refactor, though could you answer my questions I've noted in the code review? Just asking since I'm a bit busy and can't load up the build right now :)
I also had noticed that all the other tabs have an image describing their function with this specifically standing out as one without; would you like me to try to find an image suitable for it?
I was thinking of having this be a subpage from the System page instead (kind of like the Advanced Settings button there). Therefore we wouldn't need an image for it either currently!
PS: have you tested whether the manual adding works?
| if (mainWindow?.mediaManager != null) | ||
| { | ||
| var apps = mainWindow.mediaManager.CurrentMediaSessions.Values | ||
| .Select(s => MediaPlayerData.getMediaPlayerData(s.Id).Item1) |
There was a problem hiding this comment.
Could you explain why you have to specify Item1 here?
There was a problem hiding this comment.
I chose to specifically pick Item1 because the getMediaPlayerData() function returns both (string, ImageSource) and I specifically need only the title of the application.
|
Hello, thanks for the response! I will try moving it to the Systems page as you specified earlier. Regarding manual adding, it has worked with the apps I've tried (e.g. Chrome, Edge, Firefox, etc.) but I haven't come across an app that wouldn't be displayed on the default dropdown yet; I will try to test that case soon. I will have the changes done hopefully by the end of today; I will try adding pictures or a video if you are unable to build it currently. |
|
Okay, that makes sense - will wait for updates! |
|
Currently I have these for the system, I will post further images/videos in some time. I've removed the tab for App Filtering in the left-side hamburger menu and placed it here instead. The UI for the actual App Filtering page looks a little something like this: I was able to confirm that manually adding an app works for all apps that I had tested from. I am working on removing it from the main page and tidying up the code a bit. |
|
That's good! I'm thinking about the user experience with the current UI - it took me a bit to figure out what was happening myself, so I think we should definitely improve UX before we ship this to average users. We can brainstorm this after you're ready. A lot of users on GitHub called this feature something like whitelist/blacklist instead of filtering. |
|
Hello! Yes, the UX needs to be improved, if you have any ideas I am open to them as I am not very good at UX myself. I'm about to commit the changes where the button is removed from the hamburger menu and main menu; it should hopefully be a matter of naming and UX now. |
- Move the app filtering feature to the system menu
- Fix `.exe` parsing when adding custom application - Inverted if statements for easier readability - Changed type casting to use the `as` keyword for readability
- Update app comparison to compare manual additions against dropdown selections - Move `.exe` comparison within new comparison method
|
Hello, @darklinkpower made some great points surrounding the UI, the dropdown feature seems like a much better fit than what I had created. Regarding the UI, I intend to push the icon change and try to implement the feature in a little bit of time; if you could take over the UI after that would be great! |
- Add disk caching for app icons as apps may not always be open - Add `AppNameIconConverter` to convert an apps name to an icon matching the app using memory cache or disk
- Add dedicated whitelist/blacklist mode - Add app icon for apps in either entry - Add padding to dropdown entries - Update dictionary entries to match new scheme
|
I believe I should have the app icon implemented properly; I added in retrieving an icon from disk if it is not in cache as some apps may not be open and still may be on either the whitelist or blacklist. I tried my best with changing the UI to meet the ideas and address the issues you both had outlined earlier. I am still new to C# and .NET though so I cannot say it works 100%, but I believe it should be fine. The format feels as if it's missing something but I'm unsure myself what that could be. If you could look at the UI/take over the UI now that would be great! |
|
@galagyy I'll have a look at it once I'm free! By the way, are you able to exclusively use the MediaPlayerData method? No worries if it doesn't return an icon for now. We have some MediaPlayerData icon utilities in the Volume Flyout branch, and adding more methods might make things too complicated or add some redundancy. We can also work on icons in another branch/PR if you prefer to get this feature merged first. |
|
Hello! Yes, I could do that; do you want me to add the disk retrieval to the I could also go about using the old method but having the icon only when the blacklisted/whitelisted application is open doesn't sound like the best idea. |
You can remove it entirely for now. The thing is, in the Volume Flyout branch there's new methods to find icons and names more accurately automatically, so I think we should see how that works out first once both branches are merged! |
- Revert `MediaPlayerData` to use previous implementation - Remove `AppNameIconConverter` as it's no longer needed - Remove app icons from the whitelist/blacklist UI
|
Alright, I've gone ahead and removed the icons for now as you suggested! I'll await the Volume Flyout's implementation later. |
- Remove `AppFilteringRulesTitle` as it was redundant
I somehow glossed over this, I have gone through the code and removed the redundant one. |
galagyy
changed the title
If you would like, @unchihugo, I could try implementing the UI changes/ideas you had yourself; I spent some time looking into UI and I feel I can try implementing those changes. |
|
I've been quite busy lately, sorry! I will definitely get around to looking into this but you can have a go at it if you wish! |
|
All good! I'll try a few things in a bit of time. |
|
I'm going to try to fix the merge conflicts this weekend and try the UI changes; I had a few planned but did not get around to adding it. |
|
Sounds good! I've also found a similar feature in MS PowerToys - perhaps you could find inspiration or design the elements similarly to this? It doesn't look confusing and gets the point across really quickly |
|
Good idea! I'll try having that UI done by Monday as well as the merge conflicts! |
|
I just fixed the merge conflict, I will work on the UI shortly. |
- Fix merge conflict with the `master` branch - Fix formatting to match project specifications
- Remove old filtering as many media players use background processes that don't hold the main window itself
|
Hello, Here are the UI changes for the app filtering page; I tried keeping it similar to the PowerToys reference image @unchihugo gave while keeping the dropdown and custom add option. I've also updated the dictionary definitions to be more clear to the user with simpler language. Link to the page
Dropdown to pick between whitelist and blacklist
Whitelist option
Blacklist option
|
- Change language to have more layman's terms - Change page format to be easier to understand
|
Thank you @galagyy, looks great! I'll test it out soon :) |
- Add dropdown icons to the whitelist and blacklist dropdown - Center the dropdown to be more user-friendly
- Fix issue where selecting certain areas in the dropdown did not select the option
- Add hint to app dropdown for user experience
|
I asked a few people for feedback with the UI and I've gone ahead and added these slight changes in response to what I heard:
Nothing massive changed logic-wise, just mainly UI changes. With respect to the dropdown alignment change, I can change it back if you feel it to be a bit off. Example:
|
3 participants
Summary
This PR introduces an app filtering feature. I had seen a feature request, specifically #633, which discusses an idea for a feature similar to this.
This allows the user to create a custom "Allow list" and "Block list" where users may pick which apps they want affecting the flyout and taskbar player.
Motivation
As mentioned above, feature request #633 introduced an idea of a feature similar to this. This request has more features they want added so I do not suggest closing it yet.
Type of Change
What Changed
IsSessionAllowed(), to validate the current session. This function has replaced the previousGetFocusedSession().Additional Information
Originally I had the filter list housed under the taskbar section; I have moved it to its own section due to how it affects both the taskbar and flyout.
NOTE: I have also gone ahead and added the copyright disclaimer on top of the files added, but I am unsure if I was supposed to do that.
Checklist