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

Add MarkdownCodeExporter #9005

Merged
merged 2 commits into from
Dec 1, 2024
Merged

Conversation

maliayas
Copy link
Contributor

  • I'm the package's author and/or maintainer.
  • I have have read the docs.
  • I have tagged a release with a semver version number.
  • My package repo has a description and a README describing what it's for and how to use it.
  • My package doesn't add context menu entries. *
  • My package doesn't add key bindings. **
  • Any commands are available via the command palette.
  • Preferences and keybindings (if any) are listed in the menu and the command palette, and open in split view.
  • If my package is a syntax it doesn't also add a color scheme. ***
  • If my package is a syntax it is named after the language it supports (without suffixes like "syntax" or "highlighting").
  • I use .gitattributes to exclude files from the package: images, test files, sublime-project/workspace.

My package is MarkdownCodeExporter

There are no packages like it in Package Control.

Copy link
Collaborator

@packagecontrol-bot packagecontrol-bot left a comment

Choose a reason for hiding this comment

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

Automated testing result: SUCCESS

Repo link: MarkdownCodeExporter

Packages added:
  - MarkdownCodeExporter

Processing package "MarkdownCodeExporter"
  - All checks passed

@braver
Copy link
Collaborator

braver commented Nov 30, 2024

Thanks for your submission. Some remarks, thoughts:

  • You're using an event listener so we need to worry about performance. Your on_modified handler for instance is synchronous, that looks like a risk.
  • Finding out if the current file is markdown via the view's syntax is tricky (as you seemingly noticed) due to all the different syntax packages for markdown. It might be easier to check the file's scope, it should be text.html.markdown regardless of the package used.
  • Currently the only way of interacting with your package's features is via clicks on the buttons you add to the view. It would be nice if one could do similar things via the keyboard. For instance, when the cursor is in a code block, trigger commands via the command palette.
  • If the code block is rendered using a specific embedded syntax (as it should), you should be able to get the actual syntax used for that block so that you don't have to use that lookup mapping.

@braver braver added feedback provided mergeable The PR is in a mergeable state but awaiting some final comments/acknowledgement from either side labels Nov 30, 2024
@maliayas
Copy link
Contributor Author

maliayas commented Dec 1, 2024

Thanks for the valuable review!

You're using an event listener so we need to worry about performance. Your on_modified handler for instance is synchronous, that looks like a risk.

I gave it a try and used async version. But this time long texts caused a lag. I'm not an expert on async stuff. Would you like to test it on your side, such that replace "on_modified" with "on_modified_async" and create a md document which have 20 times of this code block:

```
lorem ipsum
```

Then try to insert any char to anywhere in the document. On my ST version (3125) unacceptable lags occur with the async version. Non-async function works with almost zero lag. I can only notice a small lag when the document has more than 500 code blocks.

@maliayas
Copy link
Contributor Author

maliayas commented Dec 1, 2024

Finding out if the current file is markdown via the view's syntax is tricky (as you seemingly noticed) due to all the different syntax packages for markdown. It might be easier to check the file's scope, it should be text.html.markdown regardless of the package used.

Good idea but there is a limitation with the ST API. In the is_applicable() method I don't have access to the view object, so I can't do a if sublime.score_selector(self.view.scope_name(0), "text.html.markdown") == 0 check there. I only have settings object at that point, which gives me only the active syntax file's path.

Instead I can move that check to on_modified() like below:

--- main.py
+++ main.py
@@ -5,16 +5,16 @@
 from html import escape
 
 class MarkdownCodeExporter(sublime_plugin.ViewEventListener):
-    @classmethod
-    def is_applicable(cls, settings):
-        return settings.get('syntax', '').lower().find('markdown') != -1
-
     def __init__(self, view):
         super().__init__(view)
         self.phantom_set = sublime.PhantomSet(view)
         self.update_phantoms()
 
     def on_modified(self):
+        # Limit the event listener to Markdown.
+        if sublime.score_selector(self.view.scope_name(0), "text.html.markdown") == 0:
+            return
+
         self.update_phantoms()
 
     def on_load(self):

But this time I loose the performance benefit of is_applicable() which is only run upon settings change (e.g. syntax changed) and then cached. On the other hand if I do the check in on_modified() every non-markdown file gets triggered on every modification which is a performance penalty even if the trigger does little job.

I think we can safely assume that almost any Markdown syntax file has a "markdown" phrase in its file name. Thus keep the existing approach.

What is your opinion?

@braver
Copy link
Collaborator

braver commented Dec 1, 2024

Good idea but there is a limitation with the ST API

Oh, you're right of course. Well, at least we considered it :) Your existing approach is certainly just as valid.

On my ST version (3125) unacceptable lags occur with the async version

Hm, indeed. Since you seem to re-render all phantoms on every update, when doing it async will make the entire document jump around. I wonder if you could leave any unchanged phantoms alone.... It's been a while since I went deep into phantoms, but I don't think I ever had that issue with ColorHints. I do notice your PhantomSet doesn't have a key, that might help?

By the way, something else. I noticed the border radius on the buttons is not equal. It looks like the tops are getting clipped.

Scherm­afbeelding 2024-12-01 om 14 57 26

Copy link
Collaborator

@packagecontrol-bot packagecontrol-bot left a comment

Choose a reason for hiding this comment

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

Automated testing result: SUCCESS

Repo link: MarkdownCodeExporter

Packages added:
  - MarkdownCodeExporter

Processing package "MarkdownCodeExporter"
  - All checks passed

@maliayas
Copy link
Contributor Author

maliayas commented Dec 1, 2024

Hm, indeed. Since you seem to re-render all phantoms on every update, when doing it async will make the entire document jump around. I wonder if you could leave any unchanged phantoms alone.... It's been a while since I went deep into phantoms, but I don't think I ever had that issue with ColorHints. I do notice your PhantomSet doesn't have a key, that might help?

Created an issue: SublimeText-Markdown/MarkdownCodeExporter#1

By the way, something else. I noticed the border radius on the buttons is not equal. It looks like the tops are getting clipped.

Created an issue: SublimeText-Markdown/MarkdownCodeExporter#2

Currently the only way of interacting with your package's features is via clicks on the buttons you add to the view. It would be nice if one could do similar things via the keyboard. For instance, when the cursor is in a code block, trigger commands via the command palette.

Created an issue: SublimeText-Markdown/MarkdownCodeExporter#3

If the code block is rendered using a specific embedded syntax (as it should), you should be able to get the actual syntax used for that block so that you don't have to use that lookup mapping.

Created an issue: SublimeText-Markdown/MarkdownCodeExporter#4


Once I have time, I'll look into these feedbacks. Thanks for them. In its current state, I believe the plugin is ready to be published.

P.S. Please be aware that today I transferred my repo to another organizaton and pushed a commit to this PR to reflect that.

@braver
Copy link
Collaborator

braver commented Dec 1, 2024

Excellent, have fun developing your package!

@braver braver merged commit d71bef3 into wbond:master Dec 1, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback provided mergeable The PR is in a mergeable state but awaiting some final comments/acknowledgement from either side
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants