Upgrade to Marionette 3

[ChristophWurst]

TODO:

• empty content view was apparently removed, hence empty folders are missing the approriate icon and text
• folders can be expanded/collapsed even if they don't have sub-folders
• scroll events to refresh and load more are broken

[ChristophWurst]

Basic functionality seems to work 🚀

@Gomez @nextcloud/mail review please, so we can base the next PRs on the new Marionette features :-)

[ChristophWurst]

@jancborchardt@tahaalibra@skjnldsv@Gomez would be great to get this in, so I can continue with the other PRs and use the new Mariontte APIs. Thanks :-)

[ChristophWurst]

Btw. the client should be a bit faster now that we use onRender everywhere and the views are being generated in memory. In contrast, we've used onShow extensively before, which operated on DOM and therefore caused slower view rendering. 🚀

[jancborchardt]

Only shows spinners and I get this in the console:

JQMIGRATE: Migrate is installed, version 1.4.0
require.js?v=366fed6…:1958 GET http://localhorst/nextcloud/apps/mail/js/notification.js req.load @ require.js?v=366fed6…:1958load @ require.js?v=366fed6…:1682load @ require.js?v=366fed6…:832fetch @ require.js?v=366fed6…:822check @ require.js?v=366fed6…:854enable @ require.js?v=366fed6…:1173enable @ require.js?v=366fed6…:1554(anonymous function) @ require.js?v=366fed6…:1158(anonymous function) @ require.js?v=366fed6…:134each @ require.js?v=366fed6…:59enable @ require.js?v=366fed6…:1110init @ require.js?v=366fed6…:786(anonymous function) @ require.js?v=366fed6…:1457
require.js?v=366fed6…:168 Uncaught Error: Script error for "notification"
http://requirejs.org/docs/errors.html#scripterrormakeError @ require.js?v=366fed6…:168onScriptError @ require.js?v=366fed6…:1735
appview.js:114 Uncaught TypeError: this.navigation.showChildView is not a function

[ChristophWurst]

Ah, that's an error since #115, the fix for that is in #125.

appview.js:114 Uncaught TypeError: this.navigation.showChildView is not a function

That indicates that you did not actually pull the new Marionette version. Run bower install or the makefile to install it ;-)

[jancborchardt]

Ahh, thanks! Of course that has to be said for testers ;)
Some issues:

• loading by scrolling down doesn’t seem to work or is quite janky
• while loading messages, the list sometimes lags
• when reading and then deleting the top message, the bottom one (in this case the 20th) is opened. Instead deleting the first one when reading that should open the second one (now first one)

[ChristophWurst]

Of course that has to be said for testers

I thought german computer programmers would figure that out themselves ;-)

[ChristophWurst]

@jancborchardt regarding the first two issues it looks like Marionette is re-rendering the messages list for each element added to the collection, therefore the browser feels janky shortly.
To fix that we'll have to add messages to the collection in bulk, which will conflict with #127. So it would be great if you could review that other PR and then I'll rebase, fix and finish this one :-)

[ChristophWurst]

• TODO: view's this.ui.… was replaced by this.getUI('…')http://marionettejs.com/docs/master/marionette.view.html#accessing-ui-elements

[ChristophWurst]

Might be worth to directly migrate to marionettejs/backbone.marionette#3192 🚀

[ChristophWurst]

• BUG: load more causes the collection view to re-render and FF scrolls up all the way, which causes a refresh of the message list. Moreover, the load more spinner is not shown.

[jancborchardt]

Again only spinners (even after making, and also in incognito window), and this error:

require.js?v=cf07c3a…:1958 Refused to load the script 'http://localhorst/nextcloud/apps/mail/js/init.js' because it violates the following Content Security Policy directive: "script-src 'nonce-Rm5JQUJRVXVGQnd6SlRjUEEyWXBHQkpoUDM0ck9URUxDRWNHT2pkZkJTMD06UDVkQzRPWEtYR2VLdVFwd0E0bEdPaXRsWnJldmVpNFlDNDg5aHhxK0hNOD0=' 'unsafe-eval'".
req.load @ require.js?v=cf07c3a…:1958
require.js?v=cf07c3a…:143 Uncaught Error: Script error for "init"
http://requirejs.org/docs/errors.html#scripterror

(This is with Chrome.)

[ChristophWurst]

there was a fix for that in #169. I need to rebase this PR

[ChristophWurst]

fixed the load-more spinner. Only issue left: FF jump scrolls when we add more messages to the list. This might be an issue that we had before already: FF always scrolled up when the message list was refreshed, whereas Chrome didn't.

[ChristophWurst]

found a quick and dirty fix for the FF issue: remember and restore the scroll position when the list is re-rendered. Seems to work, although there might be a better solution.

@jancborchardt please give it another test run and let me know there's anything else that does not work

[jancborchardt]

Seems good with Chrome at least 👍 couldn’t test with Firefox yet.

[jancborchardt]

So @ChristophWurst I would say let’s get this in and fix things moving forward so it doesn’t diverge more. :)

[ChristophWurst]

Agreed. Let me fix the code style issues and then we can merge 🙏

[ChristophWurst]

🚀 💃 🎆 🍾 it's in!

[jancborchardt]

@ChristophWurst btw don’t remember if it’s related, but replying to emails from the unified inbox doesn’t put the »reply« icon ;)

[ChristophWurst]

Unified inbox will hopefully die soon. I have found a way to paginate our APIs, which means I can finally re-implement it client side: less code, fewer hacks and definitely better performance. https://github.com/ChristophWurst/multi-source-pagination

[jancborchardt]

Ok, but maybe just say »the current implementation of unified inbox« otherwise I’m afraid. ;D

[ChristophWurst]

hehe, yes, that's what I meant ;-)

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and questions.