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 AutoHotKey files, CSV, and other plaintext files #34824

Closed
wants to merge 2 commits into from

Conversation

daverayment
Copy link
Contributor

@daverayment daverayment commented Sep 12, 2024

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

It also introduces an extensible way to add further file extensions for plaintext previews in the future via a simple settings file.

PR Checklist

Detailed Description of the Pull Request / Additional comments

This uses Monaco to preview plaintext files even if they are not contained within the Monaco supported languages JSON file.

.ahk, .csv, and .tsv extensions have been added for now. Others may be added (possibly by end-users) by editing the new 'Assets\Monaco\monacoExtraExtensions.txt' file.

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

Care has been taken to not override any Shell preview handlers which could render these new file types. (This was necessary because ShellPreviewHandlerPreviewer is later than WebBrowserPreviewer in the IsItemSupported() checks in PreviewerFactory.cs)

It may be worthwhile enhancing this in the future. For example, by:

  • Allowing end users to edit the list of plaintext file extensions via Settings; or
  • Using file introspection to determine whether a file is plaintext, instead of rejecting it because its extension is unknown and then opening the generic unsupported file previewer.

Validation Steps Performed

  • I used the current version of PowerToys to attempt to open .ahk, .csv and .tsv files on my machine, which does not have Microsoft Office installed (so no preview handler for .csv files). I confirmed that the contents were not rendered and the UnsupportedFilePreview was used to show summary information only.
  • I confirmed each of the file types 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.

Please note: I don't own Microsoft Office / Office 365, so could not confirm that the Excel previewer handler for CSV files still works correctly with this update. Could this please be tested by one of the devs with that software installed? Many thanks.

Copy link

@check-spelling-bot Report

🔴 Please review

See the 📂 files view, the 📜action log, or 📝 job summary for details.

Unrecognized words (2)

ahk
tsv

Previously acknowledged words that are now absent applayout appsfolder cswinrt systemsettings SYSTEMWOW USEPOSITION USESIZE 🫥
To accept these unrecognized words as correct and remove the previously acknowledged and now absent words, you could run the following commands

... in a clone of the [email protected]:daverayment/PowerToys.git repository
on the keep-add-ahk branch (ℹ️ how do I use this?):

curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/v0.0.22/apply.pl' |
perl - 'https://github.com/microsoft/PowerToys/actions/runs/10821759987/attempts/1'
Available 📚 dictionaries could cover words (expected and unrecognized) not in the 📘 dictionary

This includes both expected items (1897) from .github/actions/spell-check/expect.txt and unrecognized words (2)

Dictionary Entries Covers Uniquely
cspell:r/src/r.txt 543 1 1
cspell:cpp/src/people.txt 23 1
cspell:cpp/src/ecosystem.txt 51 1

Consider adding them (in .github/workflows/spelling2.yml) for uses: check-spelling/[email protected] in its with:

      with:
        extra_dictionaries:
          cspell:r/src/r.txt
          cspell:cpp/src/people.txt
          cspell:cpp/src/ecosystem.txt

To stop checking additional dictionaries, add (in .github/workflows/spelling2.yml) for uses: check-spelling/[email protected] in its with:

check_extra_dictionaries: ''
Warnings (1)

See the 📂 files view, the 📜action log, or 📝 job summary for details.

ℹ️ Warnings Count
ℹ️ non-alpha-in-dictionary 1

See ℹ️ Event descriptions for more information.

If the flagged items are 🤯 false positives

If items relate to a ...

  • binary file (or some other file you wouldn't want to check at all).

    Please add a file path to the excludes.txt file matching the containing file.

    File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

    ^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

  • well-formed pattern.

    If you can write a pattern that would match it,
    try adding it to the patterns.txt file.

    Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

    Note that patterns can't match multiline strings.

@daverayment
Copy link
Contributor Author

@htcfreek Hi. Are you able to add a new file to the spelling checker excludes.txt file, please? The path to the file is \src\common\FilePreviewCommon\Assets\Monaco\monacoExtraExtensions.txt. Thank you!

@htcfreek
Copy link
Collaborator

@daverayment
Thank you for the contribution. But unfortunately we can't accept this PR.

  1. This is the wrong way of implementing this. Please read the docs under https://github.com/microsoft/PowerToys/blob/main/doc%2Fdevdocs%2Fcommon%2FFilePreviewCommon.md .
  2. You did not ask for implementation of the csv part. This is tricky as it may override the explorer default preview handling and may prevents Excel from handling it.

@daverayment
Copy link
Contributor Author

Thank you for reviewing the PR, @htcfreek 😊

Sorry, I don't think I explained the aims very well.

The intention is to use the Monaco renderer as a plaintext viewer for AutoHotKey scripts and other currently unsupported file extensions. The decision was deliberately made not to add these as new languages to Monaco, or as extensions to existing supported Monaco languages. This has the following advantages:

  • Plaintext files do not require syntax highlighting or any other special processing. They are for quick unformatted display only.
  • We can add new file extensions to the list much more easily than adding a new language to Monaco.
  • If a new language or syntax highlighting for a file type is added to Monaco's supported list, this will be automatically used, superseding the plaintext rendering.
  • Users themselves can add their own file extensions to the list without needing us to release a new version of PowerToys. (This is why I didn't hard-code the file extensions and created the new text file instead.)

Essentially, this adds a plaintext preview capability which just uses Monaco as the rendering engine, and I should have made that clearer. Apologies.

I understand that I didn't ask about the CSV or TSV additions. I wanted to provide an example that showed that a file extension could be handled either with a preview handler (for those viewing CSVs with Microsoft Office installed) or with Monaco (for those without Office using this plaintext renderer). There is code within the PR which specifically excludes those extensions which are supported by the ShellHandlerPreviewer, using its IsItemSupported() feature directly.

I still think this is a worthwhile approach to displaying files which do not need special processing. If this PR is unsuitable, I would like to raise it as another issue and perhaps explain it better and open it up to further discussion.

Thanks again.

@htcfreek htcfreek added the Status-Blocked We can't make progress due to a dependency or issue label Sep 12, 2024
@crutkas
Copy link
Member

crutkas commented Sep 12, 2024

Why is this blocked?

@htcfreek
Copy link
Collaborator

Why is this blocked?

  1. Only the changes for AHK are discussed.
  2. The way we did it till yet is updating the language json in Monaco. But this PR implements a different way to define supported extensions for plain text that never has neen discussed in the issue and allows manual user configuration in a text file.
  3. The now configuration file is not configurable through setting ui.

I think we should discuss first if we want this.

Copy link

@LandonMoss LandonMoss left a comment

Choose a reason for hiding this comment

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

This is some good stuff!

@pep-sanwer
Copy link

Would love to see this land!

@htcfreek
Copy link
Collaborator

@crutkas , @Aaron-Junker
Can you please look at this PR and discuss internally if you are okay with supporting additional text file extension managed in a settings file by the user. (And without ui in PT Settings.)

@htcfreek htcfreek added the Needs-Team-Response An issue author responded so the team needs to follow up label Oct 14, 2024
@crutkas crutkas added Needs-Review This Pull Request awaits the review of a maintainer. and removed Status-Blocked We can't make progress due to a dependency or issue Needs-Team-Response An issue author responded so the team needs to follow up labels Oct 15, 2024
@crutkas
Copy link
Member

crutkas commented Oct 15, 2024

this isn't directly a settings file but i do get the feeling. also reason why this isn't a settings file is this would be overwritten every release

@Aaron-Junker
Copy link
Collaborator

this isn't directly a settings file but i do get the feeling. also reason why this isn't a settings file is this would be overwritten every release

So imo, it should definitely be a settings file, so in the future we can also build an UI on top of it. But the bigger problem here is that this only adds support for using it in Peek, as for using it in File Explorer these file extensions would have to be registered additionally in the registry by the settings or the installer. This would require additional logic

@htcfreek
Copy link
Collaborator

htcfreek commented Oct 15, 2024

Imo this gets handled by PT settings and we provide a default value that the user can extend.

I think we should add the three extensions in the regular way as fallback and open a new issue for the custom extension setting. (At the end the current way of this PR implements something that was never requested and discussed in the issues.)

@crutkas
Copy link
Member

crutkas commented Oct 15, 2024

this isn't directly a settings file but i do get the feeling. also reason why this isn't a settings file is this would be overwritten every release

So imo, it should definitely be a settings file, so in the future we can also build an UI on top of it. But the bigger problem here is that this only adds support for using it in Peek, as for using it in File Explorer these file extensions would have to be registered additionally in the registry by the settings or the installer. This would require additional logic

We do reg stuff for stuff on and off for when we but this does get a little complicated i think. If we shifted to more of a mental model like what #32817 it would unblock the "now" and get these extensions supported in both, no?

I think for a good longer term

#32817
c458087#diff-f56a13d2e109105150aaf7bc8ed89dbdc676942801a86bf83c4c42d09f1bcc02L1

@htcfreek
Copy link
Collaborator

@crutkas
I am not 100% what yout want to say because your comment seems to miss some words.

But the PR you referenced/adding the extensions to languages.json is the preferred way we do since ever. Doing this is easy and simple and I think for now the best solution to get the preview part of this PR in. What do you think @Aaron-Junker ?

Later and discussed in a new issue we can implement a way for our users to add their own extension for txt files through PT settings.

@daverayment
Copy link
Contributor Author

Thank you everyone for discussing this.

The .txt file for user settings was not a good decision on my part. As @crutkas says, user entries would potentially be overwritten each install. Having an extra 'user-only' file where the user could list their own extensions could be a possibility, but discoverability would be a problem, it would be fragile, and it's all information I accept should really be exposed by an official PowerToys settings property.

I agree that the way forward is to create a new issue for user-configurable plain text file support. I can certainly do that. I'm afraid I'm unfamiliar with the settings system at the moment, but I'll ask if I need a hand.

Likewise, I can edit the monaco_languages.json and monacoSpecialLanguages.js files and hopefully that will cover the AutoHotKey requirement, which was the original issue. You know, before I went and complicated everything 🙄

Do you think it would be better if I closed this off and handled that via another PR?

Thanks again.

@htcfreek
Copy link
Collaborator

htcfreek commented Oct 15, 2024

@daverayment

  1. Implementing the user configurable plain text extension in a separate issue and pr sounds good. Please create the issue that we can discuss a good way of implementation. Later we can help with implementation.
  2. For the implementation of the original issues (.csv, .tsv, .ahk) as plain text using the currently approved way please open a new and clean PR. That easier to review.
  3. For the language.json part you find a documentation here: https://github.com/microsoft/PowerToys/blob/main/doc%2Fdevdocs%2Fcommon%2FFilePreviewCommon.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Review This Pull Request awaits the review of a maintainer.
Projects
Development

Successfully merging this pull request may close these issues.

6 participants