Allow plugins to use web URL's for icon paths by Garulf · Pull Request #1351 · Flow-Launcher/Flow.Launcher

Garulf · 2022-08-30T05:54:43Z

This PR allows Flow Launcher to accept remote URL's provided in the IcoPath field.

This greatly speeds up results.

taooceros

Nice change, but requires a bit (maybe quite a bit🤣) modification to make it perfect.

Flow.Launcher.Infrastructure/Image/ImageLoader.cs

Flow.Launcher.Plugin/Result.cs

taooceros · 2022-10-27T16:27:43Z

https://stackoverflow.com/questions/36610442/bitmapimage-how-do-i-wait-for-bitmapimage-to-be-initialized-before-returning-im

I will adjust the code a bit before marking ready.

…onse)

…low.Launcher into handle-icon-urls

jjw24 · 2022-11-14T09:58:52Z

@Garulf or @taooceros please resolve conflict

…low.Launcher into handle-icon-urls

jjw24 · 2022-11-22T02:55:11Z

Flow.Launcher.Infrastructure/Http/Http.cs

        {
            Log.Debug($"|Http.Get|Url <{url}>");
-            return GetAsync(new Uri(url.Replace("#", "%23")), token);
+            return GetAsync(new Uri(url), token);


Is this change intended?

yeah I don't think that's needed since it is creating a url.

what do you mean? the old code is also creating a url too no?

Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs

jjw24 · 2022-11-22T03:01:38Z

Please update PR description on what tests have been done.

taooceros · 2022-10-30T15:58:13Z

Flow.Launcher.Infrastructure/Http/Http.cs

        }

        /// <summary>
-        /// Asynchrously get the result as stream from url.


I revised some of the Http design to align with the HttpClient design. Their design is much better

taooceros · 2022-10-30T15:58:59Z

Flow.Launcher.Infrastructure/Image/ImageLoader.cs

                    {
-                        Load(x.Key);
-                    });
+                        await LoadAsync(imageUsage.Key);


Due to the ImageLoader change to async (not always async so ValueTask). I wonder whether we should use Task to wrap the nonasync part so we can always just await it.

taooceros · 2022-10-30T15:59:39Z

Flow.Launcher.Infrastructure/Image/ImageLoader.cs

+                    image.CacheOption = BitmapCacheOption.OnLoad;
+                    image.StreamSource = buffer;
+                    image.EndInit();
+                    image.StreamSource = null;


to make the streamsource collectable

taooceros · 2022-10-30T16:00:29Z

Flow.Launcher.Infrastructure/Image/ImageLoader.cs

+                    image.StreamSource = buffer;
+                    image.EndInit();
+                    image.StreamSource = null;
+                    image.Freeze();


Freeze is needed because the BitmapImage is not created in the ui thread.

taooceros · 2022-11-22T05:28:00Z

