Add App Filtering Page and Session Validation (ref #633)#652
Add App Filtering Page and Session Validation (ref #633)#652galagyy wants to merge 17 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
- Added documentation to all methods for future maintenance
|
I went ahead and added a few changes for the manual addition, namely better comparison against known sources to prevent two rules for the same app. I also went ahead and added documentation to all the methods for the I believe the only hurdle remaining as you said should be the UI now! |
|
I've tried making it more friendly, using the whitelist and blacklist language you had pointed out earlier; I am still unsure if it is intuitive to the average user though. 
|
- Change page wording to be more intuitive - Reformat UI to be more friendly
unchihugo
left a comment
unchihugo
left a comment
There was a problem hiding this comment.
Look great @galagyy! I think the renaming definitely makes things a bit more clear. We're almost ready to add this feature in, but I've noticed that if you click a media player in the list it adds a colored background (indicating select), are you able to remove that? Thank you!
By the way, I'd love to have icons displayed next to the name in the future; if you'd like contributing more, do you think you could handle that as well?
|
I'll work on the changes when I get back today, they should be ready by the end of today. I will try adding the icons as that seems like a great addition. |
|
Hi, first of all, thank you for working on this feature, it looks great. Hope you don't mind me chiming in with an outsider perspective, but I think there are a few potential issues with the proposed implementation. I think having both a whitelist and blacklist in their current form introduces some ambiguity and UX friction. Right now there are effectively two concurrent filtering models:
The issue is that once the whitelist has any entries, it implicitly overrides the blacklist, making it effectively unused. This creates:
I'm wondering if it might be clearer to make this a single-mode selection feature in a simple dropdown/ComboBox, for example: a) Blacklist mode: Allow all except blocked apps Then show only the relevant list based on the selected mode. This would make the behavior explicit, avoid overlap, and simplify the mental strain for users, resulting in a clearer UX. I'd suggest making "Blacklist" (allow-all except blocked) the default, since it preserves current behavior. If "Whitelist mode" is selected, it should behave consistently regardless of whether the list is empty, meaning no implicit mode or logic-behavior switching based on list contents (Is the list empty or not?). In my opinion avoiding that kind of hidden logic would make the feature more predictable and easier to understand. Apologies if this feedback is unnecessary or wrong, especially when the developer is okay with the current approach. Just wanted to share my perspective in case it's helpful. |
|
Hi @darklinkpower, you're making great points against the current UX. I was also brainstorming about the dropdown -> whitelist / blacklist feature instead of presenting both at once, as I had some difficulties understanding the UI as well. Your feedback definitely clarify the reasons to change implementation, and is incredibly helpful to us. That said, @galagyy if you're able to, could you change the implementation to a dropdown with blacklist and whitelist options separated? I can take over the UI if you'd like! |
|
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
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