added support for shell icons for files on Windows #63

iksi4prs · 2023-10-01T11:13:21Z

Support for shell icons for files on Windows.

Added support for shell icons on Windows.
It is planned to add support to Linux, so common components, like cache, were implemented as non-platform specific.
For images cache, since System.Drawing is Windows specific, and Avalonia is not referenced in Abstractions module, an addition of abstract class for image model was needed.
On Windows, the shell icons are implemented differently for files (data files eg .docx, etc, and exe files) and for folders (folder can also include desktop.ini, etc), so this feature relates only to files, not folders (hence also name in settings)
It is planned also to add support for folders later.

See attached images.

IngvarX

Hey @iksi4prs ! Thanks for your PR. Please see my file specific comments below. Some general comments here:

Please make sure you follow current code formatting for consistency
Please add tests where possible/exclude some classes from tests coverage if needed

IngvarX · 2023-10-03T23:26:35Z

src/Camelot.Services/IconsSettingsService.cs

+    private readonly IconsSettingsModel _builtin;
+    private readonly IUnitOfWorkFactory _unitOfWorkFactory;
+    private readonly Platform _platform;
+    private IconsSettingsModel _cachedValue = null;


We don't really need to cache values here because calling DB is rare and doesn't take too much time

every time we draw an icon for a file, we need to check if to use shell-icon or bulit-in, so this value should be kept somewhere, no point in checking db every time

Please see my comment about loading it on startup

IngvarX · 2023-10-03T23:33:34Z

src/Camelot.Services/IconsSettingsService.cs

+
+    public IconsSettingsModel GetIconsSettings()
+    {
+        if (_platform != Platform.Windows)


I would recommend to avoid platform-specific code here. Just allow to read/save as usual. See my comment regarding settings for more info

I kept this as lower level possible, so implementers of feature for linux/mac, will have less to do.

IngvarX · 2023-10-03T23:34:29Z

src/Camelot.ViewModels/Implementations/Dialogs/SettingsDialogViewModel.cs

@@ -30,15 +31,18 @@ public int SelectedIndex

    public SettingsDialogViewModel(
        ISettingsViewModel generalSettingsViewModel,
-        ISettingsViewModel terminalSettingsViewModel)
+        ISettingsViewModel terminalSettingsViewModel,
+        ISettingsViewModel iconsSettingsViewModel)


Let's simply don't show icon VM for non-windows environments.

As I wrote before, I kept this as lower level possible, so implementers of feature for linux/mac, will have less to do.

ok, if Linux/Mac PRs are incoming I'm ok with keeping it as is

IngvarX · 2023-10-03T23:40:38Z

src/Camelot.ViewModels/Implementations/MainWindow/FilePanels/Nodes/FileViewModel.cs

    private long _size;
+    private Bitmap _shellIcon = null;


= null is redundant

Habit. used to newer projects with Bitmap?.

IngvarX · 2023-10-03T23:41:34Z

src/Camelot.ViewModels/Implementations/MainWindow/FilePanels/Nodes/FileViewModel.cs

