Style lenses

[cchalmers]

Not ready to merge: diagrams-lib needs to make a few changes before merging.

This adds lenses for Styles onto attributes, and prisms for Prim and RNode. I've also removed some unused functions.

The lenses provide valid lenses onto the attributes of a style. Even though you can't traverse over the styles of a diagram yet they're useful for things like ArrowOpts (I also deal with them a lot in plots) where you deal with styles explicitly. The lookup is determined by the type of the target. It's possible to specialise these lenses for particular attributes:

_LineJoinA :: Lens' (Style v n) (Maybe LineJoinA)
_LineJoinA = atAttr

Another useful trick is using the Wrapped instance of attributes to have a lens to the direct value:

sty & _LineJoin . mapping _Wrapped' ?~ LineCapButt

If an attribute has a default and we can check equality for this default, we can go further by using a non or anon Iso:

_LineJoin :: Lens' (Style v n) LineJoin
_LineJoin = _LineJoinA . mapping _Wrapped . non def

This is because having no entry in the style is the same as using the default:

> (mempty :: Style V2 Double) ^. _LineJoin
LineJoinMiter

I haven't implemented this in diagrams-lib yet. Is it something we'd want for all attributes? I'm not sure about naming conventions.

[bergey]

The new Semigroup instance for Attributes seems potentially confusing. Is it needed to implement the lenses? Semigroup on Style? Could you explain why we need the instance, maybe as a comment in the code?

How much existing user code will this break? I expect not much - the usual code using fc, lw & so forth is unaffected.

Otherwise, this looks good, and I agree that it's helpful for arrows & plots, where we manipulate styles directly.

@haasn Do you have time to take a look at this? You were working on something similar, right? Except depending on fixpoint changes that haven't landed?

[cchalmers]

Thanks for taking a look.

The Semigroup instance on Style is essentially the same, it just uses view patterns to clean up the case boilerplate. I can revert it back if you want. The Semigroup on Style is unchanged, it's just been moved. I'll still add little explanation for it.

I'd be surprised if any user code is broken. The only ones I could imagine users using is setAttr and attrToStyle and combineAttr but I removed them because they're kinda confusing and easy to misuse (there was a lib function that used setAttr on a Texture which is wrong because it wasn't encoded as a TAttribute). It's safer to do these things with functions in lib.

[bergey]

OK, sounds good. No need to revert anything, I was just trying to understand.

[haasn]

This looks similar to parts of what I had implemented, modulo some API changes. The way I approached the API was by providing this function instead: https://github.com/diagrams/diagrams-core/blob/gsoc/src/Diagrams/Core/Style.hs#L221 instead of using the internal mkAttrLens + explicitly providing named instantiations of it.

I think I prefer my design for that, but maybe you could find a way to merge them.

[cchalmers]

Ah, didn't realise there was other work on it. They turned out pretty similar. I didn't see any need expose mkAttrLens because they're only going to be used with one of the three attribute prisms. Unless I'm missing something it's

attr _TAttribute . _LineTexture

vs

atTAttr . _LineTexture

so there's not much difference but I prefer mine :)

Are you going to continue with your gsoc? I've got a whole bunch of lens related stuff for diagrams if you're interested.

[haasn]

I have no immediate plans for continuing it, since development pretty much stalled due to blocking on fixpoint developments. The scope of my project was larger than just adding lenses, which means that those never ended up trickling upstream. If you want to merge our lens approaches and get them into master, I'd greatly appreciate it.

For the “attr” vs “atTAttr” distinction, it's been so long that I forgot most of the original motivation for my design choices, but it's possible that I may have picked them so you can use attr with more complicated prisms, too (eg. _Attribute merged with something else). That aside, I simply feel like it's better to provide few, general functions rather than more specific instantiations of them.

[byorgey]

Looks good to me.

[cchalmers]

I wouldn't call mkAttrLens general because the three prisms are the only use case. Anything you could compose with the Prism given to mkAttrLens could be composed with the resulting lens (plus more), so I'll keep it as it is.

I'll get lib rebased with HEAD and merge this.