Code Quality: Icon improvements #14792
Conversation
yaira2
commented
Feb 20, 2024
•
yaira2
commented
- added an IconOptions enum
- separated code for retrieving icons & icon overlays
yaira2
force-pushed
the
ya/GetOverlay
branch
4 times, most recently
from
6b17da7 to
1779e4e
Compare
yaira2
changed the title
| /// </summary> | ||
| public float AppWindowDpi | ||
| private float appWindowDPI = InteropHelpers.GetDpiForWindow(MainWindow.Instance.WindowHandle) / 96f; |
There was a problem hiding this comment.
According to the coding guideline, this should be _AppWindowDPI - a field backing a property.
There was a problem hiding this comment.
I updated the other properties in AppModel as well in order to be consistent
| /// <returns></returns> | ||
| public static (byte[]? icon, byte[]? overlay, bool isIconCached) GetFileIconAndOverlay( | ||
| public static (byte[]? icon, bool isIconCached) GetIcon( |
There was a problem hiding this comment.
You could use out variable for isIconCached.
There was a problem hiding this comment.
What changes would need to be made and what's the benefit?
There was a problem hiding this comment.
Just to avoid returning tuple type. I'd like methods to return single variable(struct type, pre-defined types) to have single resposibility.
Now
public static async Task<(byte[] IconData, bool isIconCached)> GetIconAsync(
string path,
uint requestedSize,
bool isFolder,
bool getThumbnailOnly,
IconOptions iconOptions)After
public static async Task<byte[] IconData> GetIconAsync(
string path,
uint requestedSize,
bool isFolder,
bool getThumbnailOnly,
IconOptions iconOptions,
out bool isIconCached)Besides, I see isIconCached often is not used.
So when you want to discard, you can just do:
public static async Task<byte[] IconData> GetIconAsync(
"",
0,
false,
false,
IconOptions.None,
out var _)There was a problem hiding this comment.
But this is just my preferences for codebase quality.
There was a problem hiding this comment.
We can do this in a future phase 👍
| @@ -97,7 +97,9 @@ protected override void OnNavigatedTo(NavigationEventArgs e) | |||
| { | |||
| ViewModel.IsAblumCoverModified = true; | |||
| ViewModel.ModifiedAlbumCover = new Picture(file.Path); | |||
| ViewModel.IconData = await FileThumbnailHelper.LoadIconWithoutOverlayAsync(file.Path, Constants.ShellIconSizes.ExtraLarge, false, false, false, true); | |||
|
|
|||
| var result = await FileThumbnailHelper.GetIconAsync(file.Path, Constants.ShellIconSizes.ExtraLarge, false, false, IconOptions.UseCurrentScale); | |||
| var iconData = await FileThumbnailHelper.LoadIconWithoutOverlayAsync(Item.ItemPath, Constants.ShellIconSizes.Jumbo, false, false, false, false); | ||
| if (iconData is not null) | ||
| await MainWindow.Instance.DispatcherQueue.EnqueueOrInvokeAsync(async () => FileImage = await iconData.ToBitmapAsync()); | ||
| var result = await FileThumbnailHelper.GetIconAsync(Item.ItemPath, Constants.ShellIconSizes.Jumbo, false, false, IconOptions.None); |
There was a problem hiding this comment.
As above. I think this line should one more line break.
There was a problem hiding this comment.
I'm still working on the code in GetIconAsync so I'd rather wait until that's complete before putting time into the formatting.
| /// <summary> | ||
| /// Behavior used to retrieve and adjust icons | ||
| /// </summary> | ||
| public enum IconOptions |
There was a problem hiding this comment.
This doesn't need [Flags] attribute?
yaira2
added
ready to merge