Flow.Launcher.Infrastructure/Http/Http.cs

        {
            Log.Debug($"|Http.Get|Url <{url}>");
-            return GetAsync(new Uri(url.Replace("#", "%23")), token);
+            return GetAsync(new Uri(url), token);


yeah I don't think that's needed since it is creating a url.

Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs

jjw24 · 2022-11-22T10:08:16Z

Flow.Launcher.Plugin/Result.cs

-                    _icoPath = Path.Combine(value, IcoPath);
+                    string absPath = Path.Combine(value, IcoPath);
+                    // Only convert relative paths if its a valid path
+                    if (File.Exists(absPath))


Is there another way to check this path without using File.Exists? Doesn't seem necessary to use it here.

This way allows us to improve how Flow auto appends the plugins path to icon paths that are relative and still accept URI such as http, https, ftp, etc. instead of checking every URI under the sun to see if it's valid let's just detect if that file even exists and leave it alone if it doesn't.

Edit: original code just tacked on the plugins path to any string it didn't detect a root file path on.

I mean this is an unnecessary call because we already checking that the path exists or not at

Flow.Launcher/Flow.Launcher.Infrastructure/Image/ImageLoader.cs

Lines 183 to 202 in e5948a7

private static ImageResult GetThumbnailResult(ref string path, bool loadFullImage = false)

{

ImageSource image;

ImageType type = ImageType.Error;

if (Directory.Exists(path))

{

/* Directories can also have thumbnails instead of shell icons.

* Generating thumbnails for a bunch of folder results while scrolling

* could have a big impact on performance and Flow.Launcher responsibility.

* - Solution: just load the icon

*/

type = ImageType.Folder;

image = GetThumbnail(path, ThumbnailOptions.IconOnly);

}

else if (File.Exists(path))

{

var extension = Path.GetExtension(path).ToLower();

if (ImageExtensions.Contains(extension))

{

(GetThumbnailResult is called by LoadInternalAsync)

We just need to determine that it is not a URI.

We're not really concerned with if the path exists or not. What we're really concerned with is the path really a relative path or not.

The built-in method for detecting relative paths isnt very smart.

The built-in method for detecting relative paths isnt very smart

Agree, using Uri class is not working for our cases. Just doing a simple check for http and https seems more effective to determine that it is Url, then consider everything else as local absolute/relative path.

@Garulf @taooceros I reworked IcoPath, PluginDirectory and ImageLoader 83ec809, let me know if ok.

jjw24 · 2022-11-22T21:19:32Z

Flow.Launcher.Plugin/Result.cs

@@ -62,7 +62,13 @@ public string IcoPath
            {
                if (!string.IsNullOrEmpty(PluginDirectory) && !Path.IsPathRooted(value))


@taooceros & @Garulf not related to this PR but I am trying to figure out why we need to check PluginDirectory is not an empty string? I cant seem to hit this condition.

Flow.Launcher.Plugin/Result.cs

…low.Launcher into handle-icon-urls

taooceros · 2022-11-24T20:42:31Z

I remove the await Task.Run outside. However, it sometimes makes the debugger stop when fetching image fails (which throws an error that is caught outside). I wonder whether we should make the method more robust by using return code🤔

resolved

Garulf added 4 commits August 30, 2022 01:48

Add logic for IcoPath's that are web URL's

ffbd8fd

Remove whitespace

03e319e

DOn't prepend pluginpath if IcoPath is a web URL

fda155a

Merge branch 'dev' into handle-icon-urls

b5c2125

taooceros previously requested changes Aug 31, 2022

View reviewed changes

Garulf added 2 commits September 21, 2022 12:01

Use URI class to detect web URL

d62d710

Switch from WebClient to HTTP

301882d

jjw24 marked this pull request as draft September 27, 2022 11:11

onesounds assigned Garulf Sep 27, 2022

Garulf and others added 14 commits October 7, 2022 10:47

Only convert relative paths if its a valid path

5c33b0d

// Only convert relative paths if its a valid path

b7900b2

Add logic for IcoPath's that are web URL's

57d63cf

Remove whitespace

ed107cc

DOn't prepend pluginpath if IcoPath is a web URL

7f1b9d0

Use URI class to detect web URL

416c44f

Switch from WebClient to HTTP

ec1a061

Only convert relative paths if its a valid path

ebd6f17

// Only convert relative paths if its a valid path

eb3f723

Adjust Http Code

ef575bb

Adjust Google Code a bit due to http change

3708489

Adjust Bing Code a bit due to http change

191c6af

Adjust Http Design to avoid memory leak

957c4e2

Make ImageLoader Async

eb5e33a

taooceros force-pushed the handle-icon-urls branch from 5baace7 to eb5e33a Compare October 13, 2022 05:41

Revert back some unnecessary change

c39a727

taooceros and others added 4 commits October 27, 2022 11:38

Dispose Stream (though it maybe already disposed by disposed the resp…

99fd8d1

…onse)

remove testing code

2507d1a

resize bitmapimage when not loading full image

ba0aee1

Merge branch 'handle-icon-urls' of https://github.com/Flow-Launcher/F…

cae0b7b

…low.Launcher into handle-icon-urls

jjw24 added enhancement New feature or request review in progress Indicates that a review is in progress for this PR labels Nov 14, 2022

jjw24 added this to the 1.10.0 milestone Nov 14, 2022

jjw24 requested a review from taooceros November 14, 2022 09:59

Garulf and others added 4 commits November 17, 2022 14:30

Merge branch 'dev' into handle-icon-urls

26668b4

Merge branch 'dev' into handle-icon-urls

ce52919

Merge branch 'dev' into handle-icon-urls

414e55c

Merge branch 'handle-icon-urls' of https://github.com/Flow-Launcher/F…

622b130

…low.Launcher into handle-icon-urls

jjw24 assigned taooceros Nov 22, 2022

jjw24 reviewed Nov 22, 2022

View reviewed changes

taooceros reviewed Nov 22, 2022

View reviewed changes

Merge branch 'dev' into handle-icon-urls

e5948a7

jjw24 reviewed Nov 22, 2022

View reviewed changes

Flow.Launcher.Plugin/Result.cs Show resolved Hide resolved

Garulf and others added 5 commits November 23, 2022 01:19

Merge branch 'handle-icon-urls' of https://github.com/Flow-Launcher/F…

67443fd

…low.Launcher into handle-icon-urls

Wrap non async part in task

3665bd9

remove PluginDirectory, ImageLoader path checking, update IcoPath logic

83ec809

remove task.run outside

303d3b9

remove unnecessary local variable

fb3a23f

jjw24 approved these changes Nov 25, 2022

View reviewed changes

jjw24 enabled auto-merge November 25, 2022 02:50

jjw24 merged commit 5bc6acf into dev Nov 25, 2022

jjw24 deleted the handle-icon-urls branch November 25, 2022 02:50

jjw24 removed the review in progress Indicates that a review is in progress for this PR label Nov 25, 2022

jjw24 mentioned this pull request Dec 12, 2022

Release 1.10.0 | Plugin 3.0.0 #957

Merged

	private static ImageResult GetThumbnailResult(ref string path, bool loadFullImage = false)
	{
	ImageSource image;
	ImageType type = ImageType.Error;

	if (Directory.Exists(path))
	{
	/* Directories can also have thumbnails instead of shell icons.
	* Generating thumbnails for a bunch of folder results while scrolling
	* could have a big impact on performance and Flow.Launcher responsibility.
	* - Solution: just load the icon
	*/
	type = ImageType.Folder;
	image = GetThumbnail(path, ThumbnailOptions.IconOnly);
	}
	else if (File.Exists(path))
	{
	var extension = Path.GetExtension(path).ToLower();
	if (ImageExtensions.Contains(extension))
	{

		@@ -62,7 +62,13 @@ public string IcoPath
		{
		if (!string.IsNullOrEmpty(PluginDirectory) && !Path.IsPathRooted(value))

Uh oh!

Comments

Conversation

Garulf commented Aug 30, 2022

Uh oh!

taooceros left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

taooceros commented Oct 27, 2022

Uh oh!

jjw24 commented Nov 14, 2022

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!

jjw24 commented Nov 22, 2022

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!

Garulf Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jjw24 Nov 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

taooceros commented Nov 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Garulf Nov 22, 2022 •

edited

Loading

jjw24 Nov 24, 2022 •

edited

Loading