Show Gaps in the post stream when filters are active

Conflicts:
	app/assets/javascripts/discourse/templates/topic.js.handlebars

app/assets/javascripts/discourse/components/post_gap_component.js

@@ -0,0 +1,48 @@
+/**
+  Handles a gap between posts with a click to load more
+
+  @class PostGapComponent
+  @extends Ember.Component
+  @namespace Discourse
+  @module Discourse
+**/
+Discourse.PostGapComponent = Ember.Component.extend({
+  classNameBindings: [':gap', 'gap::hidden'],
+
+  init: function() {
+    this._super();
+    this.set('loading', false);
+
+    var before = this.get('before') === 'true',
+        gaps = before ? this.get('postStream.gaps.before') : this.get('postStream.gaps.after');
+
+    if (gaps) {
+      this.set('gap', gaps[this.get('post.id')]);
+    }
+  },
+
+  render: function(buffer) {
+    if (this.get('loading')) {
+      buffer.push(I18n.t('loading'));
+    } else {
+      buffer.push("<i class='icon icon-cut'></i>" + I18n.t('post.gap', {count: this.get('gap.length')}));
+    }
+  },
+
+  click: function() {
+    if (this.get('loading') || (!this.get('gap'))) { return false; }
+    this.set('loading', true);
+    this.rerender();
+
+    var self = this,
+        postStream = this.get('postStream'),
+        filler = this.get('before') === 'true' ? postStream.fillGapBefore : postStream.fillGapAfter;
+
+    filler.call(postStream, this.get('post'), this.get('gap')).then(function() {
+      // hide this control after the promise is resolved
+      self.set('gap', null);
+    });
+
+    return false;
+  }
+});

app/assets/javascripts/discourse/controllers/topic_controller.js

@@ -500,6 +500,12 @@ Discourse.TopicController = Discourse.ObjectController.extend(Discourse.Selected
     }
   },
 
+  /**
+    Called the the topmost visible post on the page changes.
+
+    @method topVisibleChanged
+    @params {Discourse.Post} post that is at the top
+  **/
   topVisibleChanged: function(post) {
     var postStream = this.get('postStream'),
         firstLoadedPost = postStream.get('firstLoadedPost');
@@ -523,11 +529,18 @@ Discourse.TopicController = Discourse.ObjectController.extend(Discourse.Selected
     }
   },
 
-  bottomVisibleChanged: function(post) {
-    this.set('progressPosition', post.get('post_number'));
+  /**
+    Called the the bottommost visible post on the page changes.
 
+    @method bottomVisibleChanged
+    @params {Discourse.Post} post that is at the bottom
+  **/
+  bottomVisibleChanged: function(post) {
     var postStream = this.get('postStream'),
-        lastLoadedPost = postStream.get('lastLoadedPost');
+        lastLoadedPost = postStream.get('lastLoadedPost'),
+        index = postStream.get('stream').indexOf(post.get('id'))+1;
+
+    this.set('progressPosition', index);
 
     if (lastLoadedPost && lastLoadedPost === post) {
       postStream.appendMore();

app/assets/javascripts/discourse/models/post_stream.js

@@ -126,31 +126,11 @@ Discourse.PostStream = Em.Object.extend({
     return result;
   }.property('userFilters.[]', 'summary'),
 
-  /**
-    The text describing the current filters. For display in the pop up at the bottom of the
-    screen.
-
-    @property filterDesc
-  **/
-  filterDesc: function() {
+  hasNoFilters: function() {
     var streamFilters = this.get('streamFilters');
-
-    if (streamFilters.filter && streamFilters.filter === "summary") {
-      return I18n.t("topic.filters.summary", {
-        n_summarized_posts: I18n.t("topic.filters.n_summarized_posts", { count: this.get('filteredPostsCount') }),
-        of_n_posts: I18n.t("topic.filters.of_n_posts", { count: this.get('topic.posts_count') })
-      });
-    } else if (streamFilters.username_filters) {
-      return I18n.t("topic.filters.user", {
-        n_posts: I18n.t("topic.filters.n_posts", { count: this.get('filteredPostsCount') }),
-        by_n_users: I18n.t("topic.filters.by_n_users", { count: streamFilters.username_filters.length })
-      });
-    }
-    return "";
+    return !(streamFilters && ((streamFilters.filter === 'summary') || streamFilters.userFilters));
   }.property('streamFilters.[]', 'topic.posts_count', 'posts.length'),
 
-  hasNoFilters: Em.computed.empty('filterDesc'),
-
   /**
     Returns the window of posts above the current set in the stream, bound to the top of the stream.
     This is the collection we'll ask for when scrolling upwards.
@@ -274,6 +254,66 @@ Discourse.PostStream = Em.Object.extend({
   },
   hasLoadedData: Em.computed.and('hasPosts', 'hasStream'),
 
+
+  /**
+    Fill in a gap of posts before a particular post
+
+    @method fillGapBefore
+    @paaram {Discourse.Post} post beside gap
+    @paaram {Array} gap array of post ids to load
+    @returns {Ember.Deferred} a promise that's resolved when the posts have been added.
+  **/
+  fillGapBefore: function(post, gap) {
+    var postId = post.get('id'),
+        stream = this.get('stream'),
+        idx = stream.indexOf(postId),
+        currentPosts = this.get('posts'),
+        self = this;
+
+    if (idx !== -1) {
+      // Insert the gap at the appropriate place
+      stream.splice.apply(stream, [idx, 0].concat(gap));
+      stream.enumerableContentDidChange();
+
+      var postIdx = currentPosts.indexOf(post);
+      if (postIdx !== -1) {
+        return this.findPostsByIds(gap).then(function(posts) {
+          posts.forEach(function(p) {
+            var stored = self.storePost(p);
+            if (!currentPosts.contains(stored)) {
+              currentPosts.insertAt(postIdx++, stored);
+            }
+          });
+
+          delete self.get('gaps.before')[postId];
+        });
+      }
+    }
+    return Ember.RSVP.resolve();
+  },
+
+  /**
+    Fill in a gap of posts after a particular post
+
+    @method fillGapAfter
+    @paaram {Discourse.Post} post beside gap
+    @paaram {Array} gap array of post ids to load
+    @returns {Ember.Deferred} a promise that's resolved when the posts have been added.
+  **/
+  fillGapAfter: function(post, gap) {
+    var postId = post.get('id'),
+        stream = this.get('stream'),
+        idx = stream.indexOf(postId),
+        currentPosts = this.get('posts'),
+        self = this;
+
+    if (idx !== -1) {
+      stream.pushObjects(gap);
+      return this.appendMore();
+    }
+    return Ember.RSVP.resolve();
+  },
+
   /**
     Appends the next window of posts to the stream. Call it when scrolling downwards.
 
@@ -522,9 +562,9 @@ Discourse.PostStream = Em.Object.extend({
     @method updateFromJson
   **/
   updateFromJson: function(postStreamData) {
-    var postStream = this;
+    var postStream = this,
+        posts = this.get('posts');
 
-    var posts = this.get('posts');
     posts.clear();
     if (postStreamData) {
       // Load posts if present

app/assets/javascripts/discourse/templates/post.js.handlebars

@@ -1,3 +1,5 @@
+{{post-gap post=this postStream=controller.postStream before="true"}}
+
 <div class='row'>
   {{view Discourse.ReplyHistory contentBinding="replyHistory"}}
 </div>
@@ -82,3 +84,5 @@
   </div>
 
 </article>
+
+{{post-gap post=this postStream=controller.postStream before="false"}}

app/assets/javascripts/discourse/templates/topic.js.handlebars

@@ -52,7 +52,7 @@
             <nav id='topic-progress' title="{{i18n topic.progress.title}}" {{bindAttr class="hideProgress:hidden"}}>
               <button id='jump-top' title="{{i18n topic.progress.jump_top}}" {{bindAttr disabled="jumpTopDisabled"}} {{action jumpTop}}><i class="icon-circle-arrow-up"></i></button>
               <div class='nums' {{bindAttr title="progressPositionTitle"}}>
-                <h4>{{progressPosition}}</h4><span {{bindAttr class="hugeNumberOfPosts:hidden"}}> <span>{{i18n of_value}}</span> <h4>{{highest_post_number}}</h4></span>
+                <h4>{{progressPosition}}</h4><span {{bindAttr class="hugeNumberOfPosts:hidden"}}> <span>{{i18n of_value}}</span> <h4>{{postStream.filteredPostsCount}}</h4></span>
               </div>
               <button id='jump-bottom' title="{{i18n topic.progress.jump_bottom}}" {{bindAttr disabled="jumpBottomDisabled"}} {{action jumpBottom}}><i class="icon-circle-arrow-down"></i></button>
               <div class='bg'>&nbsp;</div>
@@ -121,12 +121,6 @@
   {{/if}}
 {{/if}}
 
-
-<div id='topic-filter' {{bindAttr class="postStream.hasNoFilters:hidden"}}>
-  {{postStream.filterDesc}}
-  <a href='#' {{action cancelFilter target="postStream"}}>{{i18n topic.filters.cancel}}</a>
-</div>
-
 {{render share}}
 {{render posterExpansion}}
 

app/assets/stylesheets/desktop/topic-post.scss

@@ -1,6 +1,22 @@
 @import "common/foundation/variables";
 @import "common/foundation/mixins";
 
+.gap {
+  background-color: #f9f9f9;
+  border: 1px solid #eee;
+  padding: 5px 10px;
+  margin-bottom: 10px;
+  color: #555;
+  cursor: pointer;
+
+  &:hover {
+    background-color: #eee;
+  }
+
+  i.icon {
+    margin-right: 6px;
+  }
+}
 
 .container {
   @extend .clearfix;

app/serializers/gap_serializer.rb

@@ -0,0 +1,11 @@
+class GapSerializer < ApplicationSerializer
+  attributes :before, :after
+
+  def before
+    @object.before
+  end
+
+  def after
+    @object.after
+  end
+end

app/serializers/post_stream_serializer_mixin.rb

@@ -1,3 +1,6 @@
+require_dependency 'gap_serializer'
+require_dependency 'post_serializer'
+
 module PostStreamSerializerMixin
 
   def self.included(klass)
@@ -5,17 +8,18 @@ module PostStreamSerializerMixin
   end
 
   def post_stream
-    { posts: posts,
-      stream: object.filtered_post_ids }
+    result = { posts: posts, stream: object.filtered_post_ids }
+    result[:gaps] = GapSerializer.new(object.gaps, root: false) if object.gaps.present?
+    result
   end
 
   def posts
     return @posts if @posts.present?
     @posts = []
-    @highest_number_in_posts = 0
+    highest_number_in_posts = 0
     if object.posts
       object.posts.each_with_index do |p, idx|
-        @highest_number_in_posts = p.post_number if p.post_number > @highest_number_in_posts
+        highest_number_in_posts = p.post_number if p.post_number > highest_number_in_posts
         ps = PostSerializer.new(p, scope: scope, root: false)
         ps.topic_slug = object.topic.slug
         ps.topic_view = object

config/locales/client.en.yml

@@ -815,6 +815,9 @@ en:
         other: "(post withdrawn by author, will be automatically deleted in %{count} hours unless flagged)"
       deleted_by: "deleted by"
       expand_collapse: "expand/collapse"
+      gap:
+        one: "1 post was omitted due to your current filter. Click to show it."
+        other: "{{count}} posts were omitted due to your current filter. Click to show them."
 
       has_replies:
         one: "Reply"

lib/gaps.rb

@@ -0,0 +1,54 @@
+#
+# This is used for finding the gaps between a subset of elements in an array
+# and the original layout. We use this in Discourse to find gaps between posts.
+#
+# Note that we will only return a gap as 'before' or 'after', not both. We only
+# want to display the gap once.
+#
+class Gaps
+
+  attr_reader :before, :after
+
+  def initialize(subset, original)
+    @before = {}
+    @after = {}
+    @subset = subset
+    @original = original
+
+    find_gaps
+  end
+
+  def empty?
+    @before.size == 0 && @after.size == 0
+  end
+
+  def find_gaps
+    return if @subset.nil? or @original.nil?
+
+    i = j = 0
+    gaps = {}
+    current_gap = []
+
+    while
+      e1 = @subset[i]
+      e2 = @original[j]
+
+      if (e1 == e2)
+        if current_gap.size > 0
+          @before[e1] = current_gap.dup
+          current_gap = []
+        end
+
+        i = i + 1
+      else
+        current_gap << e2
+      end
+      j = j + 1
+
+      break if (i == @subset.size) || (j == @original.size)
+    end
+
+    @after[@subset[i-1]] = @original[j..-1] if j < @original.size
+  end
+
+end

lib/topic_view.rb

@@ -2,6 +2,7 @@ require_dependency 'guardian'
 require_dependency 'topic_query'
 require_dependency 'filter_best_posts'
 require_dependency 'summarize'
+require_dependency 'gaps'
 
 class TopicView
 
@@ -44,6 +45,15 @@ class TopicView
     path
   end
 
+  def contains_gaps?
+    @contains_gaps
+  end
+
+  def gaps
+    return unless @contains_gaps
+    Gaps.new(filtered_post_ids, unfiltered_posts.order(:sort_order).pluck(:id))
+  end
+
   def last_post
     return nil if @posts.blank?
     @last_post ||= @posts.last
@@ -113,9 +123,7 @@ class TopicView
 
   # Filter to all posts near a particular post number
   def filter_posts_near(post_number)
-
     min_idx, max_idx = get_minmax_ids(post_number)
-
     filter_posts_in_range(min_idx, max_idx)
   end
 
@@ -255,14 +263,36 @@ class TopicView
     finder.first
   end
 
+  def unfiltered_posts
+    result = @topic.posts.where(hidden: false)
+    result = result.with_deleted if @user.try(:staff?)
+    result
+  end
+
   def setup_filtered_posts
-    @filtered_posts = @topic.posts.where(hidden: false)
+
+    # Certain filters might leave gaps between posts. If that's true, we can return a gap structure
+    @contains_gaps = false
+    @filtered_posts = unfiltered_posts
     @filtered_posts = @filtered_posts.with_deleted if @user.try(:staff?)
-    @filtered_posts = @filtered_posts.summary if @filter == 'summary'
-    @filtered_posts = @filtered_posts.where('posts.post_type <> ?', Post.types[:moderator_action]) if @best.present?
-    return unless @username_filters.present?
-    usernames = @username_filters.map{|u| u.downcase}
-    @filtered_posts = @filtered_posts.where('post_number = 1 or user_id in (select u.id from users u where username_lower in (?))', usernames)
+
+    # Filters
+    if @filter == 'summary'
+      @filtered_posts = @filtered_posts.summary
+      @contains_gaps = true
+    end
+
+    if @best.present?
+      @filtered_posts = @filtered_posts.where('posts.post_type <> ?', Post.types[:moderator_action])
+      @contains_gaps = true
+    end
+
+    if @username_filters.present?
+      usernames = @username_filters.map{|u| u.downcase}
+      @filtered_posts = @filtered_posts.where('post_number = 1 or user_id in (select u.id from users u where username_lower in (?))', usernames)
+      @contains_gaps = true
+    end
+
   end
 
   def check_and_raise_exceptions

spec/components/gaps_spec.rb

@@ -0,0 +1,81 @@
+require 'spec_helper'
+require 'cache'
+
+describe Gaps do
+
+
+  it 'returns no gaps for empty data' do
+    Gaps.new(nil, nil).should be_blank
+  end
+
+  it 'returns no gaps with one element' do
+    Gaps.new([1], [1]).should be_blank
+  end
+
+  it 'returns no gaps when all elements are present' do
+    Gaps.new([1,2,3], [1,2,3]).should be_blank
+  end
+
+  context "single element gap" do
+    let(:gap) { Gaps.new([1,3], [1,2,3]) }
+
+    it 'has a gap for post 3' do
+      gap.should_not be_blank
+      gap.before[3].should == [2]
+      gap.after.should be_blank
+    end
+  end
+
+  context "larger gap" do
+    let(:gap) { Gaps.new([1,2,3,6,7], [1,2,3,4,5,6,7]) }
+
+    it 'has a gap for post 6' do
+      gap.should_not be_blank
+      gap.before[6].should == [4,5]
+      gap.after.should be_blank
+    end
+  end
+
+  context "multiple gaps" do
+    let(:gap) { Gaps.new([1,5,6,7,10], [1,2,3,4,5,6,7,8,9,10]) }
+
+    it 'has both gaps' do
+      gap.should_not be_blank
+      gap.before[5].should == [2,3,4]
+      gap.before[10].should == [8,9]
+      gap.after.should be_blank
+    end
+  end
+
+  context "a gap in the beginning" do
+    let(:gap) { Gaps.new([2,3,4], [1,2,3,4]) }
+
+    it 'has the gap' do
+      gap.should_not be_blank
+      gap.before[2].should == [1]
+      gap.after.should be_blank
+    end
+  end
+
+  context "a gap in the ending" do
+    let(:gap) { Gaps.new([1,2,3], [1,2,3,4]) }
+
+    it 'has the gap' do
+      gap.should_not be_blank
+      gap.before.should be_blank
+      gap.after[3].should == [4]
+    end
+  end
+
+  context "a large gap in the ending" do
+    let(:gap) { Gaps.new([1,2,3], [1,2,3,4,5,6]) }
+
+    it 'has the gap' do
+      gap.should_not be_blank
+      gap.before.should be_blank
+      gap.after[3].should == [4,5,6]
+    end
+  end
+
+
+end

spec/components/topic_view_spec.rb

@@ -89,7 +89,6 @@ describe TopicView do
 
     end
 
-
     it "raises NotLoggedIn if the user isn't logged in and is trying to view a private message" do
       Topic.any_instance.expects(:private_message?).returns(true)
       lambda { TopicView.new(topic.id, nil) }.should raise_error(Discourse::NotLoggedIn)
@@ -233,6 +232,26 @@ describe TopicView do
       p6.save!
     end
 
+    describe "contains_gaps?" do
+      it "does not contain contains_gaps with default filtering" do
+        topic_view.contains_gaps?.should be_false
+      end
+
+      it "contains contains_gaps when filtered by username" do
+        TopicView.new(topic.id, coding_horror, username_filters: ['eviltrout']).contains_gaps?.should be_true
+      end
+
+      it "contains contains_gaps when filtered by summary" do
+        TopicView.new(topic.id, coding_horror, filter: 'summary').contains_gaps?.should be_true
+      end
+
+      it "contains contains_gaps when filtered by best" do
+        TopicView.new(topic.id, coding_horror, best: 5).contains_gaps?.should be_true
+      end
+
+    end
+
+
     describe '#filter_posts_paged' do
       before { SiteSetting.stubs(:posts_per_page).returns(2) }
 

test/javascripts/models/post_stream_test.js

@@ -141,12 +141,10 @@ test("streamFilters", function() {
 
   deepEqual(postStream.get('streamFilters'), {}, "there are no postFilters by default");
   ok(postStream.get('hasNoFilters'), "there are no filters by default");
-  blank(postStream.get("filterDesc"), "there is no description of the filter");
 
   postStream.set('summary', true);
   deepEqual(postStream.get('streamFilters'), {filter: "summary"}, "postFilters contains the summary flag");
   ok(!postStream.get('hasNoFilters'), "now there are filters present");
-  present(postStream.get("filterDesc"), "there is a description of the filter");
 
   postStream.toggleParticipant(participant.username);
   deepEqual(postStream.get('streamFilters'), {