+
+    private IconsType GetUserSelectedType()
+    {
+        var model = _iconsSettingsService.GetIconsSettings();


I'm ok with settings applying only after restart so I think we can just inject this value from factory (where we can read it once) instead of doing this call every time

As a user, very confusing. I applied the theme, and didn't understand why nothing happens.

True but it's a tradeoff between code complexity and functionality. In the future I'm planning to add automatic restart to avoid that issue. Loading and storing this value on start saves us a lot of effort of loading it during runtime

I think Avalonia v11 has different way of handling theme, that support it (at least for themes)

IngvarX · 2023-10-04T00:21:49Z

src/Camelot.Services.AllPlatforms/ShellIconsCacheService.cs

+
+namespace Camelot.Services.AllPlatforms;
+
+public class ShellIconsCacheService : IShellIconsCacheService


I think we should leave "Cache" part of the name for this class out because we don't really want to expose to consumers such details as using cache internally. It seems that we use ShellIconsService only here so moving all code to the same file sounds fine to me

ShellIconsService is per platform, just asking platform for icon, based on file extension or application.
ShellIconsCacheService is higher level, shared to all platform, that checks if it already has icon for extension.
eg for file1.txt and file2.txt, ShellIconsCacheService will call ShellIconsService only on 1st time.
on 2nd time, it will use cache, as it already has icon for ".txt".

I got the idea but I don't like the naming, we should not let consumers of this class know that there is some cache being used. I recommend to implement this caching logic as a decorator or put it in some base class and for platforms just add some platform-specific implementation

IngvarX · 2023-10-04T00:24:58Z

src/Camelot.Services.AllPlatforms/ShellIconsCacheService.cs

+{
+    private readonly IShellLinksService _shellLinksService;
+    private readonly IShellIconsService _shellIconsService;
+    private readonly Dictionary<string, Bitmap> _cache = new();


What happens if the cache grows too large? Should we enforce some size limits?

Good question. Cache is limited to number of extensions and apps.
I guess for normal user, if he has 30 extensions and some 100 apps, then it will not grow more than 150-200 icons.
But its a good idea to add some limit.

IngvarX · 2023-10-04T00:25:37Z

src/Camelot.Services.AllPlatforms/ShellIconsCacheService.cs

+        {
+            case IShellIconsService.ShellIconType.Extension:
+                {
+                    var ext = Path.GetExtension(path);


Please use IPathService

IngvarX · 2023-10-04T00:29:08Z

src/Camelot.Services.Abstractions/IShellIconsService.cs

+
+namespace Camelot.Services.Abstractions;
+
+public interface IShellIconsService


These class & interface should be merged with IShellService with some methods being private

see notes before about IShellIconsService as per platform.

IngvarX · 2023-10-04T00:37:25Z

src/Camelot.Images/ConcreteImage.cs

+
+public class ConcreteImage : ImageModel
+{
+    public Bitmap Bitmap { get; }


Is there a way to follow one of approaches mentioned here? https://learn.microsoft.com/en-us/dotnet/core/compatibility/core-libraries/6.0/system-drawing-common-windows-only Current solution looks hacky. Alternative approach would be to get rid of Camelot.Images and create separate projects for platform specific stuff instead of touching services/abstractions projects that designed for a business logic and not UI stuff

This is the trickiest thing on this PR.
I understand the separation of logic and UI.
But here, I think although its "image", it is part of business logic.
At first I thought as a solution to use Skia, or something like that, but it is actually not necessary.
Avalonia provides all that is necessary, just need some Image class in the abstractions layer.

I see. My recommendation is:

Create Camelot.UI.AllPlatforms and corresponding platform specific projects if needed

Move this logic there. Services was designed for a business logic and not rendering stuff. Feel free to use Avalonia in new projects instead. It's totally ok to have a different logic for rendering on different platforms (surprisingly it didn't happen before).

Camelot.Images can be removed in this case and all casts won't be necessary anymore

a. "Create Camelot.UI.AllPlatforms" - can u send picture when do u want it in solution ?
b. "Move this logic there" - do u mean all the icons services ?

I will actually try to build and push this change today in the evening

codecov-commenter · 2023-10-06T03:13:24Z

Codecov Report

Attention: 181 lines in your changes are missing coverage. Please review.

Comparison is base (379a713) 97.68% compared to head (c7b4e2e) 92.89%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
- Coverage   97.68%   92.89%   -4.79%     
==========================================
  Files         177      185       +8     
  Lines        4619     4830     +211     
  Branches        0      388     +388     
==========================================
- Hits         4512     4487      -25     
- Misses        107      288     +181     
- Partials        0       55      +55

Files	Coverage Δ
.../Implementations/FileSystemNodeViewModelFactory.cs	`100.00% <100.00%> (ø)`
...Implementations/Dialogs/SettingsDialogViewModel.cs	`100.00% <100.00%> (ø)`
...iewModels/Services/Interfaces/Models/ImageModel.cs	`0.00% <0.00%> (ø)`
....ViewModels.Windows/ShellIcons/WindowsIconTypes.cs	`0.00% <0.00%> (ø)`
...tions/MainWindow/FilePanels/Nodes/FileViewModel.cs	`56.66% <20.00%> (-43.34%)`	⬇️
src/Camelot.Services/IconsSettingsService.cs	`40.90% <40.90%> (ø)`
...els.Windows/ShellIcons/WindowsShellLinksService.cs	`0.00% <0.00%> (ø)`
...Implementations/Settings/IconsSettingsViewModel.cs	`23.52% <23.52%> (ø)`
...ShellIcons/SystemImageToAvaloniaBitmapConverter.cs	`0.00% <0.00%> (ø)`
...els.Windows/ShellIcons/WindowsShellIconsService.cs	`0.00% <0.00%> (ø)`
... and 1 more

... and 30 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

IngvarX · 2023-10-06T03:14:08Z

Pushed some changes to avoid casts, unfortunately tests are still broken. Next step will be to address tests and do refactoring

IngvarX · 2023-10-06T05:53:16Z

Fixed tests in the follow up commit. So just refactoring/addressing PR comments is left

IngvarX · 2023-10-10T02:25:07Z

src/Camelot.Services.Windows/ShellIcons/WindowsIconTypes.cs

+                return true; // icon in indexed resource
+
+            var ext = Path.GetExtension(filename).ToLower();
+            var extensionsOfIcoFiles = new List<string>() { ".ico", ".exe", ".dll" , ".cpl", ".appref-ms", ".msc" };


I suspect that this list should be in sync with WindowsShellIconsService and this code might be refactored to avoid this class creation at all

IngvarX · 2023-10-21T03:57:52Z

Thanks, merged!

iksi4prs added 2 commits October 1, 2023 13:41

added support for shell icons on Windows

ac67f0d

cleanup

befc589

IngvarX reviewed Oct 4, 2023

View reviewed changes

Refactored using icons to avoid casts

1188739

Fixed tests for Icons

f13a258

Used path service and minor refactoring

b30cae6

IngvarX reviewed Oct 10, 2023

View reviewed changes

IngvarX and others added 3 commits October 9, 2023 19:30

Adjusted stylings in shell icons files

bf9f947

Camelot.ViewModels.Windows restructuring

98f4b77

fixed, case where value is wrapped with double-quotes

c7b4e2e

IngvarX merged commit 1a513c0 into IngvarX:master Oct 21, 2023
3 checks passed

iksi4prs deleted the shell-icons-windows branch October 22, 2023 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added support for shell icons for files on Windows #63

added support for shell icons for files on Windows #63

iksi4prs commented Oct 1, 2023

IngvarX left a comment

IngvarX Oct 3, 2023

iksi4prs Oct 4, 2023

IngvarX Oct 5, 2023

IngvarX Oct 3, 2023

iksi4prs Oct 4, 2023

IngvarX Oct 3, 2023

iksi4prs Oct 4, 2023

IngvarX Oct 5, 2023

IngvarX Oct 3, 2023

iksi4prs Oct 4, 2023

IngvarX Oct 3, 2023

iksi4prs Oct 4, 2023

IngvarX Oct 5, 2023

iksi4prs Oct 5, 2023

IngvarX Oct 4, 2023

iksi4prs Oct 4, 2023 •

edited

Loading

IngvarX Oct 5, 2023

IngvarX Oct 4, 2023

iksi4prs Oct 4, 2023

IngvarX Oct 4, 2023

iksi4prs Oct 4, 2023

IngvarX Oct 4, 2023

iksi4prs Oct 4, 2023

IngvarX Oct 4, 2023

iksi4prs Oct 4, 2023

IngvarX Oct 5, 2023

iksi4prs Oct 5, 2023

IngvarX Oct 5, 2023

codecov-commenter commented Oct 6, 2023 •

edited

Loading

IngvarX commented Oct 6, 2023

IngvarX commented Oct 6, 2023

IngvarX Oct 10, 2023

IngvarX commented Oct 21, 2023


		namespace Camelot.Services.AllPlatforms;

		public class ShellIconsCacheService : IShellIconsCacheService


		namespace Camelot.Services.Abstractions;

		public interface IShellIconsService

added support for shell icons for files on Windows #63

added support for shell icons for files on Windows #63

Conversation

iksi4prs commented Oct 1, 2023

IngvarX 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

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

iksi4prs Oct 4, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 6, 2023 • edited Loading

Codecov Report

IngvarX commented Oct 6, 2023

IngvarX commented Oct 6, 2023

Choose a reason for hiding this comment

IngvarX commented Oct 21, 2023

iksi4prs Oct 4, 2023 •

edited

Loading

codecov-commenter commented Oct 6, 2023 •

edited

Loading