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

Sketch 98 compatibility #38

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rodionovd
Copy link

@rodionovd rodionovd commented Oct 9, 2023

This PR contains a bunch of minor changes that should allow the plugin to function properly on Sketch 98 (and 99 Beta). Resolves #35, #36 and #37.

Feel free to download the fixed version of the plugin from my fork and let me know if there are any problems with it on the latest Sketch versions.

It returns the same value for symbol masters and artboard-less symbol instances, as they are both page-level and thus only have absolute (aka page) coordinates to them
@jazzybeat
Copy link

Hi Dmitry,
Thank you again for your effort in fixing this plugin. I tested it on a large document, and it works but is a bit slow, even on the M2 Mac chip. It takes about 1 minute to process the file each time I want to search for a symbol. I don't know the search logic, but it seems it's scanning all the instances every time. There may be a way to optimize it, or cache scanned instances. Please find attached file if you want to test performance.
Test - Performance 2K.zip

@rodionovd
Copy link
Author

rodionovd commented Oct 12, 2023

Thanks for checking it out @jazzybeat!

I tested it on a large document, and it works but is a bit slow, even on the M2 Mac chip. It takes about 1 minute to process the file each time I want to search for a symbol

Well, yes, as already noted by Jason himself, performance of the plugin is underwhelming on large documents.

I've conducted a quick investigation and found the following areas which affect the performance the most:

  1. The plugin does indeed enumerates all symbol instances and all override points of each symbol instance.

    Now, it's not that enumerating a large amount of layers is a problem for Sketch (they actually maintain a decent cache of things internally for this exact purpose). It's that the plugin uses the JavaScript Sketch API for this and it's utterly ineffective: it completely ignores an internal Sketch cache when going thru the whole document content (e.g. there's a ready-made list of all instances for a particular master symbol) and it has to wrap all the symbols/overrides into a JavaScript objects which also adds a bit of overhead to the process.

  2. The plugin window greedily renders previews for all of the symbol instances and overrides even tho just a tiny portion of them are visible in the list at a time.

    So if there're like 10000 instances in the list, the plugin will first generate previews for all of them and only then will display a window with the list. As a result, it may take minutes for the list to appear and scrolling is unlikely to be smooth in there. This whole step takes 2-3x more time than searching for symbol instances on the first step, so it contributes a lot to the overall slowness of the plugin.


The first point could be improved by going back to the private/unsafe/unofficial Sketch API and using it to gather a list of symbol instances and their overrides – it will literally take a second instead of ten.

The second one is much more tricker tho: fixing it would require a complete rewrite of the plugin (possibly in a different language, like Swift) to take advantage of native macOS UI optimizations. So if we ever decide to go this route, I guess it'd be better to start off a new plugin from scratch – but I'm not sure if it's worth the trouble after all...

@rodionovd rodionovd marked this pull request as ready for review October 12, 2023 13:18
@rodionovd rodionovd changed the title [WIP] Sketch 98 compatibility Sketch 98 compatibility Oct 12, 2023
@jazzybeat
Copy link

Thanks for the detailed explanation. I think your suggested first point would be a good enough solution. I recommend removing the Preview screen and showing just a text list to improve the performance.

Here, you can see an improved design for a list that is based on a table view.
Symbol Instance Locator - v2 Concept

@jazzybeat
Copy link

Artboard

@rodionovd
Copy link
Author

That's a great suggestion, Paulius!

Let me think about it for some time, because I'm not sure if it's even possible to turn the existing table into a multi-column one without throwing most of the existing plugin code away (as there're certain limitations about using native macOS UI from JavaScript).

@jazzybeat
Copy link

Here are a few more alternative options.
Alternative Options

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not working on Sketch v96
2 participants