Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Insert active explorer path #1275

Merged

Conversation

stefnotch
Copy link
Contributor

@stefnotch stefnotch commented Jul 16, 2022

I refactored the code from #904

Please test this code, as I haven't really done it.


Description:

  • When Flow Launcher is invoked with an active Explorer.exe window selected Flow Launcher will open with that folder path selected.
  • Activation via builtin shortcut 'active_explorer_path'

Example when used with builtin shortcut:

image

string locationUrl = explorerWindow.LocationURL;
if (!string.IsNullOrEmpty(locationUrl))
{
return new Uri(locationUrl).LocalPath;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This ended up being the most reliable way of getting the explorer path. I think edge cases are folders like the desktop, recycle bin, documents folder, etc.

{
// we have found the desired window, now let's make sure that it is indeed a file explorer
// we don't want the Internet Explorer or the classic control panel
if (explorerWindow.Document is not Shell32.IShellFolderViewDual2)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check could be removed, since the check below catches everything.

(The classical control panel is a pretty nasty edge case, try it out)

@@ -749,6 +751,11 @@ public void Show()
((MainWindow)Application.Current.MainWindow).WindowAnimator();

MainWindowOpacity = 1;
if (_explorerPath != null)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check has been simplified, so that's nice

@@ -734,6 +733,9 @@ public void ToggleFlowLauncher()

public void Show()
{
string _explorerPath = FileExplorerHelper.GetActiveExplorerPath();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this might be a useful function to expose to all plugins.

Copy link
Member

Choose a reason for hiding this comment

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

It seems like this is a utility function? Maybe we can put the helper in Flow.Launcher.Plugin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@taooceros Yes, that sounds useful


namespace Flow.Launcher.Helper
{
public class FileExplorerHelper
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other function that might be interesting would be getting all open file explorer paths (sorted by depth/zindex). (See also jjw24/Wox.Plugin.Runner#27 )
Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I agree, maybe we should return the paths sorted by depth/zindex (since we will need to get all of them). The overhead can be ignored.

Comment on lines 80 to 97
<COMReference Include="SHDocVw">
<VersionMinor>1</VersionMinor>
<VersionMajor>1</VersionMajor>
<Guid>eab22ac0-30c1-11cf-a7eb-0000c05bae0b</Guid>
<Lcid>0</Lcid>
<WrapperTool>tlbimp</WrapperTool>
<Isolated>false</Isolated>
<EmbedInteropTypes>true</EmbedInteropTypes>
</COMReference>
<COMReference Include="Shell32">
<VersionMinor>0</VersionMinor>
<VersionMajor>1</VersionMajor>
<Guid>50a7e9b0-70ef-11d1-b75a-00a0c90564fe</Guid>
<Lcid>0</Lcid>
<WrapperTool>tlbimp</WrapperTool>
<Isolated>false</Isolated>
<EmbedInteropTypes>true</EmbedInteropTypes>
</COMReference>
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about the COMREference? Is it better to use dynamic typing as what you did in the runner plugin? Somehow the dotnet cli doesn't support this feature, and the previous workaround is to copy the reference dll and directly reference that dll.

Copy link
Contributor Author

@stefnotch stefnotch Jul 16, 2022

Choose a reason for hiding this comment

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

I honestly have no clue.

Copy link
Member

Choose a reason for hiding this comment

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

I would say since you have shared a way to prevent usage of comreference, let's use that. Previously I workaround this by copying out the dll and reference that directly.

Copy link
Member

Choose a reason for hiding this comment

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

Though I personally think it may be better to have strongly type?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@stefnotch stefnotch Jul 17, 2022

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

yeah for sure, but I think that's pretty messy. We do have some example about that.

@Garulf
Copy link
Member

Garulf commented Jul 30, 2022

Are we able to fix the CI build issue?

@taooceros
Copy link
Member

Are we able to fix the CI build issue?

It's because of merge conflict. Just resolve it.

@Garulf
Copy link
Member

Garulf commented Jul 31, 2022

Are we able to fix the CI build issue?

It's because of merge conflict. Just resolve it.

Ah! Didn't see that. That's what I get for glancing.

@taooceros
Copy link
Member

@stefnotch Do you have time to remove the comreference and move everything to either dynamic or a template class? Then we shall be able to move to the final stage.

@stefnotch
Copy link
Contributor Author

@taooceros Okay, I've changed it to use dynamic. However, I didn't test the code at all, since I don't have the .NET 5 framework on this machine. (Only got .NET 6)

@taooceros
Copy link
Member

@taooceros Okay, I've changed it to use dynamic. However, I didn't test the code at all, since I don't have the .NET 5 framework on this machine. (Only got .NET 6)

I think flow has been upgraded to .Net 6. The original pr is pretty old, so do you mind taking sometimes to merge the dev branch? Thanks

@stefnotch
Copy link
Contributor Author

@taooceros Lovely, that's super nice to hear. Okay, I merged it in and tested it out. Seems to work rather nicely, the only thing I'd tweak is: The entire text should be selected when it automatically inserts the file explorer path. (aka it should be easy to get rid of the inserted file explorer path)

@taooceros
Copy link
Member

@taooceros Lovely, that's super nice to hear. Okay, I merged it in and tested it out. Seems to work rather nicely, the only thing I'd tweak is:

It does take almost a whole year for .Net 6 branch to be merged🤣. That's pretty nice. Thanks

The entire text should be selected when it automatically inserts the file explorer path. (aka it should be easy to get rid of the inserted file explorer path)

I am not sure whether it is easy to be done. I will reach back soon about that once I finish the current stuff.

@taooceros
Copy link
Member

One thing I am a bit worried by this feature is that it only applies to explorer plugin (although it is a built in plugin). Therefore, do you think it would be better if we put the code into plugin project instead of the main project?

Regarding text selection, there will be much more that needs to be done. Maybe we should expose an api for plugins to select a part of the query base on index.
Another idea is to use a special actionkeyword to query only in the specific path. Thereby we don't actually need to handle the long preceding path (and also don't need to select it). However, that will lose us the possibility to traverse around it (but search around it may be enough?). If this way makes more sense, maybe we need a way to quickly switch to normal path search under the same path.

I think we can put the helper class in Flow.Launcher.Plugin project, since it maybe quite useful but somewhat non-trivial to implement.

@jjw24
Copy link
Member

jjw24 commented Aug 9, 2022

Therefore, do you think it would be better if we put the code into plugin project instead of the main project?

Thought the idea was to expose this functionality via the API? If not then yes move it to plugin project so all plugins can take advantage of.

EDIT: functionality as in grabbing the active window, is this what you are asking?

@taooceros
Copy link
Member

Therefore, do you think it would be better if we put the code into plugin project instead of the main project?

Thought the idea was to expose this functionality via the API? If not then yes move it to plugin project so all plugins can take advantage of.

EDIT: functionality as in grabbing the active window, is this what you are asking?

I don't think we need to expose as api. It's nothing related to flow.

@jjw24
Copy link
Member

jjw24 commented Aug 10, 2022

But isn't this functionality potentially can be used by several plugins e.g. Explorer & Runner plugins? Even though placing in the plugin project can achieve the same, but wouldn't putting it in API will mean it will be documented and potentially allow it to be used (assuming) by other non-C# language plugins?

@taooceros
Copy link
Member

But isn't this functionality potentially can be used by several plugins e.g. Explorer & Runner plugins? Even though placing in the plugin project can achieve the same, but wouldn't putting it in API will mean it will be documented and potentially allow it to be used (assuming) by other non-C# language plugins?

hmmm make sense? Though I think there's a really long path to allow non-c# plugin to use feature like this. I just feel like the plugin api may be better to include api that is configurable in flow or depends on flow. This looks like simply a utility function.

@jjw24
Copy link
Member

jjw24 commented Aug 11, 2022

Yeah, at this stage, up to you guys, ultimately placing it in the plugin project is a good start.

@taooceros
Copy link
Member

Yeah let's put it into the plugin project first. We can add it to the api later.

@stefnotch stefnotch marked this pull request as ready for review August 17, 2022 13:56
@taooceros
Copy link
Member

I have a concern at the current stage. We actually allow customize the action keyword for exploring a particular directory. That means it is possible that the inserted directory won't do anything (which is partially why I think it would be better to move the code to explorer plugin, where it has control over its own action keyword).

@taooceros
Copy link
Member

@stefnotch maybe let's delay moving to Flow.Launcher.Plugin after we finish #1097. It contains too much change to explorer plugin that I didn't want to handle the merge conflict.

@stefnotch
Copy link
Contributor Author

@taooceros Sounds fine, we can always do more in future pull requests.

@taooceros
Copy link
Member

taooceros commented Dec 30, 2022

@stefnotch Let's make this a built-in command😉(Sorry for such a long delay).

#1474

@stefnotch
Copy link
Contributor Author

@taooceros That sounds like a great idea! Would you like to take care of it, since it might take me a while to get around to doing so?

@taooceros
Copy link
Member

@taooceros That sounds like a great idea! Would you like to take care of it, since it might take me a while to get around to doing so?

yeah sure!

…indow-tweaks

# Conflicts:
#	Flow.Launcher/ViewModel/MainViewModel.cs
@taooceros
Copy link
Member

Hmmm I guess we need to adjust the logic a bit, since the shortcut will be activated after flow showing up, so the foreground window will become flow. What do you think about getting the explorer window with the highest z-index? @stefnotch

@taooceros
Copy link
Member

@stefnotch mind give it a test?

Assign something like this or use the custom shortcut image

@stefnotch
Copy link
Contributor Author

This is pretty cool and I think I quite like how it works
image

@taooceros
Copy link
Member

Great! I will merge it!

@taooceros taooceros enabled auto-merge January 8, 2023 03:32
@taooceros taooceros merged commit c0f46f2 into Flow-Launcher:dev Jan 8, 2023
@jjw24 jjw24 added the enhancement New feature or request label Jan 9, 2023
@jjw24 jjw24 added this to the 1.12.0 milestone Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants