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.

Sign up for GitHub

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

Jump to bottom

Fix: Fixed an issue with drag and dropping items onto .ahk files #14895

Merged
merged 7 commits into from
Mar 7, 2024
Merged

Fix: Fixed an issue with drag and dropping items onto .ahk files #14895

merged 7 commits into from
Mar 7, 2024

Conversation

lunathanael
Copy link
Contributor

@lunathanael lunathanael commented Mar 4, 2024 •
edited
Loading

Allow .ahk and .py files to be treated as executable files to allow dragging and dropping other files onto them.

This was accomplished by essentially replacing current support for Python to also include AutoHotkey by combining the Script category.

Removed checks for Python/AHK Installed as AHK (and potentially other scripts) does not support version checking. This can be replaced by an alternative method for checking interpreter installation.

Note: AutoHotkey.exe is required to be added to the path, there is currently no check for this.

Resolved / Related Issues

  • Were these changes approved in an issue or discussion with the project maintainers? In order to prevent extra work, feature requests and changes to the codebase must be approved before the pull request will be reviewed. This prevents extra work for the contributors and maintainers.
    Closes Bug: Unable to drag and drop any file onto .ahk files #14430

Validation
How did you test these changes?

  • Did you build the app and test your changes?
  • Did you check for accessibility? You can use Accessibility Insights for this.
  • Did you remove any strings from the en-us resource file?
    • Did you search the solution to see if the string is still being used?
  • Did you implement any design changes to an existing feature?
    • Was this change approved?
  • Are there any other steps that were used to validate these changes?
    1. Open app ...
    2. Click settings button ...

Screenshots (optional)
image
image

@lunathanael
Added support for .ahk as script file, given that AutoHotKey was adde…
ea8240c 
…d to the path.

Removed checks for Python/AHK Installed as AHK does not support version checking.
0x5bfa
0x5bfa reviewed Mar 4, 2024
View reviewed changes
Copy link
Member

@0x5bfa 0x5bfa left a comment

Choose a reason for hiding this comment

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

Thanks for the first time contribution!

Note: AutoHotkey.exe is required to be added to the path, there is currently no check for this.

Does this mean that execution continues still the app doesn't check if python is installed? If so, is that ok?

@lunathanael
Copy link
Contributor Author

Thanks for the first time contribution!

Note: AutoHotkey.exe is required to be added to the path, there is currently no check for this.

Does this mean that execution continues still the app doesn't check if python is installed? If so, is that ok?

Yes, the execution will continue.
Correct me if I'm wrong, but when you open a file, it attempts to use the "default" application to do so, and when none is defined it'll prompt for one. For example, if I open a .ahk file without AutoHotkey installed and added to PATH, it'll prompt for a method to open the script, as if you pressed "open with...".

Thank you for clarifying this, just trying out something relatively easy as my first PR, definitely made some mistakes.

@0x5bfa 0x5bfa self-requested a review March 4, 2024 03:20
@0x5bfa 0x5bfa self-assigned this Mar 4, 2024
@0x5bfa
Copy link
Member

0x5bfa commented Mar 4, 2024

Thanks for the explanation!
You chose a right one:)

Don't worry, I will review on my local and approve.

@lunathanael
Copy link
Contributor Author

Thanks for the explanation! You chose a right one:)

Don't worry, I will review on my local and approve.

Thanks for the encouragement.
I just tested again and I believe that there is no need for AutoHotkey to be in the PATH.
My initial thought seem to be correct.
image

@0x5bfa
Copy link
Member

0x5bfa commented Mar 7, 2024 •
edited
Loading

And I wanna merge this too until the next release. @yaira2
I'll review tonight

yaira2
yaira2 reviewed Mar 7, 2024
View reviewed changes
src/Files.App/Utils/Storage/Operations/FilesystemHelpers.cs
{
var items = await GetDraggedStorageItems(packageView);
if (isTargetPythonFile && !SoftwareHelpers.IsPythonInstalled())
Copy link
Member

Choose a reason for hiding this comment

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

What happens if the target isn't installed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#14895 (comment)

Copy link
Contributor Author

@lunathanael lunathanael Mar 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Looking back, I think my comment was unclear. There is no requirement for AutoHotkey to be in the PATH. The program will still function as intended. This picture is if AutoHotkey is neither installed nor in the PATH.

Copy link
Member

Choose a reason for hiding this comment

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

I meant, what happens if Python isn't installed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my testing, it treats the file as if you would normally try to open a .py without python installed. If no windows default application for opening the file type is defined, it'll prompt for one like the picture suggests. I'm not sure if I understand or have answered your question correctly, I apologize.

Copy link
Member

Choose a reason for hiding this comment

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

That answers my question, thanks!

yaira2
yaira2 approved these changes Mar 7, 2024
View reviewed changes
Copy link
Member

@yaira2 yaira2 left a comment

Choose a reason for hiding this comment

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

LGTM

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Mar 7, 2024
@yaira2 yaira2 changed the title Fix: Fixed an issue with drag and dropping items onto .ahk files (#14430) Fix: Fixed an issue with drag and dropping items onto .ahk files Mar 7, 2024
@yaira2 yaira2 merged commit 3a8c7cf into files-community:main Mar 7, 2024
6 checks passed
@yaira2
Copy link
Member

yaira2 commented Mar 7, 2024

@lunathanael thank you for contributing to Files!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yaira2 yaira2 yaira2 approved these changes

@0x5bfa 0x5bfa Awaiting requested review from 0x5bfa

Assignees

@0x5bfa 0x5bfa

Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: Unable to drag and drop any file onto .ahk files
3 participants
@lunathanael @0x5bfa @yaira2