Don't scroll on m.route.set() to different post on same page.

This removes some messy logic, and the potential for glitches. This system worked well with Mithril 0.2 where we could listen in before (and prevent) page unload, but since that's not possible in Mithril 2, the implementation of the replacement was done in `onbeforeupdate`, which might be called while the page route is being updated, glitching out the page. Instead, extensions should check if they are already on the discussion page for the post they are linking to, and if so, use `app.current.get('stream').goToNumber(TARGET)`.

Please note that this does NOT affect going directly to posts from external links (or page reload), OR from other pages via m.route.set.

js/src/common/Application.js

@@ -272,7 +272,7 @@ export default class Application {
   /**
    * Make an AJAX request, handling any low-level errors that may occur.
    *
-   * @see https://lhorie.github.io/mithril/mithril.request.html
+   * @see https://mithril.js.org/request.html
    * @param {Object} options
    * @return {Promise}
    * @public

js/src/common/components/Page.js

@@ -12,9 +12,6 @@ export default class Page extends Component {
 
     this.onNewRoute();
 
-    app.drawer.hide();
-    app.modal.close();
-
     /**
      * A class name to apply to the body while the route is active.
      *
@@ -32,6 +29,9 @@ export default class Page extends Component {
   onNewRoute() {
     app.previous = app.current;
     app.current = new PageState(this.constructor, { routeName: this.attrs.routeName });
+
+    app.drawer.hide();
+    app.modal.close();
   }
 
   oncreate(vnode) {

js/src/common/helpers/icon.js

@@ -1,12 +0,0 @@
-/**
- * The `icon` helper displays an icon.
- *
- * @param {String} fontClass The full icon class, prefix and the icon’s name.
- * @param {Object} attrs Any other attributes to apply.
- * @return {Object}
- */
-export default function icon(fontClass, attrs = {}) {
-  attrs.className = 'icon ' + fontClass + ' ' + (attrs.className || '');
-
-  return <i {...attrs} />;
-}

js/src/common/helpers/icon.tsx

@@ -0,0 +1,13 @@
+import * as Mithril from 'mithril';
+
+/**
+ * The `icon` helper displays an icon.
+ *
+ * @param fontClass The full icon class, prefix and the icon’s name.
+ * @param attrs Any other attributes to apply.
+ */
+export default function icon(fontClass: string, attrs: Mithril.Attributes = {}): Mithril.Vnode {
+  attrs.className = 'icon ' + fontClass + ' ' + (attrs.className || '');
+
+  return <i {...attrs} />;
+}

js/src/common/utils/SubtreeRetainer.js

@@ -28,6 +28,9 @@ export default class SubtreeRetainer {
   constructor(...callbacks) {
     this.callbacks = callbacks;
     this.data = {};
+    // Build the initial data, so it is available when calling
+    // needsRebuild from the onbeforeupdate hook.
+    this.needsRebuild();
   }
 
   /**
@@ -60,6 +63,8 @@ export default class SubtreeRetainer {
    */
   check(...callbacks) {
     this.callbacks = this.callbacks.concat(callbacks);
+    // Update the data cache when new checks are added.
+    this.needsRebuild();
   }
 
   /**

js/src/forum/components/DiscussionPage.js

@@ -52,6 +52,7 @@ export default class DiscussionPage extends Page {
   }
 
   onremove() {
+    super.onremove();
     // If we are indeed navigating away from this discussion, then disable the
     // discussion list pane. Also, if we're composing a reply to this
     // discussion, minimize the composer – unless it's empty, in which case
@@ -98,28 +99,12 @@ export default class DiscussionPage extends Page {
   onbeforeupdate(vnode) {
     super.onbeforeupdate(vnode);
 
-    if (m.route.get() !== this.prevRoute) {
+    const idParam = m.route.param('id');
-      this.onNewRoute();
+    if (m.route.get() !== this.prevRoute && this.discussion && (!idParam || idParam.split('-')[0] !== this.discussion.id())) {
       this.prevRoute = m.route.get();
 
-      // If we have routed to the same discussion as we were viewing previously,
+      this.onNewRoute();
-      // cancel the unloading of this controller and instead prompt the post
+      this.oninit(vnode);
-      // stream to jump to the new 'near' param.
-      if (this.discussion) {
-        const idParam = m.route.param('id');
-
-        if (idParam && idParam.split('-')[0] === this.discussion.id()) {
-          const near = m.route.param('near') || '1';
-
-          if (near !== String(this.near)) {
-            this.stream.goToNumber(near);
-          }
-
-          this.near = near;
-        } else {
-          this.oninit(vnode);
-        }
-      }
     }
   }
 
@@ -189,15 +174,19 @@ export default class DiscussionPage extends Page {
         .slice(0, 20);
     }
 
+    const startNumber = m.route.param('near') || (includedPosts[0] && includedPosts[0].number());
+
     // Set up the post stream for this discussion, along with the first page of
     // posts we want to display. Tell the stream to scroll down and highlight
     // the specific post that was routed to.
     this.stream = new PostStreamState(discussion, includedPosts);
-    this.stream.goToNumber(m.route.param('near') || (includedPosts[0] && includedPosts[0].number()), true).then(() => {
+    this.stream.goToNumber(startNumber, true).then(() => {
       this.discussion = discussion;
 
       app.current.set('discussion', discussion);
       app.current.set('stream', this.stream);
+
+      this.positionChanged(startNumber);
     });
   }
 

js/src/forum/components/PostStream.js

@@ -359,9 +359,6 @@ export default class PostStream extends Component {
 
     return Promise.all([$container.promise(), this.stream.loadPromise]).then(() => {
       m.redraw.sync();
-      // We want to adjust this again after posts have been loaded in so that
-      // the height of the scrubber is accurate.
-      updateScrubberHeight();
 
       // After post data has been loaded in, we will attempt to scroll back
       // to the top of the requested post (or to the top of the page if the
@@ -377,6 +374,10 @@ export default class PostStream extends Component {
         $(window).scrollTop($(`.PostStream-item[data-index=${index}]`).offset().top - this.getMarginTop());
       }
 
+      // We want to adjust this again after posts have been loaded in
+      // and position adjusted so that the scrubber's height is accurate.
+      updateScrubberHeight();
+
       this.calculatePosition();
       this.stream.paused = false;
     });

js/src/forum/components/ReplyPlaceholder.js

@@ -33,7 +33,7 @@ export default class ReplyPlaceholder extends Component {
     }
 
     const reply = () => {
-      DiscussionControls.replyAction.call(this.attrs.discussion, true);
+      DiscussionControls.replyAction.call(this.attrs.discussion, true).catch(() => {});
     };
 
     return (

src/Database/AbstractModel.php

@@ -82,7 +82,7 @@ abstract class AbstractModel extends Eloquent
         }
 
         $this->attributes = array_map(function ($item) {
-            return is_callable($item) ? $item() : $item;
+            return is_callable($item) ? $item($this) : $item;
         }, $this->attributes);
 
         parent::__construct($attributes);

src/User/Event/GetDisplayName.php

@@ -12,7 +12,7 @@ namespace Flarum\User\Event;
 use Flarum\User\User;
 
 /**
- * @deprecated beta 14, removed beta 15.
+ * @deprecated beta 14, remove in beta 15.
  */
 class GetDisplayName
 {

src/User/User.php

@@ -722,7 +722,7 @@ class User extends AbstractModel
             $groupIds = array_merge($groupIds, [Group::MEMBER_ID], $this->groups->pluck('id')->all());
         }
 
-        // Deprecated, remove in beta 14.
+        /** @deprecated in beta 14, remove in beta 15 */
         event(new PrepareUserGroups($this, $groupIds));
 
         foreach (static::$groupProcessors as $processor) {

src/User/UserValidator.php

@@ -51,7 +51,7 @@ class UserValidator extends AbstractValidator
             ],
             'email' => [
                 'required',
-                'email',
+                'email:filter',
                 'unique:users,email'.$idSuffix
             ],
             'password' => [

tests/integration/extenders/EventTest.php

@@ -16,7 +16,7 @@ use Flarum\Tests\integration\RetrievesAuthorizedUsers;
 use Flarum\Tests\integration\TestCase;
 use Flarum\User\User;
 use Illuminate\Contracts\Bus\Dispatcher;
-use Illuminate\Contracts\Translation\Translator;
+use Symfony\Component\Translation\TranslatorInterface;
 
 class EventTest extends TestCase
 {
@@ -87,7 +87,7 @@ class CustomListener
 {
     protected $translator;
 
-    public function __construct(Translator $translator)
+    public function __construct(TranslatorInterface $translator)
     {
         $this->translator = $translator;
     }

tests/integration/extenders/ModelTest.php

@@ -277,7 +277,7 @@ class ModelTest extends TestCase
     {
         $this->extend(
             (new Extend\Model(Group::class))
-                ->default('counter', function () {
+                ->default('counter', function (Group $group) {
                     static $counter = 0;
 
                     return ++$counter;