On Monday, 6 January 2014 at 10:39:41 UTC, Dicebot wrote:
> On Monday, 6 January 2014 at 10:13:44 UTC, Vladimir Panteleev wrote:
>> Apologies, I missed this review thread.
>
> =/ Any good idea to make those more visible? Maybe we should setup a mailing list for those who want to be notified about ongoing reviews?

Yes, I also think that we should have separate mailing list for reviews and votings. At least, it will be easier to find previous reviews.

On Monday, 6 January 2014 at 10:39:41 UTC, Dicebot wrote:
> On Monday, 6 January 2014 at 10:13:44 UTC, Vladimir Panteleev wrote:
>> Apologies, I missed this review thread.
>
> =/ Any good idea to make those more visible? Maybe we should setup a mailing list for those who want to be notified about ongoing reviews?

Well, it's my fault, really - I was not following D development at the time, as I was visiting relatives abroad - and if voting would have started after 2 weeks of review, I probably would not had been able to participate there either.

On Monday, 6 January 2014 at 10:13:44 UTC, Vladimir Panteleev wrote:
> On Thursday, 7 November 2013 at 13:45:57 UTC, Dicebot wrote:
>> Time to move forward with some more reviews :)
>
> Hi,
>
> Apologies, I missed this review thread. So, some questions / observations:
>
> IMO, the effort to disallow third parties to "emit" signals on your object by far exceeds its worth.
>
> For this purpose, you have needed:
> - string mixins
>   - ...which declare, by convention, a field with a name not explicitly given by the user
>   - ...with a workaround for when they don't work
> - an enumeration type
> - two different signal struct types
> - a boatload of documentation
> - a nontrivial cognitive effort on the user to grok all of the above
>
> This little feature steals too much focus, and IMO is not worth it.
>
> As a compromise, have you considered template mixins?

I did, but there are still some bugs with template mixins, which finally drove me to a string mixin and in the end I liked it better. IMO there is no real benefit of a template mixin over a string mixin in this case. The implementation got a lot easier and bug free.

Instead of dropping it all together, I think I will just move it to the bottom and make it a goody, instead of the proposed default usage of std.signal.

>
>> This implementation supersedes the former std.signals
>
> I don't think comparisons of older versions of a module belong in the module's documentation.

You are probably right, will fix that.


>
>> dg must not be null. (Its context may.)
>
> I don't think the context will ever be null (unless you take the address of a null class/struct pointer's method). (toDelegate uses the context pointer to store the function pointer, and the function pointer to store a template instance according to the function's arguments.)

It is for me, if I create a lamda delegate which does not use it's context. But I might just cut this from the documentation.

>
>> It implements the emit function for all other functionality it has this aliased to RestrictedSignal.
>
> Missing some punctuation, I think.

Fixed, thanks!


>
>> The idea is to instantiate a Signal privately and provide a public accessor method for accessing the contained RestrictedSignal.
>
> I had a hard time understanding this line the first time. Please reword without using "The idea is", because it's not clear what "the idea" is for - usage? Rationalization of implementation method?
>
>> Use this overload if you either really, really want strong ref semantics for some reason
>
> Was the emphasis really necessary? It makes complete sense to build an asynchronous-event-model application using 100% only "strong connections" (delegates or delegate arrays). I don't remember ever NEEDING weak-referenced callbacks.

You are right, but you already imply the reason in your argument, if you need strong connections for your application, you are probably better off with a simple delegate array. From discussions on this mailinglist it became apparent that people often use strong connections for no good reasons, nevertheless I just dropped it :-)

Thanks a lot for this valuable feedback!

Best regards,

Robert

On 2014-01-06 11:13, Vladimir Panteleev wrote:

> Apologies, I missed this review thread. So, some questions / observations:
>
> IMO, the effort to disallow third parties to "emit" signals on your
> object by far exceeds its worth.
>
> For this purpose, you have needed:
> - string mixins
>    - ...which declare, by convention, a field with a name not explicitly
> given by the user
>    - ...with a workaround for when they don't work
> - an enumeration type
> - two different signal struct types
> - a boatload of documentation
> - a nontrivial cognitive effort on the user to grok all of the above
>
> This little feature steals too much focus, and IMO is not worth it.

I agree that it's not so nice looking. But I feel that it could be quite important to restrict emitting to the module the signal is declared in.

