Skip to content

Conversation

@VictoriousRaptor
Copy link
Contributor

@VictoriousRaptor VictoriousRaptor commented Jan 19, 2023

Changes:

Tests:

  • A single ":" when excluded path is not empty won't cause exception. (1802)
  • Unit test for environment path expansion
  • Unit test for subpath check
  • Unit test for result comparator

@VictoriousRaptor VictoriousRaptor added the bug Something isn't working label Jan 19, 2023
@VictoriousRaptor VictoriousRaptor self-assigned this Jan 19, 2023
@github-actions

This comment has been minimized.

@jjw24
Copy link
Member

jjw24 commented Jan 19, 2023

Please update description with what you have tested :)

@jjw24 jjw24 added this to the 1.11.1 milestone Jan 19, 2023
Comment on lines 120 to 124
excludedPath => r.SubTitle.StartsWith(excludedPath.Path, StringComparison.OrdinalIgnoreCase)));
excludedPath => IsSubPathOf(r.SubTitle, excludedPath.Path)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the issue with this check?

Copy link
Contributor Author

@VictoriousRaptor VictoriousRaptor Jan 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a path string starts with another doesn't necessarily mean it's contains the other one (unless ending with slash is guaranteed). like c:\program files and c:\program

@jjw24
Copy link
Member

jjw24 commented Jan 19, 2023

@z1nc0r3 just a FYI the issue with ':' is addressed here already.

@VictoriousRaptor
Copy link
Contributor Author

@z1nc0r3 just a FYI the issue with ':' is addressed here already.

that's a bit different. that pr was to ignore queries with ':'. this one is to fix the crash issue.

@VictoriousRaptor VictoriousRaptor added the review in progress Indicates that a review is in progress for this PR label Jan 19, 2023
@VictoriousRaptor
Copy link
Contributor Author

VictoriousRaptor commented Jan 20, 2023

The only other thing I picked up is the use of %USERNAME% in the form of c:\users\%USERNAME%\downloads. https://www.elevenforum.com/t/complete-list-of-environment-variables-in-windows-11.11212/ I suppose though you can use %USERPROFILE% to achieve the same as using %USERNAME%

Perhaps add an exclusion for %USERNAME%?

Just found something interesting. If you create a folder called %appdata% in some folder like D:\foo, then try to cd D:\foo\%appdata% in cmd or type it in the location bar of windows explorer, it doesn't work. But you can still enter it by double clicking. Guess we need to reconsider how to deal with it.

Edit: Only expand when matching ...\%blabla%\....

@taooceros
Copy link
Member

The only other thing I picked up is the use of %USERNAME% in the form of c:\users\%USERNAME%\downloads. https://www.elevenforum.com/t/complete-list-of-environment-variables-in-windows-11.11212/ I suppose though you can use %USERPROFILE% to achieve the same as using %USERNAME%
Perhaps add an exclusion for %USERNAME%?

Just found something interesting. If you create a folder called %appdata% in some folder like D:\foo, then try to cd D:\foo\%appdata% in cmd or type it in the location bar of windows explorer, it doesn't work. But you can still enter it by double clicking. Guess we need to reconsider how to deal with it.

Edit: Only expand when matching ...\%blabla%\....

What about check whether the path exists before expanding environmental variable?

@VictoriousRaptor
Copy link
Contributor Author

What about check whether the path exists before expanding environmental variable?

Expanding is before checking the path is incomplete (user inputting and without trailing '\'). For this bug this pr is enough imo. Even Windows Explorer can't deal with this situation perfectly.

@taooceros
Copy link
Member

Expanding is before checking the path is incomplete (user inputting and without trailing ''). For this bug this pr is enough imo. Even Windows Explorer can't deal with this situation perfectly.

lol ok

@VictoriousRaptor
Copy link
Contributor Author

Expanding is before checking the path is incomplete (user inputting and without trailing ''). For this bug this pr is enough imo. Even Windows Explorer can't deal with this situation perfectly.

lol ok

It can be done but I don't really want to do it. In this pr @kubalav 's path should be correctly parsed. but paths like d:%appdata% still can't be handled correctly.

/// </summary>
/// <param name="path"></param>
/// <returns></returns>
public static string EnsureTrailingSlash(this string path)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, this will be helpful because through out explorer there are dup code to add trailing slash.

@jjw24 jjw24 merged commit db2b856 into Flow-Launcher:dev Jan 22, 2023
@jjw24 jjw24 removed the review in progress Indicates that a review is in progress for this PR label Jan 22, 2023
@jjw24 jjw24 modified the milestones: 1.11.1, 1.12.0 Jan 22, 2023
@VictoriousRaptor VictoriousRaptor deleted the FixExplorer branch January 23, 2023 03:11
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

Archived in project

Development

Successfully merging this pull request may close these issues.

[BUG] - Explorer Plugin crash when first input is a colon ":" [BUG] Variable %LocalAppData% in path cause exception

3 participants