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

Debugging signals 🩻 #1389

Open
rickmcgeer opened this issue Apr 4, 2024 · 9 comments
Open

Debugging signals 🩻 #1389

rickmcgeer opened this issue Apr 4, 2024 · 9 comments
Labels
✨ enhancement New feature or request 📟 support inquiry Questions that can be answered without changing code

Comments

@rickmcgeer
Copy link
Collaborator

rickmcgeer commented Apr 4, 2024

What would you like to achieve?
I'm clicking on a Button and nothing is happening, even through the code says it should. When I click the Close Button on BugReporter nothing happens. BugReporter is implemented here:

$world.execCommand("open browser", {moduleName: "studio/helpers.cp.js", packageName: "engageLively--galyleo-dashboard", codeEntity: "BugReporter"});

BugReporter's model is BugReporterModel, which has the following code:

bindings: {
        get () {
          return [
            { model: 'close button', signal: 'fire', handler: 'close' },
            { model: 'report button', signal: 'fire', handler: 'reportBug' }
          ];
        }
      }

The close method is this.view.remove(), and when I execute it by hand (pull up the Object Editor, choose to edit the model, then run the method directly, the BugReporter instance is removed.
BugReporter's close button is an instance of MenuBarButton, defined here:

$world.execCommand("open browser", {moduleName: "studio/shared.cp.js", packageName: "engageLively--galyleo-dashboard", codeEntity: "MenuBarButtonDefault"});

Its defaultViewModel is ButtonModel

$world.execCommand("open browser", {moduleName: "buttons.js", packageName: "lively.components", codeEntity: "ButtonModel"});

and ButtonModel clearly has the signal fire defined:

From the get properties() method:

fire: {
        group: 'button', derived: true, readOnly: true, isSignal: true
      },

In other words, ButtonModel defines the signal fire, BugReporterModel binds button's fire signal to close, and it's quite clear close isn't getting called.

The thing is, I don't know how to debug this, and I don't have an alternative solution other than to subclass the View and override the appropriate pressed method to call it directly

How are you trying to achieve that

Alternative solutions
If applicable, what other solutions have you tried?

Additional Resources
Please provide links to any custom code that might be necessary to grasp your problem. Screenshots of custom component, etc. are also appreciated.

Version: Please paste the lively.next commit on which the problem occurred here (use the copy button of the Version Checker in the bottom left corner).
9d55f9b

@rickmcgeer rickmcgeer added the 📟 support inquiry Questions that can be answered without changing code label Apr 4, 2024
@rickmcgeer
Copy link
Collaborator Author

I found a workaround for my issue, so I can make progress. All I have to do (in BugRerporterModel) is do the following on initialization:

this.ui.closeButton.viewModel.action = _ => this.close();

which I can presumably do for every other button. I'd sooner not, for obvious reasons. But it does show me that the fire signal isn't being received, because the code to fire a ButtonModel's action is the line after the fire signal is sent.
As @merryman knows, I've been unhappy about this since pins and connections in the Lively Web 10 years ago. It's just really opaque and hard to trace.

@linusha
Copy link
Contributor

linusha commented Apr 4, 2024

@rickmcgeer on a general level, we are also not happy with this part of the system, but until now never had an idea how to tackle it. I now thought of something that might be worthwhile and hope to prototype it soon. We have #163 for this - the problem persists since basically forever, as you mentioned.

In a first step we will provide better documentation soon and I hope that alleviates some of the pain already.


Regarding this concrete problem:

We do not recommend using model inside of the bindings anymore. It works, but it encourages sloppy coding practices. The semantic of using model is such that

{ model: 'report button', signal: 'fire', handler: 'reportBug' }

binds a signal fired inside of the ViewModel class of a morph with the name report button to the reportBug method of the ViewModel the binding is defined in.
The problem with bindings like these is that it allows one to connect basically arbitrary stuff all over the place which turns out it is no good for maintenance.

The alternative to using model inside of a binding is using target: <morph name>. This connects to the morph itself and this is also where the fire signal on the buttons is signalled, as can be seen in the trigger method of the ButtonModel:

 trigger () {
    try {
      signal(this.view, 'fire');
      typeof this.action === 'function' && this.action();
    } catch (err) {
      const w = this.world();
      if (w) w.logError(err);
      else console.error(err);
    }
  }

I checked the dashboard out locally and replaced model with target and then I could close the prompt as expected.

As a general rule, components coming from the lively core will signal their stuff on the view, so than target should always be used.


As we recommend not binding onto ViewModels, we also built a small convenience thing for custom components: If you signal hello via signal(this, 'hello') inside of a ViewModel and hello is exposed on the ViewModel, the signal will automatically be signalled on the View as well. This way, you should always be able to just use target for your bindings. The idea behind this is that this way, expose neatly defines the public interface of a component.


I hope this helps, let me know if you have any more questions.

@rickmcgeer
Copy link
Collaborator Author

@linusha Thanks!
What I had suggested as a practice was something similar -- have the View implement a listener pattern. Each View would maintain a queue of listeners, each of which would be methods which took a single argument, the morph. Something like this:

class Listener {
  constructor(morph) {
    this.queue = [];
    this.owner = morph;
  }
  addListener(method) {
    // method should take a single argument, morph, to indicate which morph fired the signal
     this.queue.push(method);
  }
  removeListener(method) {
    this.queue = this.queue.filter(lambda => method != lambda);
  }
  getListeners() {
    return this.queue.slice(); // ensure no external manipulation of the queue
  }
  fire() {
    // just call each listening method with the morph
    this.queue.forEach(method => method(this.owner));
  }
}

/* Code in the View */
// init.  Create the listener for the signal.  No explicit binding
  this.mySignalListener = Listener(this) // for signal action

fireMySignal() {
  // Method which is the target for the signal, and just tells the listener to fire
  this.mySignalListener.fire();
}

// property
// Declaration of the signal
mySignal: {
        derived: true, readOnly: true, isSignal: true
},

// binding.  Bind the firing of the signal to the fireMySignal method on the morph itself

{ target: this, signal: 'mySignal', handler: 'fireMySignal' }

Does this make sense? The big advantage is that all bindings are local to the Morph and the programmer can inspect the queue to see what will fire.
Is this consistent with what you suggested? I'm really asking to see if I understand this well.

@rickmcgeer
Copy link
Collaborator Author

@linusha would {target: this.view work as well as {target: <morph name>? I could use this.view.name, I guess...

@linusha
Copy link
Contributor

linusha commented Apr 4, 2024

@linusha would {target: this.view work as well as {target: <morph name>? I could use this.view.name, I guess...

Good question! In the case you want to bind a signal directly on the view of a ViewModel, you can omit the target altogether! So { signal: mySignal, handler: myFunctionToTrigger } is a valid binding already!


I'll come back to your other reply tomorrow!

@rickmcgeer
Copy link
Collaborator Author

Thanks, @linusha

@linusha
Copy link
Contributor

linusha commented Apr 5, 2024

@rickmcgeer I have to say I did not grok your code code totally, but I tend to say yes, this would work too.

I'd say the advantage of the bindings syntax we use is that there is a single canonical place for all bindings to exist and one can get an overview quickly. Together with the mechanism that utilizes expose that I outlined above, this leads to a clear place where the interface of the component is defined.

The obvious downside to this is, as you rightly pointed out, that we provide no tooling at this point in time to quickly find out if everything is setup correctly.

Just for my understanding: Previsouly, Robin and I often talked about an interactive interface to setup bindings, and had problems thinking of something that scales.
If I understood you correctly, defining the bindings with the above syntax is alright, but you are missing something to inspect the resulting connection in the case something goes amiss, is that correct @rickmcgeer ?

@rickmcgeer
Copy link
Collaborator Author

@linusha yes, that's correct. The problem is that there's no way to see what is actually bound to the signal. That was also the case with "pins" in the Lively Web. The other problem is that binding is by name (instead of by object) and this is ambiguous -- the actual binding is to some object, of course, but which object isn't known until runtime.
There is an advantage in doing it this way, of course -- if the programmer does something like:

{signal: 'fire', handler: 'action'}

Then the binding is to any subclass.

Let me explain where the code in my previous comment came from. In many systems, UI elements export out events. An event is basically the same thing as a signal in Lively. Each event maintains a queue of listeners, and there's a specific method to add and delete listeners from events. When an event is fired, the listeners are invoked. The advantages to this architecture are:

  1. binding is by object, not by name. I don't bind the 'action' method of my object to the signal; instead, I attach whatever method I want to the event's listener queue. So there's no ambiguity about what's getting invoked.
  2. At any time, the programmer can check if a method is on the event's listener queue (event.getListerners.indexof(myMethod) >= 0)
  3. Event binding is dynamic, at runtime

This is a common architecture (and it's easy to implement on top of the current binding method that lively.next has, I think). It isn't the only one that could be used, and it could easily be that you and @merryman can come up with a better one. I'm far from the best software architect! :-). So I'm not saying that this must or should be used; it's a suggestion (and for that matter, for my own purposes I can put it on top of any architecture where the primitive binding is unambiguous).

FWIW (and this is just advice) I don't think scaling is the problem; I've never seen a case where there are lots of signals in a system. What I think the problem is is that it isn't obvious, to me at least, which method is being bound to which signal. I suspect that (and there was a lot of this in the original Lively) the original mechanism was designed to be very general and flexible -- so a connection could be made pretty much arbitrarily and in a number of different ways. That's a benefit in a lot of ways, but it does mean that the underlying semantics aren't clear and are sometimes ambiguous.

@merryman merryman changed the title Debugging signals Debugging signals 🩻 Apr 9, 2024
@merryman merryman added the ✨ enhancement New feature or request label Apr 9, 2024
@merryman
Copy link
Member

merryman commented Apr 9, 2024

@linusha What Rick basically wants is X-Ray vision for morphs that visualizes where the connections flow, and also where attachments for connections are located

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement New feature or request 📟 support inquiry Questions that can be answered without changing code
Projects
None yet
Development

No branches or pull requests

3 participants