Don't call ngf-change with an empty files parameter #764

Closed
lookfirst opened this Issue May 27, 2015 · 19 comments

Projects

None yet

4 participants

@lookfirst
Contributor

@danialfarid What I'm seeing is that ngf-change is being called TWICE. Once when you click the button and the file chooser dialog opens and once when the dialog closes after selecting a file.

I added this to your fiddle...

    $scope.upload = function (files) {
        alert('called from upload. files is: ' + files);

Whatever is calling it shouldn't call it the first time. There is no reason to call $scope.upload if files is null. This would allow me to remove the null check and simplify things greatly.

@pimpin
pimpin commented May 27, 2015

Notice that, when you select over sized files (using the optional ngf-max-size attribute), it also fire ngf-change with an empty $file Array. So it's hard to make difference on controller side between those 2 use cases.
[EDIT] silly me ! To check any over sized files, I just need to test the second argument that is $rejectedFiles

@danialfarid
Owner

I have explained here why it needs to be fired on click: #761 (comment)

@lookfirst
Contributor

There is no reason to call ngf-change with an empty files parameter. Nothing has changed.

@danialfarid
Owner

It would only call it if a file is already selected and it resets the model value to null.

@lookfirst
Contributor

Maybe I need to submit a pull request cause you don't seem to understand. Whatever the expression is in ngf-changed is, which is typically a function... does not need to be called if the parameter that is passed to it is null.

@danialfarid
Owner

If you cancel the popup file select, it should call ngf-change with null value.
I might consider adding an option to not reset the model on file click, but that would not allow you to select the same file again or clear the model when the user cancels the popup.

@lookfirst
Contributor

"If you cancel the popup file select, it should call ngf-change with null value."

No, it shouldn't. A cancel should not modify state. If there was a value there before, it will be cleared out and that is against the previous sentence.

@danialfarid
Owner

Here are related issues:
#527
#451
#499

@lookfirst
Contributor

Right, that just goes to show that your API design has issues.

@danialfarid
Owner

Yea I explained here #761 (comment) that there is no way to detect user's cancel, that's why we need to reset the value on click.

@lookfirst
Contributor

Call another method. pseudo code:

ngf-change="expression($files)" ngf-cancel="cancel()"

if (!$files) {
  cancel()
}
@danialfarid
Owner

I think that makes it more complicated, ngf-change needs to be close to ng-model behaviour, when you cancel the popup ng-model is being set to null so ngf-change should be called too.
I think a null check is easy enough that doesn't require adding another directive attribute to the api.

@lookfirst
Contributor

Right, but the problem is that ngf-change gets called in 3 different situations and for 3 different reasons. Open, Cancel and File Select. That is impossible to case for correctly.

@danialfarid
Owner

So what is your suggestion here?
Instead of ngf-cancel you could just have a check

if (!$files) {}

any other solutions you have in mind?

@danialfarid danialfarid pushed a commit that referenced this issue Jun 4, 2015
Danial Farid Fixed #783 #764 #777 #766 #763 #761 #721 #687 #650 #710 #784 #768 #789 39fa784
@danialfarid
Owner

I have added ngf-reset-on-click and ngf-reset-model-on-click options to version 5.0.0 to control this behaviour.

@danialfarid danialfarid closed this Jun 4, 2015
@lookfirst
Contributor

"Setting ngf-reset-model-on-click will not reset the model when you click on the file select, that would make reseting model when the user cancels the select popup impossible in some browsers."

Your defaults are conceived in a backwards fashion. By default, you don't want to reset the model on cancel. It is a cancel event, nothing should change.

@danialfarid
Owner

Setting it to true by default will break cross browser support.
By default it should behave the same cross browser.

@lookfirst
Contributor

Oh right. The option that you'd had to close 20 issues over is a much better default.

On Jun 4, 2015, at 11:31 AM, Danial Farid notifications@github.com wrote:

Setting it to true by default will break cross browser support.
By default it should behave the same cross browser.


Reply to this email directly or view it on GitHub.

@v29neil
v29neil commented Jun 12, 2015

Had the same issue-
Fixed it by using. Not sure if this is the correct way
<button ngf-select="" ng-model="files" ngf-multiple="false" >Upload </button>

In controller-
$scope.$watch('files', function (files) { console.log(files); if( (files != null) ){ console.log('file uploading'); } });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment