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

[Peek] Add support for previewing .ahk files as plaintext #35538

Merged
merged 4 commits into from
Nov 22, 2024

Conversation

daverayment
Copy link
Contributor

@daverayment daverayment commented Oct 22, 2024

Summary of the Pull Request

This replaces the previous #34824 PR. That PR's feature to allow the previewing of arbitrary plaintext files is not available here and is covered by a separate issue.

This change allows for the previewing of AutoHotKey .ahk script files with the Monaco renderer in Peek (via the WebBrowserPreviewer).

PR Checklist

Detailed Description of the Pull Request / Additional comments

The .ahk file extension has been added to the relevant Monaco settings files, in a similar manner to the addition of .srt files previously.

A small fix was made to MonacoHelper.cs by placing a using for the JsonDocument instantiation there, as it was not Disposed previously.

Validation Steps Performed

  • I used the current version of PowerToys to attempt to open .ahk files on my machine. I confirmed that the contents were not rendered and the UnsupportedFilePreview was used to show summary information only.
  • I confirmed the .ahk files could be opened as plaintext with the updated build.
  • I ran through my test folder, which contains a mix of image files, documents, text files and so on, confirming there were no issues created for other file types.

@daverayment
Copy link
Contributor Author

I am currently looking into a bug with the exclusion code. Please hold off from reviewing for now. Thanks.

@daverayment
Copy link
Contributor Author

@htcfreek @crutkas

Could I request some help here, please?

When you add a file extension for previewing with Monaco by editing 'monacoSpecialLanguages.js' and 'monaco_languages.json', PowerToys will create a shellex entry in the Registry for that extension on startup:

image

(With {D8034CFA-F34B-41FE-AD45-62FCBB52A6DA} here referring to the Monaco Preview Handler.)

This is fine for file extensions which don't already have preview handlers defined. However, it looks like PowerToys will overwrite any existing entry present for that extension, too. I've been trying to study the code which registers the Monaco types in the registry (in 'modulesRegistry.h' and 'registry.h'), and I've done some testing to confirm the behaviour, but my C++ isn't very strong and I may be mistaken. In any case, I could use some advice.

If this behaviour is correct, we can't add .csv to Monaco's supported list, as it would overwrite the existing Office-installed Excel previewer handler if someone had that installed. Also, is it intended that PowerToys overwrites any predefined handlers for the other file extensions, too? Shouldn't there be code in getMonacoPreviewHandlerChangeSet to add extensions with existing handlers to the ExtExclusions list?

I'm sorry if this is intended behaviour or I'm misunderstanding, but it seemed suspicious and I wanted to be sure.

@htcfreek htcfreek added the Needs-Team-Response An issue author responded so the team needs to follow up label Oct 23, 2024
@daverayment
Copy link
Contributor Author

This issue seems to confirm that existing preview handler entries are being overwritten by PowerToys. Handlers installed by Office would be common examples, but there could be others.

@daverayment
Copy link
Contributor Author

As discussed previously, I have removed .csv and .tsv support, and this PR now only adds .ahk support for Peek.

@htcfreek
Copy link
Collaborator

@daverayment
Can you update the linked issues, pr title and description please.

@htcfreek htcfreek added Needs-Review This Pull Request awaits the review of a maintainer. and removed Needs-Team-Response An issue author responded so the team needs to follow up labels Nov 13, 2024
@daverayment daverayment changed the title [Peek] Add support for .ahk, .csv and .tsv previewing as plaintext files [Peek] Add support for previewing .ahk files as plaintext Nov 13, 2024
@daverayment
Copy link
Contributor Author

@htcfreek Thanks for the reminder. Done.

Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the contribution!

@jaimecbernardo jaimecbernardo added Good to Merge and removed Needs-Review This Pull Request awaits the review of a maintainer. labels Nov 22, 2024
@jaimecbernardo jaimecbernardo merged commit 6cece12 into microsoft:main Nov 22, 2024
11 checks passed
@daverayment daverayment deleted the peek-ahk-add branch November 22, 2024 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

3 participants