socia-platform / htwplus

The social network and new eLearning system for HTW-Berlin.
http://plus.htw-berlin.de
MIT License
14 stars 8 forks source link

Media List Refresh after Upload #154

Closed fabiankirstein closed 8 years ago

fabiankirstein commented 8 years ago

The media list is now updated automatically when the upload queue is completed.

fabiankirstein commented 8 years ago

@IvenZ Can you please review and try this in detail?

IvenZ commented 8 years ago

works like a charm. two things:

  1. the refresh takes some time. don't know if it's only on my side or always the case. does it make sense to "disable" the list and enable it again when the refresh is finished?
  2. there is no error handling. if you upload a new file (maybe one that is too big) an empty list appears.
fabiankirstein commented 8 years ago
  1. It takes some time? Not for me. It only refreshes when all files are processed. Maybe that's why it seems like it takes a while.
  2. I will check it again.
IvenZ commented 8 years ago

123

fabiankirstein commented 8 years ago

That is strange. I will check tonight. It wasn't like this for me.

fabiankirstein commented 8 years ago

But great review process. :)

fabiankirstein commented 8 years ago

I fixed the obvious bugs. But the refresh time is strange. I could reproduce it. Takes about a second on my machine. If I call the /media/list/ route directly it takes the usual 20ms. I discovered it has something to do with the creation of the media object right before the AJAX call. Somehow this is slowing down the request. Can you please verify this for me? Just comment out lines 247 and 248 (https://github.com/socia-platform/htwplus/blob/media_upload/app/controllers/MediaController.java#L247) It will be very fast then. So one request influences the next one.... And I don't understand what you mean with an "empty list" appears.

IvenZ commented 8 years ago

Yes, you are right. I followed the code to this point (https://github.com/socia-platform/htwplus/blob/media_upload/app/managers/MediaManager.java#L147). Here it gets slow.

https://github.com/socia-platform/htwplus/blob/media_upload/app/controllers/MediaController.java#L253 Why do you cut off the file.id? I need it for markdown (drag&drop) link creation. Is there a better way?

empty list issue:

before

after

fabiankirstein commented 8 years ago

I deleted the id for testing. Sorry. I will fix it. I understand that this file operation is slow, but it shouldn't influence the next request. Maybe it's because the local development environment runs in one thread. We should deploy it when everything works and see how it behaves live. Otherwise we have to put the file operations into a separate thread.

IvenZ commented 8 years ago

:+1:

fabiankirstein commented 8 years ago

@IvenZ Can you check again and if okay deploy it?

IvenZ commented 8 years ago

jquery.js:1471 Uncaught Error: Syntax error, unrecognized expression: /media/200?action=download on a single download (but it gets downloaded)

filesize within the table isn't rendered after uploading

fabiankirstein commented 8 years ago

Both fixed! The JavaScript error is also present in master.

IvenZ commented 8 years ago

:+1: will be deployed on sunday

fabiankirstein commented 8 years ago

The list refresh is much faster live. Can you verify this?

IvenZ commented 8 years ago

yep