search

robotools / defcon

A set of UFO based objects for use in font editing applications.
MIT License
66 stars 36 forks source link

order of registry posibilities #102

Open typemytype opened 7 years ago

typemytype commented 7 years ago

when an object is self observing it is subscribed to all notifications, this makes sense in order to destroy representation factories.

But the order of calling observers puts the self observing as third option. see https://github.com/typesupply/defcon/blob/master/Lib/defcon/tools/notifications.py#L146-L151

This causes issues when a factories has no destructive notifications (aka ".Changed" notification) as this is being called afterwards.

a small example to demonstrate this issue.

I don't have any idea to push self observing notifications upfront. The factories could add all possible notifications but that does not feels right, especially with heavy calculating factories.

from defcon import Font, Kerning, registerRepresentationFactory

count = 0

def testFactory(kerning):
    print "    *** create a new repr ***"
    global count
    count += 1
    return count

registerRepresentationFactory(Kerning, "test", testFactory, destructiveNotifications=None)

class TestKerning(Kerning):

    def selfNotificationCallback(self, notification):
        print "    ----> self notification", notification.name
        super(TestKerning, self).selfNotificationCallback(notification)
        print "    ----> self notification done", notification.name

class Test(object):

    def __init__(self):
        f = Font(kerningClass=TestKerning)
        f.kerning.addObserver(self, "_kerningChanged", "Kerning.Changed")
        print "testing kerning:"
        print "initiate", f.kerning.getRepresentation("test")
        f.kerning[("a", "b")] = 10
        print "done", f.kerning.getRepresentation("test")

    def _kerningChanged(self, notification):
        print "    kerning changed", notification.name
        print "    %s" % notification.object.getRepresentation("test"), "(should build a new representation but the factory is not destroyed yet...)"

Test()
testing kerning:
initiate     *** create a new repr ***
1
    ----> self notification Kerning.PairSet
    ----> self notification done Kerning.PairSet
    kerning changed Kerning.Changed
    1 (should build a new representation but the factory is not destroyed yet...)
    ----> self notification Kerning.Changed
    ----> self notification done Kerning.Changed
done     *** create a new repr ***
2
typemytype commented 7 years ago

I wondering why the order is going from most specific -> least specific?

adrientetar commented 7 years ago

I just hit this issue with a SelectionChanged notification sent from contour, which the glyph watches then sends its own Glyph.SelectionChanged that destroys the representation.
This problem affects the call to destroyRepresentations in BaseObject since it makes a subscription to all the notifications it sends. This means destructors aren't run first when they absolutely should.

@typesupply Could you explain the rationale for the order of notifications:

I wondering why the order is going from most specific -> least specific?

I expect simply reversing that order should fix this issue.

Edit: Although reversing the order fixes the issue with destroyRepresentations, I think destructors should have some special rooting in NotificationCenter so they're guaranteed to be called first.

typesupply commented 7 years ago

Could you explain the rationale for the order of notifications:

I don't remember. That # most specific -> least specific note is a sign that I had some reason, but I don't know what it was. I'm 100% okay with changing the order and seeing if it breaks anything. I'm also okay with some sort of special destructive pre-run if necessary.

  • © Githubissues.
  • Githubissues is a development platform for aggregating issues.