External Storage UI Improvements

[MTGap]

Multiple improvements for the external storage UI

• Status icons show if connection to the external storage is successful (Green - good, red - error, gray - waiting for configuration)
• Saves on paste events and 2 seconds after typing is finished
• External storage backends throw exceptions in constructor to prevent mounting if bad configuration
• Fix javascript bug with admin external storage UI

@jancborchardt Could you provide some pretty status icons and some UI feedback?

@icewind1991 I can't figure out how to test if the configuration for SMB is good or not, it seems to return something strange for stat()

Please review @karlitschek @MTRichards

[icewind1991]

I don't the constructor is the best place to put the configuration test, adding additional "cost" to the constructor will slow down every request that the store backend is used.

I think it's better to add a new function for that to the backends

[MTGap]

It shouldn't be every call to the backend. Just when it is created in createStorage, after that it is cached in the $storages variable.

[icewind1991]

Yes, but that is still an additional request over the network each time the page is loaded

[MTGap]

Well it should only be an additional request when that storage is needed.

I don't know. I think it's better to be safe and make sure that the storage is working.

[icewind1991]

The storage backend is used every page load though for checking if the storage has been updated, if the storage backend is broken it should throw the exception when the request already being made fails.

For the configuration feedback a simple test() function is enough, no need to add a potentially large delay every time a storage backend is created.

[MTGap]

Okay, I wasn't thinking of that. I'll move them out.

Do you still agree that there should be an exception thrown in the constructor if the correct parameters are not found?

[icewind1991]

Yes, if we can detect an incorrect configuration without any overhead there is no reason not to throw an exception there

Michael Gapczynski notifications@github.com wrote:

Okay, I wasn't thinking of that. I'll move them out.

Do you still agree that there should be an exception thrown in the constructor if the correct parameters are not found?

—
Reply to this email directly or view it on GitHub.



[MTGap]

I think this is ready. Could some people start testing in both the personal and admin settings?

@jancborchardt I could still use some UI feedback.

[jancborchardt]

@MTGap which status icons do you exactly need? Maybe it’s better if Achim makes them, as he also did the status icons for the desktop client.

Regarding the UI feedback, a screenshot would help a lot as I’m a bit busy with the new design. :)

[JFOC]

the icon looks good but sometimes its slow change from red to green even already in storage settings and status its granted, so it looks takes 5-10 secs before changed from red to green

[MTGap]

@jancborchardt It would be helpful to understand by trying out yourself. I didn't realize there was secret design stuff. Here's some screenshots:

I don't know Achim. How do I contact him.

@JFOC Not much I can do because it depends on your server and the external storage to respond. I'm not causing it to have any delay.

[icewind1991]

@MTGap because the external storage might take some time to react, it's probably best to change the status to gray while waiting for the storage

[JFOC]

I dont know how the icons state are changed based on access granted or other things.

[MTGap]

@icewind1991 Yeah I'll probably do that for Dropbox and Google Drive.

[DeepDiver1975]

@karlitschek Can this still be merged into master at this time?

I'd strongly advise to do so - it's a great enhancement to the ui and will save the user and use trouble

[jancborchardt]

Agree with @DeepDiver1975 on everything: adding a spinner, icon reset back
to grey, and advising to merge into master still as soon as aforementioned
changes are done (including the string changes).

On Tue, Feb 12, 2013 at 4:14 PM, Thomas Müller notifications@github.comwrote:

@karlitschek https://github.com/karlitschek Can this still be merged
into master at this time?

I'd strongly advise to do so - it's a great enhancement to the ui and will
save the user and use trouble

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1031#issuecomment-13436796.

[karlitschek]

Yes. Let's merge it. But as soon as possible.
And from now on we are really in complete feature freeze. ;-)

[MTGap]

I don't have time to do the spinner changes, but the string changes are already in here.

[DeepDiver1975]

I can add the spinner later - give me time until 00:00 or we close it now.

[DeepDiver1975]

@MTGap @jancborchardt @karlitschek spinner in place

[karlitschek]

Nice. 👍

[MTGap]

Holding off on merge until issues are resolved.

[karlitschek]

So ready for merge? ;-)

[DeepDiver1975]

No - we detected some issue last night. I'd vote for shipping OC5 Alpha without this.

[jancborchardt]

In IE8 the field labels aren’t visible because the placeholder attribute is unsupported. Should use infield labels, like the log in form for instance.

[MTGap]

@butonic The switch from jQuery live to on broke the UI in this pull request. Could you investigate, I'm not sure how to resolve the problems.