>> a.valueChanged.connect!"watch"(o);        // o.watch is the slot
>
> This is really ugly, but I don't see any way to improve it either. Maybe
> if the type of &classInstance.method was actually a special
> "pointer-to-class-method" type that implicitly down-casted to a
> delegate, it would be possible to safely use &o.watch instead...

I agree. Is the only reason to have a weak connection?


-- 
/Jacob Carlborg

On 2013-11-07 13:45:56 +0000, Dicebot said:

> Our current victim is std.signal by Robert Klotzner which is supposed to deprecate and supersede existing (and broken) std.signals

Hi, not really a comment regarding the actual implementation but I think that good debug support for signales & slots helps a lot in using it. What do I mean with this:

- a way to dump in a human readable form the run-time connections. Which function / class / etc. is currently attached to which signal?
- automatic logging like a call-stack in a debugger to get an idea when which signal is acted on
- a way to get the order of activation for debugging to identify unwanted side-effects
- etc.

Big signal & slot implementaitons can be hard to debug, this should be as simple as possible.

-- 
Robert M. Münch
Saphirion AG

http://www.saphirion.com
smarter | better | faster

Definitely a good thing to have. But nothing that could not be added later on, for now I just hope there will be enough people voting, so it can actually be included in phobos.

If your only concern is additional features and nothing that is wrong with the API, consider voting.

I added the issue to phobosx for now:

https://github.com/phobos-x/phobosx/issues/4

Best regards,

Robert

On Saturday, 11 January 2014 at 18:02:15 UTC, Robert M. Münch wrote:
> On 2013-11-07 13:45:56 +0000, Dicebot said:
>
>> Our current victim is std.signal by Robert Klotzner which is supposed to deprecate and supersede existing (and broken) std.signals
>
> Hi, not really a comment regarding the actual implementation but I think that good debug support for signales & slots helps a lot in using it. What do I mean with this:
>
> - a way to dump in a human readable form the run-time connections. Which function / class / etc. is currently attached to which signal?
> - automatic logging like a call-stack in a debugger to get an idea when which signal is acted on
> - a way to get the order of activation for debugging to identify unwanted side-effects
> - etc.
>
> Big signal & slot implementaitons can be hard to debug, this should be as simple as possible.

>>
>> This is really ugly, but I don't see any way to improve it either. Maybe
>> if the type of &classInstance.method was actually a special
>> "pointer-to-class-method" type that implicitly down-casted to a
>> delegate, it would be possible to safely use &o.watch instead...
>
> I agree. Is the only reason to have a weak connection?

Yes. Weak connections are the only reason.

On Tuesday, 7 January 2014 at 08:18:26 UTC, Robert wrote:
> Instead of dropping it all together, I think I will just move it to the bottom and make it a goody, instead of the proposed default usage of std.signal.

[...]

> You are probably right, will fix that.

Has this been done? I don't see any changes on https://vhios.dyndns.org/dlang.org/web/phobos/std_signal.html

> It is for me, if I create a lamda delegate which does not use it's context.

Right, I forgot about that case.

On Sunday, 12 January 2014 at 01:06:45 UTC, Vladimir Panteleev wrote:
> On Tuesday, 7 January 2014 at 08:18:26 UTC, Robert wrote:
>> Instead of dropping it all together, I think I will just move it to the bottom and make it a goody, instead of the proposed default usage of std.signal.
>
> [...]
>
>> You are probably right, will fix that.
>
> Has this been done? I don't see any changes on https://vhios.dyndns.org/dlang.org/web/phobos/std_signal.html

Look this page:
https://vhios.dyndns.org/dlang.org/web/phobos-prerelease/std_signal.html
Is it OK?

On Saturday, 11 January 2014 at 18:02:15 UTC, Robert M. Münch wrote:
> Hi, not really a comment regarding the actual implementation but I think that good debug support for signales & slots helps a lot in using it. What do I mean with this:
>
> - a way to dump in a human readable form the run-time connections. Which function / class / etc. is currently attached to which signal?
> - automatic logging like a call-stack in a debugger to get an idea when which signal is acted on
> - a way to get the order of activation for debugging to identify unwanted side-effects
> - etc.
>
> Big signal & slot implementaitons can be hard to debug, this should be as simple as possible.

Yes, it will be good to have.

Also, you can vote `yes with condition` and write your condition after it (I think this description is good). In this case Robert receives your vote only if he implement your condition. It looks like not so many people use signals, so any feedback are welcome.