Only Start if not Disabled #2

[michaschwab]

This is a follow up on this pull request: #436
This new pull request is based on the 0.14 branch. Also, I made the changes in the code more obvious and you can see that I barely changed anything in the diff below.

Find the description of the original pull request below:

Unfortunately, jquery ui sortable is very slow sometimes, specially if there are hidden elements as described here: http://stackoverflow.com/questions/9440664/jquery-ui-sortable-drag-initiation-is-slow-when-container-has-hidden-items

This adds up to more than 150ms for me on every user action, which is absolutely inacceptable, because I only need to use ui-sortable in very few cases. I need to be able to disable it.

Right now, it is not possible to hold off on launching ui sortable, similar to the problem discussed here: #397
Unfortunately, disabling jquery ui sortable only prevents the dragging from working, without actually not running. It still does all the expensive calls to the refreshPositions function.

All I need is to hold off before the start if the disabled option is set. My change is only about 7 lines - I wrapped the executed code into a "main" function, and only call that function if disabled is not set. The diff below is misleading.

I really hope this, or a different solution, can make it into the main rep. Thanks!

[thgreasi]

Are the tests passing in your test environment?
The tests of the current v0.14 branch are failing on Travis when using PhatnomJS but pass 100% on Firefox.
I should first try to fix that, but this PR also has 66 failing tests on FF.

[thgreasi]

Found a workaround for the phantomJS issue.
As noted in angular/angular.js#13794 I will try using
https://github.com/tom-james-watson/phantomjs-polyfill for
Function.prototype.bind.

Thodoris Greasidis
Computer, Networking &
Communications Engineer

[michaschwab]

Oh - they fail for me, too. I'm sorry, I should have checked.

This is caused by calling element.sortable from within a callback function. Doing this works fine:
if(!scope.uiSortable || !scope.uiSortable.disabled) { element.sortable(opts); }

But this way it can't be disabled first, and enabled after.

Even though the problem is narrowed down this much, I'm not sure what it is. Usually problems like this arise from having the wrong context inside the function that is called. I tried calling it using element.sortable.call(context, opts) with various context but it seems to already have the correct one by default.

Help very much appreciated. Thank you for taking the time!

[thgreasi]

We should have this code as a function and invoke it right before the
watcher initialization. Also, it would be great if you cancelled the
watcher after the plugin gets enabled, using the value returned by the
$watch method.

Thodoris Greasidis
Computer, Networking &
Communications Engineer

[michaschwab]

sure, see if this is better.

[thgreasi]

That's exactly what I meant, great!
I will merge it as soon as I manage to fix the test cases.
As a memory efficiency extra, consider using angular.noop instead of a
newly created empty function, or test if the watcher handler has an
assigned value before invoking it.
Also it would be great to use a more semantic name for the variable holding
the reference of the watcher cancel method.

Thodoris Greasidis
Computer, Networking &
Communications Engineer

[michaschwab]

I miss working in a team 8-)
Thanks a lot for fixing the problems.

[thgreasi]

@michaschwab Earlier today I pushed some commits to v0.14 branch and the test are now passing.
I tried doing a quick merge with your branch (locally, no conflicts occurred) and some tests failed.
It would be great if you could rebase your code and help me debug the issues.

[thgreasi]

Merged in v0.14.x-dev branch with #444.
@michaschwab 👍 a great thanks for your awesome idea & work!
Waiting for your feedback on the final code and whether it actually works for your use case.

[michaschwab]

Hi! Sorry for not getting back to you earlier.

Thanks so much, it's almost perfect! For me, calling watchOnce('uiSortable.disabled', lazyInit); in lazyInit, and watchOnce calling fn.apply(this, arguments); caused an endless loop, because $watch is always called at least once for me.

After making a slight alteration to the watchOnce function it is perfect for me. I simply checked if the watched expression is actually different:

var lastVal; function watchOnce (watchExpr, fn) { var cancelWatcher = scope.$watch(watchExpr, function (newVal) { if(newVal !== lastVal) { lastVal = newVal; fn.apply(this, arguments); cancelWatcher(); } }); }

Please let me know what you think, and if you can manage to put that in by tomorrow. I have a user study lined up tomorrow and having this then would mean a smoother experience. But I can hack it up for the demo if needed, so no pressure.

Thanks again!

[thgreasi]

I was thinking about adding such a check in the $watch. How about using the
second argument of the $watch for the comparison instead of the newly
declared lastVal variable?
function (newVal, oldVal) {
if(newVal !== oldVal) {

Thodoris Greasidis
Computer, Networking &
Communications Engineer

[thgreasi]

It seems that the related tests are failing either way. I will give it a

try.

Thodoris Greasidis
Computer, Networking &
Communications Engineer

[thgreasi]

At last! I managed to reproduce the infinite digest loop problem and
created two new test cases that use $timeout to change the sortable options
object. Perhaps this should be considered the primary use case.

Thodoris Greasidis
Computer, Networking &
Communications Engineer

[thgreasi]

Just pushed some commits with fixes... Actually reverted the watchOnce
implementation since I couldn't make it work for the four related test
cases in the same time.

Can you test it on your use case?

Thodoris Greasidis
Computer, Networking &
Communications Engineer

[michaschwab]

Thanks so much for working so hard on this. It works!! Haven't tested extensively yet, but the delayed launch definitely works. Great work!! I'll do more testing later to see if anything else might be affected, but everything is looking perfect.

[thgreasi]

Great! Nice to hear that.

[thgreasi]

@michaschwab I just pushed some extra commits to the v0.14 branch.
If your tests were successful and have a possitive feedback, we could publish v0.14 as the new stable release.

[michaschwab]

Thanks! Everything is looking great now. It's working well, including dragging across hierarchies, and only starts if not disabled. Perfect! 0.14 as stable sounds good to me.

[thgreasi]

Ok great! Thanks for your contribution to this repo!

Thodoris Greasidis
Computer, Networking &
Communications Engineer