Flow-Launcher / Flow.Launcher

:mag: Quick file search & app launcher for Windows with community-made plugins
https://flowlauncher.com
MIT License
7.82k stars 307 forks source link

Result comparison causing Query result's action to be outdated #2812

Open HorridModz opened 3 months ago

HorridModz commented 3 months ago

I've been having a weird issue with my Add2Path plugin, which has been present ever since I started developing it. It seems that in my Query method (which returns the results for a query), query is getting outdated.

My plugin operates on one-word commands, like add and remove, which take the succeeding text in the query as the command's input:

path add foo - command add with input foo path remove bar - command remove with input bar

I've been noticing that the input part - foo and bar - doesn't always update. It seems the input stays the same, even when I change . This has been bugging me a lot, and it makes the plugin practically unusable. So I did some investigating. I replaced my Query method with a simple tester that tells me the "query" passed to it when I click the result:

public List<Result> Query(Query query)
    {
        return new List<Result>
        {
            new Result
            {
                Title = "test - view query",
                SubTitle = "view query",
                Action = c => { Context.API.ShowMsg("Query", query.Search); return false; }
            }
        };
    }

Sure enough, the strange behavior occurs here. It might be better to fire it up and see yourself, but I'll try to explain what I'm experiencing:

  1. I open FlowLauncher and type my action keyword (path), so the query is path.
  2. I click the test result and get a notification reporting that the query is blank (as it should be - after the action keyword path, the query is just blank)
  3. I type the characters hello world, leading to the query being: path hello word.
  4. I once again click the test result, and the notification once again reports that the query is blank (this is wrong; the query should be hello world)
  5. Keep clicking, the buggy behavior still occurs and the reported query does not update
  6. Now, delete the text path (leaving hello world intact), and retype it.
  7. Click the test result. This time, it properly returns the query: hello world, rather than reporting that it is blank (this is right, though the query is the same as it was last time, in step 4)

So, why's it work the second time but not the first time? It seems that deleting and re-adding the action keyword (path) forces it to update. This means that simply changing the input - from blank to hello world does not trigger an update.

However, if I change the code to report the query directly passed into the Query method rather than the Action function in one of the results, it works properly:

public List<Result> Query(Query query)
    {
        return new List<Result>
        {
            new Result
            {
                Title = "Query",
                SubTitle = query.Search,
                Action = c => { return false; }
            }
        };
    }

This means that the Query method is getting passed the properly updated query parameter, as it should. However, the Action in the returned result has the old query parameter.

This perplexes me, as the Query method is creating a new Result every time it is called - and if the query parameter is correct, it should be passing it to the result's Action - right?

But no, the Action is old and only updates when the result visually updates. I'm not very experienced with C#, but there seems to be two possible things that could cause this:

  1. The Result constructor is not working intuitively, perhaps using some kind of caching. Thus, when I initialize the Action field in the new Result I am constructing and returning, it is not really making a new one but giving me an old one - which still contains the old Action field with the old query value.
  2. The Result constructor is indeed creating a new Result object when I initialize and return it, but the Action field is not being updated. Maybe because of some rule in C# having to do with delegates as fields? This seems unlikely as I can't see how it could happen, but it's a possibility as I don't understand how C# handles stuff under the hood.

Of course, I'm leading toward my first theory, which suggests something happening with FlowLauncher's handling of Results and their Actions rather than my plugin. But I'm completely perplexed here, and I wouldn't be surprised if I'm just doing something wrong and overlooking something silly.

If you'd like my full plugin's code, let me know (though I believe that the initial, published version of the plugin, which is in the official plugins directory, contains this bug itself).

HorridModz commented 3 months ago

@jjw24 @taooceros

taooceros commented 3 months ago

Yes there are some caching. You can invalidate the cache by modifying the title/subtitle/score. This is a subtle issue which we haven't solved.

jjw24 commented 3 months ago

@taooceros can you expand more on your answer please, the behaviour of 'changing the input - from blank to hello world' should trigger a result update.

JohnTheGr8 commented 3 months ago