[butonic]

UI fixed with 05a8766, please review.

@MTGap I just tried to test this by adding my production oc as an external mount point. @jancborchardt The following is a documentation of what I did. Maybe it helps find usability issues:

I used /Music as the mount point, chose ownCloud / WebDAV as the type, enterd the url and my credentials and used / as root ... did not really know what to put in that field.
Unfortunately, nothing happened in the UI and when navigating to the files app I was greeted with a 500 Internal Server Error because of this:
PHP Fatal error: Call to undefined function curl_init() in /home/jfd/Repositories/oc/3rdparty/Sabre/DAV/Client.php on line 441, referer: http://localhost/core/index.php/settings/personal

After installing php5-curl the fatal error vanishes but now I am mocked by a white page when navigation into the folder. 😡

Going back to the personal settings shows a green bulb. Navigating back to /Music in the files app now shows me a spinner but after a while I can see my personal owncloud files. nice!

Let's try mounting the owncloud demo to demo ... long story short. I sporadically receive a white page and sometimes it works.

Nothing in the log files. @icewind1991 With DEBUG defined I get notifications:

PHP Notice:  Undefined index: mtime in /home/jfd/Repositories/oc/core/lib/files/storage/common.php on line 73, referer: http://localhost/core/index.php/apps/files?dir=/Music
PHP   1. {main}() /home/jfd/Repositories/oc/core/index.php:0, referer: http://localhost/core/index.php/apps/files?dir=/Music
PHP   2. OC::handleRequest() /home/jfd/Repositories/oc/core/index.php:28, referer: http://localhost/core/index.php/apps/files?dir=/Music
PHP   3. OC_Router->match() /home/jfd/Repositories/oc/core/lib/base.php:586, referer: http://localhost/core/index.php/apps/files?dir=/Music
PHP   4. call_user_func() /home/jfd/Repositories/oc/core/lib/router.php:127, referer: http://localhost/core/index.php/apps/files?dir=/Music
PHP   5. OC::loadAppScriptFile() /home/jfd/Repositories/oc/core/lib/router.php:127, referer: http://localhost/core/index.php/apps/files?dir=/Music
PHP   6. require_once() /home/jfd/Repositories/oc/core/lib/base.php:652, referer: http://localhost/core/index.php/apps/files?dir=/Music
PHP   7. OC\\Files\\Cache\\Scanner->backgroundScan() /home/jfd/Repositories/oc/core/apps/files/ajax/scan.php:29, referer: http://localhost/core/index.php/apps/files?dir=/Music
PHP   8. OC\\Files\\Cache\\Scanner->scan() /home/jfd/Repositories/oc/core/lib/files/cache/scanner.php:165, referer: http://localhost/core/index.php/apps/files?dir=/Music
PHP   9. OC\\Files\\Cache\\Scanner->scanFile() /home/jfd/Repositories/oc/core/lib/files/cache/scanner.php:111, referer: http://localhost/core/index.php/apps/files?dir=/Music
PHP  10. OC\\Files\\Cache\\Scanner->getData() /home/jfd/Repositories/oc/core/lib/files/cache/scanner.php:66, referer: http://localhost/core/index.php/apps/files?dir=/Music
PHP  11. OC\\Files\\Storage\\Common->filemtime() /home/jfd/Repositories/oc/core/lib/files/cache/scanner.php:47, referer: http://localhost/core/index.php/apps/files?dir=/Music
PHP Notice:  Undefined index: size in /home/jfd/Repositories/oc/core/lib/files/storage/common.php on line 37, referer: http://localhost/core/index.php/apps/files?dir=/Music
[... same trace 1-10 as above ...]
PHP  11. OC\\Files\\Storage\\Common->filesize() /home/jfd/Repositories/oc/core/lib/files/cache/scanner.php:51, referer: http://localhost/core/index.php/apps/files?dir=/Music

.... hm not sure If PEBKAC or real issue ...

[MTGap]

@icewind1991 Can you take a look at the WebDAV errors? I think this is ready for merge.

@karlitschek @DeepDiver1975

[icewind1991]

@butonic for the root you need to fill in the location of the webdav endpoint on the server, so /remote.php/webdav for the demo.

#1906 will fix the issue where a misconfigured storage backend will create a whole bunch of errors

[leadmocha]

👍
Makes everything work for me!

[karlitschek]

So ready for merge now? Today is really the absolute deadline and I would love to have this in. Finally ;-)