[Feature request] Discard entry/entries

[Mark2Mark]

Loving this tools so far a lot. Thanks for making it available.

I wonder if a "Discard" Context Menu Item could be added. Preferably on any possible level of the hierarchy. I happen to use discard a lot with other git apps as a kind of bigger undo.

[florianpircher]

Thanks for trying it! Yes, there are a many small things like this that are just waiting to get implemented. But do not hesitate to call anything out if you want/need it, it helps me prioritize things.

Discarding entire files is somewhat easy. Discarding individual changes within files is more difficult, but doable.

The main question is how that would work with the open document. If you discard a change in a glyph or some metadata in the fontinfo.plist, that changes the underlying files. I think Light Table should thus reload the font of the document to reflect the change state. Or maybe I can reload just bits instead of the entire GSFont. Maybe @schriftgestalt knows if there is an efficient way to sync the open document to changes of the source files.

[schriftgestalt]

Reloading the full font is probably the safest. It might be possible to implement some special cases like load and replacing a single glyph or just the kerning.

[Mark2Mark]

Interesting. Looks like this could be another advantage of using Light Table over another Git app, as it can communicate better with Glyphs.

I just double checked and surely, using the discard function e.g. in the Github App does not show in Glyphs until you re-open the file. (I guess you know that already).

I think it might not be a high priority feature, but I think things like discard a (part of a) change is a good selling point for users who are not yet well with git.

[florianpircher]

Next version.

[florianpircher]

I got discarding to work by reloading the font of the document. That works, but reloading the font of an open document is not something that many plugins expect (and even some parts of Glyphs are not fully prepared for such a hot swap).

So, it would be better to discard changes by analyzing the semantics of the changes and undoing them “manually” (from Light Table’s perspective) in the document. For example, move nodes back by setting their position, inserting deleted anchors again, removing added layers, etc. That also allows the user to go back to the document window and undo the discard action in many cases. But this approach requires a of special “handlers” that know how to discard all kinds of changes. And that will take some time. I got discarding modified nodes working; once I can also handle added/deleted nodes, then I will publish a first preview to discard those and only those and then expand from there. Maybe a better approach comes to mind …

[Mark2Mark]

🤯

[florianpircher]

Discarding nodes mostly works now:

There are still a few edge cases (nodes on background layer, nodes with attributes) that are not supported yet, but the concept of discarding changes by manipulating the Glyphs objects directly appears fruitful.

[Mark2Mark]

Woohooo, this is a biggie. I think this can help "selling" a git workflow even more to not-yet-git type designers 🔥 That is amazing.

[florianpircher]

I finally figured out what it takes to do the above reliably. However, the system needs extra work to handle different parts of a file. Currently, I can discard anchors and nodes. Are there any other parts that you would be most interested in discarding for a start?

Technical Details

I need some sort of “handler” code that can discard objects in the file. For example, a handle that discards an anchor. That handler can then discard either all changes of an anchor or just some changes (e.g., discard only the position but not the user data if both are changed).

“Fine-grained” handlers (e.g., font → glyph → layer → anchor → position) are more efficient and less disruptive than “coarse” handlers (e.g., font → glyph → layer). Coarse handers can still operate on just a small part of their inner structure, just like fine-grained handlers. However, to do so, coarse handlers must make bigger modifications to the Glyphs objects (e.g., replace an entire layer just to move an anchor). Bigger modifications don’t always play nice with Glyphs, especially plugins.

So, I would like to start with fine-grained handlers first. They are quite laborious, though, as it would need many of them to cover all subtrees of the file structure. This is why I would like to prioritize certain file structure subtrees for now and fill in the gaps and/or create total coverage with coarse handlers later.

[schriftgestalt]

Can’t you build a factory that builds those handlers? They can’t be soo different?

[florianpircher]

I have thus far abstracted discarding (internally “editing”) into two types of handlers. Handlers where the object is identifiable (e.g. ID’d by name) and handlers where the object is positional (e.g. index in array). Anchors are identifiable, nodes are positional. Both work much the same, but they have different strategies to get the container GS-object. After that, they both have the same set of functions: create a GS-object from property list data (sourceValue), and remove, insert, and update the old/new GS-object:

let layerEditHandlers: PathMapping<DiffItem.Path, EditHandler> = [
    "anchors": [
        .any: .match(.identifiableElement("anchor", isUndoable: true) { item, info in
            guard let (layer, subpath) = layer(for: item.path, info: info),
                  let item = item.asItem(ofType: DictionaryDiffItem.self),
                  let anchorNameItem = item.nonReplacementItem(forKey: "name", as: StringDiffItem.self) else {
                throw EditHandler.Error.invalidContainer
            }
            return (layer, anchorNameItem.delta)
        } sourceValue: { item, info, id, sourceFormatVersion in
            try GSAnchor.decode(item.encodePlist(representation: .staged), format: sourceFormatVersion)
        } remove: { layer, targetAnchorName in
            layer.removeAnchor(withName: targetAnchorName.value)
        } insert: { layer, sourceAnchorName, sourceAnchor in
            layer.addAnchor(sourceAnchor)
        } update: { layer, targetAnchorName, sourceAnchor in
            guard let targetAnchor = layer.anchor(forName: targetAnchorName.value) else {
                throw EditHandler.Error.missingTargetValue
            }
            targetAnchor.name = sourceAnchor.name
            targetAnchor.position = sourceAnchor.position
            targetAnchor.orientation = sourceAnchor.orientation
            targetAnchor.isLocked = sourceAnchor.isLocked
            targetAnchor.userData = sourceAnchor.userData
        })
    ],
    "shapes": [
        .any: [
            "nodes": [
                .any: .match(.positionalElement("node", isUndoable: true) { item, info in
                    guard let (path, subpath) = shape(for: item.path, info: info, as: GSPath.self),
                          subpath.count == 2,
                          subpath[offset: 0] == "nodes",
                          case .index(let nodeIndex) = subpath[offset: 1] else {
                        throw EditHandler.Error.invalidContainer
                    }
                    return (path, nodeIndex)
                } sourceValue: { item, info, sourceFormatVersion in
                    try GSNode.decode(item.encodePlist(representation: .staged), format: sourceFormatVersion)
                } remove: { path, targetIndex in
                    path.removeObjectFromNodes(at: targetIndex)
                } insert: { path, insertionIndex, sourceNode in
                    path.insertObject(sourceNode, inNodesAt: insertionIndex)
                } update: { path, targetIndex, sourceNode in
                    guard let targetNode = path.node(at: targetIndex) else {
                        throw EditHandler.Error.missingTargetValue
                    }
                    targetNode.position = sourceNode.position
                    targetNode.type = sourceNode.type
                    targetNode.connection = sourceNode.connection
                    targetNode.orientation = sourceNode.orientation
                    targetNode.isLocked = sourceNode.isLocked
                    targetNode.attributes = sourceNode.attributes
                    targetNode.userData = sourceNode.userData
                }),
            ],
        ],
    ],
]

“Moves” (a positional object getting placed at a new position, e.g., reordering custom parameters) are currently implemented as a remove + insert action. That should also get its own move function definition in handlers for efficiency.

All of the code above can probably be abstracted further once I implement hints, guides, annotations, etc. and recognize the common patterns. But first, I would like to do this step by step so that I can test that the edit subsystem can make these modifications reliably.