adobe-photoshop / generator-assets

Generator Plug-in for Adobe Photoshop CC that helps users export image assets
MIT License
496 stars 86 forks source link

layer type for smart object layers is not correct #305

Open joelrbrandt opened 10 years ago

joelrbrandt commented 10 years ago

In both doc info and in change events, the layer "type" for smart object layers (both embedded and linked) is reported as "layer".

There's evidence in our code that at one point, the reported type was "smartObjectLayer": https://github.com/adobe-photoshop/generator-assets/blob/b76afe383d61d7c78d6764ddd9df9b90031bc61b/lib/dom/layer.js#L1234

Regardless of what may have changed, we are now no longer constructing the right DOM object types for smart object layers. A workaround on the JS side would be to look for the smartObject property of a layer as a way to set the type. (This is what the unmerged PR #298 did.)

joelrbrandt commented 10 years ago

@timothynoel is this a regression or an intended change in functionality on the native side?

timothynoel commented 10 years ago

I'm having trouble seeing where that was changed/broken in the version history, and I'm not even seeing that string anywhere. I do see that I documented it that way on the wiki, so I wonder if it's more a documentation glitch. We're using the type used by the scripting engine, except for shape layers.

joelrbrandt commented 10 years ago

@timothynoel well, it must have worked at one point because @iwehrman wrote a WHOLE BUNCH of code that was dependent on that string existing for it to ever execute. Or @iwehrman wrote a whole bunch of code that has never been executed.

Anyway, the important question is this: should we fix this on the native side (and report a "smartObjectLayer" type) or should we work around this on the JS side (looking for a smartObject property in the layer). Either is cool with me.

iwehrman commented 10 years ago

I think it just never executed. It's like 20 lines of code. I started going down a false path based on what I found on the wiki.

timothynoel commented 10 years ago

15.1 is at least reporting them as "layer". I don't have an earlier version to check. I guess the only slight (?) worry is that if does turn out to be a new format addition, it could change some behavior in some slight way (if for some reason code doesn't treat unrecognized types the same as "layer", like an error).

timothynoel commented 10 years ago

It was a documentation fail by me. I fixed the format document to remove "smartObjectLayer" back in march. https://github.com/adobe-photoshop/generator-core/wiki/JSON-event-format/_history

joelrbrandt commented 10 years ago

All right. We'll fix it on the JS side by either a.) looking at the smartObjects property or b.) removing any mention of smart objects from our DOM.

@iwehrman which should we do?

mcilroyc commented 8 years ago

Assigning to myself to check this out.

mcilroyc commented 8 years ago

To my eye, it looks like we are already looking at the smartObjects property and handling appropriately. One question: In the unused code path (if layer.type were to equal smartObjectLayer) it seems to include _setTimeContent see here. @iwehrman is this ringing any bells?

Demoting this to "code cleanup" to remove the unnecessary code.

iwehrman commented 8 years ago

I don't have a clear recollection, but I remember that there were some parts of the DOM that I started building but which didn't have much chance to test or use. image