@taooceros can you expand more on your answer please, the behaviour of 'changing the input - from blank to hello world' should trigger a result update.

@HorridModz is changing the query input, but the plugin results are visually the same as before, and Flow just keeps the old results that have been cached. This is mostly to avoid UI flickering iirc.

jjw24 commented 3 months ago

Ok I see, this is actually to do with how we are comparing existing and updated result, it's comparing on title, subtitle and score. Therefore your updated result must have one of the three different to the existing result, that's why your second plugin example is working because subtitle is different.

JohnTheGr8 commented 3 months ago

If I remember correctly, it's more than just Title, Subtitle and Score that affect this behavior. Pretty sure other fields that change the result visually (like TitleHighlightData and AutoCompleteText) also do.

In any case, this part of Flow needs to receive some attention. Should we move this issue to the main repo?

HorridModz commented 3 months ago

Woah, this seems much more complicated than what I originally thought. But why is this undocumented? And what even is the point of it? With absolutely no override?

HorridModz commented 3 months ago

Yes there are some caching. You can invalidate the cache by modifying the title/subtitle/score. This is a subtle issue which we haven't solved.

Interesting, is there a way to force invalidate the cache for results? Or some simple way to just reload Result. When I create a new Result, why would it cache? That seems so unintuitive, especially with no way to bypass caching.

HorridModz commented 3 months ago

Reference in new i

@JohnTheGr8 I put this here because I was unsure if it happened in other languages. From my perspective, this is a major issue if it can lead a developer like myself to have perfectly fine code that is bugging due to a weird, undocumented type of caching - so if it's embedded in the design, it has to at least be documented.

HorridModz commented 3 months ago

@HorridModz is changing the query input, but the plugin results are visually the same as before, and Flow just keeps the old results that have been cached. This is mostly to avoid UI flickering iirc.

Why should a visual cache be applied to something non-visual like the result Action? That sounds inherently flawed. Caching these 3 things but not caching the Action should make it so the UI won't flicker while fixing the issue.

jjw24 commented 3 months ago

If I remember correctly, it's more than just Title, Subtitle and Score that affect this behavior. Pretty sure other fields that change the result visually (like TitleHighlightData and AutoCompleteText) also do.

In any case, this part of Flow needs to receive some attention. Should we move this issue to the main repo?

For reference this is the part of the code: https://github.com/Flow-Launcher/Flow.Launcher/blob/94288b3ebab9e0ff25c20483d3f2b8ddf21b34eb/Flow.Launcher.Plugin/Result.cs#L160

jjw24 commented 3 months ago

I agree with documentation, and unfortunately we also struggle to keep it up like other projects.

Not sure about the caching part since I haven't worked on this part of the code so maybe the others could shed some light what it is, I just thought the result comparison code is the root cause of this issue.

Moving this to main.

HorridModz commented 3 months ago

You can invalidate the cache by modifying the title/subtitle/score.

@taooceros In the meantime, do you have a suggestion for how to do this? Perhaps I could change the score as a workaround, as I always have only one result? In my plugin, this makes it hell to debug and confusing to use - so I can't just pretend there's no issue.

taooceros commented 3 months ago

I don't think it is necessary to cache old result to avoid flickering. I will post a pr recently to remove the cache and see the result.

taooceros commented 3 months ago

You can invalidate the cache by modifying the title/subtitle/score.

@taooceros In the meantime, do you have a suggestion for how to do this? Perhaps I could change the score as a workaround, as I always have only one result? In my plugin, this makes it hell to debug and confusing to use - so I can't just pretend there's no issue.

Yes I think that's good workaround.

dxp227 commented 5 days ago

I lost days on this bug 😂😭 I haven't been so baffled in a long time 😅

HorridModz commented 5 days ago

@dxp227

I lost days on this bug 😂😭 I haven't been so baffled in a long time 😅

I gave up on the bug and just published the plugin, thinking I was crazy - I didn't even realize something funny was going on until I went back to update it! And yes, I also spent days - glad you made it here. As I said, this NEEDS to be documented!