Skip to content

Conversation

vicb
Copy link

@vicb vicb commented Feb 12, 2014

This is really a WIP, I'll be pushing changes as they stabilize.

The goal of this PR is only informative for now.

@vicb
Copy link
Author

vicb commented Feb 13, 2014

@mhevery I've just rebased this PR on your latest changes. I'll add changes here once they are stable.

Feel free to merge this PR any time back into your branch if you agree with the changes, it will prevent me from having to rebase often. In the meantime I'll ping you when I add more commits so that you can validate them.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may not be the right style, but I find the original code way easier to read.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're talking about the extra else... it's no more equivalent but it is now symmetric.

If you're talking about all on the same line vs separated lines, we should have Dart specific CCs if we go this way otherwise anyone could use his own and the result will be messy. For now I try to apply Dart CCs.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am of the opinion that we should follow the guidelines, unless they get
in the way of readability. In this case having things in one line makes it
very easy to see that b.x and b.y logic is same just y replaced with x. If
you do proper style, you loose that benefit. So on occasions I do break the
rule.

On Thu, Feb 13, 2014 at 9:24 AM, Victor Berchet notifications@github.comwrote:

In demo/bouncing_balls/bouncy_balls.dart:

 fps = (1000/delay).round();
 for(var i=0, ii=balls.length; i<ii; i++) {
   var b = balls[i];
   b.x += delay * b.velX;
   b.y += delay * b.velY;
  •  if (b.x < 0) { b.x *= -1; b.velX *= -1; }
    
  •  if (b.y < 0) { b.y *= -1; b.velY *= -1; }
    
  •  if (b.x > width) { b.x = 2*width - b.x; b.velX *= -1; }
    
  •  if (b.y > height) { b.y = 2*height - b.y; b.velY *= -1; }
    
  •  if (b.x < 0) {
    

If you're talking about the extra else... it's no more equivalent but it
is now symmetric.

If you're talking about all on the same line vs separated lines, we should
have Dart specific CCs if we go this way otherwise anyone could use his own
and the result will be messy. For now I try to apply Dart CCs.


Reply to this email directly or view it on GitHubhttps://github.com//pull/6/files#r9717945
.

@mhevery
Copy link
Owner

mhevery commented Feb 13, 2014

MERGED, I already updated, just FYI

@mhevery mhevery closed this Feb 13, 2014
mhevery pushed a commit that referenced this pull request May 15, 2014
-  BUG: DynamicFieldGetterFactory::isMethod did not handle methods
   defined in superclasses.
-  BUG: Upon detecting a method, the code assumed that you would only
   invoke it.  This broke application code that watched a method (e.g.
   by way of Component mapping) just to get the closurized value and
   store it somewhere to invoke later (test case and stack trace at end
   of this commit message.)
-  BUG: StaticFieldGetterFactory::method() and
   DynamicFieldGetterFactory::method() differed.  There was no
   difference between StaticFieldGetterFactory::method() and
   StaticFieldGetterFactory::getter(). 
   DynamicFieldGetterFactory::method(), as mentioned before, assumed
   that the only thing you could do with it was to invoke it (i.e. not a
   leaf watch.)
-  There was very little testing for StaticFieldGetterFactory.  This
   meant that, though it was out of sync with DynamicFieldGetterFactory,
   no tests were failing.

Changes in this commit:

- run the same tests against StaticFieldGetterFactory that are run
  against DynamicFieldGetterFactory
- do not call the result of GetterFactory.method()
  in "set object(value)"
- reduce the difference between the two different factories. 
  GetterFactory now only has one method in it's interface definition -
  the getter function.  `_MODE_METHOD_INVOKE_`, `isMethod`,
  `isMethodInvoke`, etc. are gone.

**Bug Details:**

Refer to the repro case at
chirayuk/angular.dart@issue_999^...issue_999

```dart
// Given this object.
class Foo {
  bar(x) => x+1;
}

// This test case (in an appropriate file like `scope_spec.dart`) fails
// with a traceback

it('should watch closures', (RootScope rootScope, Logger log) {
  rootScope.context['foo'] = new Foo();
  rootScope.context['func'] = null;
  rootScope.watch('foo.bar', (v, _) { rootScope.context['func'] = v; });
  rootScope.watch('func(1)', (v, o) => log([v, o]));
  rootScope.apply();
  expect(log).toEqual([[null, null], [2, null]]);
});
```

**Stack Trace:**

    Chrome 34.0.1847 (Mac OS X 10.9.2) scope watch/digest should watch closures FAILED
    Test failed: Caught Closure call with mismatched arguments: function 'DynamicFieldGetterFactory.method.<anonymous closure>'

    NoSuchMethodError: incorrect number of arguments passed to method named 'DynamicFieldGetterFactory.method.<anonymous closure>'
    Receiver: Closure: (List, Map) => dynamic
    Tried calling: DynamicFieldGetterFactory.method.<anonymous closure>(Instance of 'Foo')
    Found: DynamicFieldGetterFactory.method.<anonymous closure>(args, namedArgs)
    #0      Object.noSuchMethod (dart:core-patch/object_patch.dart:45)
    #1      DirtyCheckingRecord.object= (package:angular/change_detection/dirty_checking_change_detector.dart:465:78)
    #2      _FieldHandler.acceptValue (package:angular/change_detection/watch_group.dart:630:17)
    #3      WatchGroup.addFieldWatch (package:angular/change_detection/watch_group.dart:171:29)
    #4      FieldReadAST.setupWatch (package:angular/change_detection/ast.dart:67:31)
    #5      WatchGroup.watch.<anonymous closure> (package:angular/change_detection/watch_group.dart:144:40)
    #6      _HashMap.putIfAbsent (dart:collection-patch/collection_patch.dart:124)
    #7      WatchGroup.watch (package:angular/change_detection/watch_group.dart:143:27)
    #8      Scope.watch (package:angular/core/scope.dart:240:31)
    #9      main.<anonymous closure>.<anonymous closure>.<anonymous closure> (/Users/chirayu/work/angular.dart/test/core/scope_spec.dart:1308:24)
    #10     _LocalInstanceMirror._invoke (dart:mirrors-patch/mirrors_impl.dart:440)
    #11     _LocalInstanceMirror.invoke (dart:mirrors-patch/mirrors_impl.dart:436)
    #12     _LocalClosureMirror.apply (dart:mirrors-patch/mirrors_impl.dart:466)
    #13     DynamicInjector.invoke (package:di/dynamic_injector.dart:97:20)
    #14     _SpecInjector.inject (package:angular/mock/test_injection.dart:58:22)
    #15     inject.<anonymous closure> (package:angular/mock/test_injection.dart:100:44)
    #16     _rootRun (dart:async/zone.dart:723)
    #17     _ZoneDelegate.run (dart:async/zone.dart:453)
    #18     _CustomizedZone.run (dart:async/zone.dart:663)
    dart-archive#19     runZoned (dart:async/zone.dart:954)
    dart-archive#20     _syncOuter.<anonymous closure> (package:angular/mock/zone.dart:227:22)
    dart-archive#21     _withSetup.<anonymous closure> (/Users/chirayu/work/angular.dart/test/jasmine_syntax.dart:13:14)
    dart-archive#22     _run.<anonymous closure> (package:unittest/src/test_case.dart:102:27)
    dart-archive#23     _rootRunUnary (dart:async/zone.dart:730)
    dart-archive#24     _ZoneDelegate.runUnary (dart:async/zone.dart:462)
    dart-archive#25     _CustomizedZone.runUnary (dart:async/zone.dart:667)
    dart-archive#26     _Future._propagateToListeners.handleValueCallback (dart:async/future_impl.dart:488)
    dart-archive#27     _Future._propagateToListeners (dart:async/future_impl.dart:571)
    dart-archive#28     _Future._completeWithValue (dart:async/future_impl.dart:331)
    dart-archive#29     _Future._asyncComplete.<anonymous closure> (dart:async/future_impl.dart:393)
    dart-archive#30     _rootRun (dart:async/zone.dart:723)
    dart-archive#31     _ZoneDelegate.run (dart:async/zone.dart:453)
    dart-archive#32     _CustomizedZone.run (dart:async/zone.dart:663)
    dart-archive#33     _BaseZone.runGuarded (dart:async/zone.dart:574)
    dart-archive#34     _BaseZone.bindCallback.<anonymous closure> (dart:async/zone.dart:599)
    dart-archive#35     _asyncRunCallbackLoop (dart:async/schedule_microtask.dart:23)
    dart-archive#36     _asyncRunCallback (dart:async/schedule_microtask.dart:32)
    dart-archive#37     _handleMutation (file:///Volumes/data/b/build/slave/dartium-mac-full-dev/build/src/dart/tools/dom/src/native_DOMImplementation.dart:588)

    DECLARED AT:#0      inject (package:angular/mock/test_injection.dart:97:5)
    #1      _injectify (/Users/chirayu/work/angular.dart/test/_specs.dart:236:25)
    #2      iit (/Users/chirayu/work/angular.dart/test/_specs.dart:244:53)
    #3      main.<anonymous closure>.<anonymous closure> (/Users/chirayu/work/angular.dart/test/core/scope_spec.dart:1305:10)
    #4      describe.<anonymous closure> (/Users/chirayu/work/angular.dart/test/jasmine_syntax.dart:62:9)
    #5      group (package:unittest/unittest.dart:396:9)
    #6      describe (/Users/chirayu/work/angular.dart/test/jasmine_syntax.dart:60:15)
    #7      describe (/Users/chirayu/work/angular.dart/test/_specs.dart:248:46)
    #8      main.<anonymous closure> (/Users/chirayu/work/angular.dart/test/core/scope_spec.dart:1009:13)
    #9      describe.<anonymous closure> (/Users/chirayu/work/angular.dart/test/jasmine_syntax.dart:62:9)
    #10     group (package:unittest/unittest.dart:396:9)
    #11     describe (/Users/chirayu/work/angular.dart/test/jasmine_syntax.dart:60:15)
    #12     describe (/Users/chirayu/work/angular.dart/test/_specs.dart:248:46)
    #13     main (/Users/chirayu/work/angular.dart/test/core/scope_spec.dart:14:11)
    #14     main.<anonymous closure> (http://localhost:8765/__adapter_dart_unittest.dart:169:15)
    #15     _rootRun (dart:async/zone.dart:723)
    #16     _ZoneDelegate.run (dart:async/zone.dart:453)
    #17     _CustomizedZone.run (dart:async/zone.dart:663)
    #18     runZoned (dart:async/zone.dart:954)
    dart-archive#19     main (http://localhost:8765/__adapter_dart_unittest.dart:146:11)

    #0      _SpecInjector.inject (package:angular/mock/test_injection.dart:60:7)
    #1      inject.<anonymous closure> (package:angular/mock/test_injection.dart:100:44)
    #2      _rootRun (dart:async/zone.dart:723)
    #3      _rootRun (dart:async/zone.dart:724)
    #4      _rootRun (dart:async/zone.dart:724)
    #5      _ZoneDelegate.run (dart:async/zone.dart:453)
    #6      _CustomizedZone.run (dart:async/zone.dart:663)
    #7      runZoned (dart:async/zone.dart:954)
    #8      _syncOuter.<anonymous closure> (package:angular/mock/zone.dart:227:22)
    #9      _withSetup.<anonymous closure> (/Users/chirayu/work/angular.dart/test/jasmine_syntax.dart:13:14)
    #10     _withSetup.<anonymous closure> (/Users/chirayu/work/angular.dart/test/jasmine_syntax.dart:14:5)
    #11     _withSetup.<anonymous closure> (/Users/chirayu/work/angular.dart/test/jasmine_syntax.dart:14:5)
    #12     _run.<anonymous closure> (package:unittest/src/test_case.dart:102:27)
    #13     _rootRunUnary (dart:async/zone.dart:730)
    #14     _ZoneDelegate.runUnary (dart:async/zone.dart:462)
    #15     _CustomizedZone.runUnary (dart:async/zone.dart:667)
    #16     _Future._propagateToListeners.handleValueCallback (dart:async/future_impl.dart:488)
    #17     _Future._propagateToListeners (dart:async/future_impl.dart:571)
    #18     _Future._completeWithValue (dart:async/future_impl.dart:331)
    dart-archive#19     _Future._asyncComplete.<anonymous closure> (dart:async/future_impl.dart:393)
    dart-archive#20     _rootRun (dart:async/zone.dart:723)
    dart-archive#21     _ZoneDelegate.run (dart:async/zone.dart:453)
    dart-archive#22     _CustomizedZone.run (dart:async/zone.dart:663)
    dart-archive#23     _BaseZone.runGuarded (dart:async/zone.dart:574)
    dart-archive#24     _BaseZone.bindCallback.<anonymous closure> (dart:async/zone.dart:599)
    dart-archive#25     _asyncRunCallbackLoop (dart:async/schedule_microtask.dart:23)
    dart-archive#26     _asyncRunCallback (dart:async/schedule_microtask.dart:32)
    dart-archive#27     _handleMutation (file:///Volumes/data/b/build/slave/dartium-mac-full-dev/build/src/dart/tools/dom/src/native_DOMImplementation.dart:588)

Closes dart-archive#999
mhevery pushed a commit that referenced this pull request Jul 1, 2014
Refer dart-archive#1182
and repro https://github.com/chirayuk/sample/tree/issue_1182_leaving_a_nested_ng_view

NgView's register cleanup handlers this way:

    _leaveSubscription = route.onLeave.listen((_) {
      _leaveSubscription.cancel();
      // …
      _cleanUp();
    });

When there are nested ng-views, upon a route change, the parent NgView
calls it's _cleanUp() first (which destroys it's child scope) and then
the child NgView attempts a cleanup.  However, it's child scope is
already detached due to the parent NgView cleaning up causing an
exception.

Stack trace is:

    'package:angular/core/scope.dart': Failed assertion: line 335 pos 12: 'isAttached' is not true.

    STACKTRACE:
    #0      Scope.destroy (package:angular/core/scope.dart:335:12)
    #1      NgView._cleanUp (package:angular/routing/ng_view.dart:130:24)
    #2      NgView._show.<anonymous closure> (package:angular/routing/ng_view.dart:106:15)
    #3      _rootRunUnary (dart:async/zone.dart:730)
    #4      _ZoneDelegate.runUnary (dart:async/zone.dart:462)
    #5      _onRunUnary.<anonymous closure> (package:angular/core/zone.dart:113:63)
    #6      VmTurnZone._onRunBase (package:angular/core/zone.dart:97:16)
    #7      _onRunUnary (package:angular/core/zone.dart:113:17)
    #8      _ZoneDelegate.runUnary (dart:async/zone.dart:462)
    #9      _CustomizedZone.runUnary (dart:async/zone.dart:667)
    #10     _BaseZone.runUnaryGuarded (dart:async/zone.dart:582)
    #11     _BufferingStreamSubscription._sendData (dart:async/stream_impl.dart:333)
    #12     _BufferingStreamSubscription._add (dart:async/stream_impl.dart:263)
    #13     _SyncBroadcastStreamController._sendData.<anonymous closure> (dart:async/broadcast_stream_controller.dart:344)
    #14     _BroadcastStreamController._forEachListener (dart:async/broadcast_stream_controller.dart:297)
    #15     _SyncBroadcastStreamController._sendData (dart:async/broadcast_stream_controller.dart:343)
    #16     _BroadcastStreamController.add (dart:async/broadcast_stream_controller.dart:227)
    #17     Router._leaveCurrentRouteHelper (package:route_hierarchical/client.dart:654:48)
    #18     Router._leaveCurrentRouteHelper (package:route_hierarchical/client.dart:656:47)
    dart-archive#19     Router._leaveCurrentRoute (package:route_hierarchical/client.dart:645:41)
    dart-archive#20     Router._leaveOldRoutes (package:route_hierarchical/client.dart:525:30)
    dart-archive#21     Router._processNewRoute (package:route_hierarchical/client.dart:497:27)
    dart-archive#22     Router._route.<anonymous closure> (package:route_hierarchical/client.dart:481:29)
    dart-archive#23     _rootRunUnary (dart:async/zone.dart:730)
    dart-archive#24     _ZoneDelegate.runUnary (dart:async/zone.dart:462)
    dart-archive#25     _onRunUnary.<anonymous closure> (package:angular/core/zone.dart:113:63)
    dart-archive#26     VmTurnZone._onRunBase (package:angular/core/zone.dart:97:16)
    dart-archive#27     _onRunUnary (package:angular/core/zone.dart:113:17)
    dart-archive#28     _ZoneDelegate.runUnary (dart:async/zone.dart:462)
    dart-archive#29     _CustomizedZone.runUnary (dart:async/zone.dart:667)
    dart-archive#30     _Future._propagateToListeners.handleValueCallback (dart:async/future_impl.dart:488)
    dart-archive#31     _Future._propagateToListeners (dart:async/future_impl.dart:571)
    dart-archive#32     _Future._completeWithValue (dart:async/future_impl.dart:331)
    dart-archive#33     _Future._asyncComplete.<anonymous closure> (dart:async/future_impl.dart:393)
    dart-archive#34     _rootRun (dart:async/zone.dart:723)
    dart-archive#35     _ZoneDelegate.run (dart:async/zone.dart:453)
    dart-archive#36     _onScheduleMicrotask.<anonymous closure> (package:angular/core/zone.dart:117:43)
    dart-archive#37     VmTurnZone._finishTurn (package:angular/core/zone.dart:143:34)
    dart-archive#38     VmTurnZone._onRunBase (package:angular/core/zone.dart:104:43)
    dart-archive#39     _onRunUnary (package:angular/core/zone.dart:113:17)
    dart-archive#40     _ZoneDelegate.runUnary (dart:async/zone.dart:462)
    dart-archive#41     _CustomizedZone.runUnary (dart:async/zone.dart:667)
    dart-archive#42     _BaseZone.runUnaryGuarded (dart:async/zone.dart:582)
    dart-archive#43     _BaseZone.bindUnaryCallback.<anonymous closure> (dart:async/zone.dart:608)

Closes dart-archive#1182
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants