Skip to content

Conversation

@jjw24
Copy link
Member

@jjw24 jjw24 commented Jun 24, 2020

This fixes some win32 app loading for Program plugin.

  • More specifically, it is able to retrieve the correct file path for programs in the start menu that have LNK extension, ie. Command Prompt from start menu.

  • Fix the issue where type 'cmd' does not get you Command Prompt

  • Remove ShObjldITIb.dll which was for launching applications using Application Activation Manager and loading app info using Shell Links. We now directly use the code, not needing this dll to be embedded.

  • Remove duplicate lnk program pointing to the same exe with the same description. close Combine/remove duplicate entries #307

@jjw24 jjw24 added the bug Something isn't working label Jun 24, 2020
@jjw24 jjw24 self-assigned this Jun 24, 2020
@jjw24 jjw24 marked this pull request as draft July 12, 2020 10:43
use LnkResolvedPath which is the original lnk path, rather than using the FullPath property which is assigned with the path to the actual exe
@taooceros
Copy link
Member

Should we use LnkResolved Path to distinct programs so that there won't be multiple program items from multiple Link pointing to the same exe?

@jjw24
Copy link
Member Author

jjw24 commented Dec 16, 2020

yeah makes sense, will take a look.

When they are pointing to the same exe, how do we know which to pick?

@taooceros
Copy link
Member

yeah makes sense, will take a look.

When they are pointing to the same exe, how do we know which to pick?

Well I think we can pick those with distinct descriptions, and only pick the one with no description when none contains a description.
I do have taken a trial on that, but the speed seems not fast enough.

@taooceros
Copy link
Member

private class Win32ComparatorBasedonDescription : IEqualityComparer<Win32>
        {
            public static Win32ComparatorBasedonDescription Default = new Win32ComparatorBasedonDescription();

            public bool Equals([AllowNull] Win32 x, [AllowNull] Win32 y)
            {
                return x.Description == y.Description;
            }

            public int GetHashCode([DisallowNull] Win32 obj)
            {
                return obj.Description.GetHashCode();
            }
        }

        private static Win32[] ProgramsHasher(ParallelQuery<Win32> programs)
            => programs
            .GroupBy(p => p.FullPath.ToLower())
            .SelectMany(g =>
            {
                var tempList = g;
                if (tempList.Count() > 1 && tempList.Any(p => !string.IsNullOrEmpty(p.Description)))
                    return g.Where(p => !string.IsNullOrEmpty(p.Description))
                    .Distinct(Win32ComparatorBasedonDescription.Default);
                else
                    return g.Take(1);
            }).ToArray();

Take a look on this, but I think the performance isn't quite satisfied.

@taooceros
Copy link
Member

There is a weird situation..... if replacing Parallel Query with IEnumerable, the running time can reduce hugely significantly...... in my computer, it is about 1000-2000ms to 1-10ms, which is unreasonable.

@taooceros
Copy link
Member

Seems sometime PLINQ do performs slower than LINQ…… especially when the task is simple for each one but contains a lot of split.
https://stackoverflow.com/questions/3354518/plinq-performs-worse-than-usual-linq

@taooceros
Copy link
Member

taooceros commented Dec 19, 2020

Found out that changing the Parallel query to sequential query make indexing Win32 programs faster, should we remove the use of parallel indexing? (from about 2600 ms to 1600ms)

Should I push the change so that you can test it on your computer?

@jjw24
Copy link
Member Author

jjw24 commented Dec 20, 2020

Should I push the change so that you can test it on your computer?

yep push it up please and i will have a look

@taooceros
Copy link
Member

Should I push the change so that you can test it on your computer?

yep push it up please and i will have a look

Please take a look

@jjw24
Copy link
Member Author

jjw24 commented Feb 14, 2021

@taooceros I added match on path as well as this is one of the things the PR tries to resolve, eg. cmd should return Command Prompt result. Please have a look.

@jjw24
Copy link
Member Author

jjw24 commented Feb 14, 2021

one strange thing i noticed, is this duplicate despite only having one result returned

image

@taooceros
Copy link
Member

one strange thing i noticed, is this duplicate despite only having one result returned

image

Let me check that out because when duplicate results present, only the one with different descriptions should be kept. The one without description should be removed.

@taooceros
Copy link
Member

@taooceros I added match on path as well as this is one of the things the PR tries to resolve, eg. cmd should return Command Prompt result. Please have a look.

Haha this has been removed at about v1.4.0 I think? Let't only do that for lnk program to avoid duplicate match because exe program should have Name equal to their file name.

@jjw24
Copy link
Member Author

jjw24 commented Feb 15, 2021

@taooceros I added match on path as well as this is one of the things the PR tries to resolve, eg. cmd should return Command Prompt result. Please have a look.

Haha this has been removed at about v1.4.0 I think? Let't only do that for lnk program to avoid duplicate match because exe program should have Name equal to their file name.

ok make sense. I will update it

@jjw24
Copy link
Member Author

jjw24 commented Feb 15, 2021

one strange thing i noticed, is this duplicate despite only having one result returned
image

Let me check that out because when duplicate results present, only the one with different descriptions should be kept. The one without description should be removed.

@taooceros It's from Explorer, lol, of course.

OK Let me know if PR is good to go.

I also want to get the other UWP one in as well

@taooceros
Copy link
Member

one strange thing i noticed, is this duplicate despite only having one result returned
image

Let me check that out because when duplicate results present, only the one with different descriptions should be kept. The one without description should be removed.

@taooceros It's from Explorer, lol, of course.

OK Let me know if PR is good to go.

I also want to get the other UWP one in as well

Oh haha. Give me a few minutes to have a last check.

@taooceros
Copy link
Member

emmm are you able to debug the program? I think I have encountered some assembly issue because of the migration of ISavable.
image

@taooceros
Copy link
Member

ok found the issue. Everything plugin utilizes the old infrastructure, so that its ISavable is from there, which cause the type load issue.

@taooceros
Copy link
Member

@jjw24 I fix a few logic in HighlightData, please take a look. Also, I make the ExecutableName field be the file name we want for LnkProgram so that we don't need to get it every time.

@jjw24
Copy link
Member Author

jjw24 commented Feb 15, 2021

@taooceros LGTM, lets get this in

@jjw24 jjw24 merged commit 69c0fd8 into dev Feb 15, 2021
@jjw24 jjw24 deleted the fix_win32_loading branch February 15, 2021 08:58
@jjw24 jjw24 mentioned this pull request Feb 15, 2021
@jjw24 jjw24 removed the review in progress Indicates that a review is in progress for this PR label Feb 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Combine/remove duplicate entries

4 participants