From 79627101322d6bc90052849ec21efbf74b00072a Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Tue, 18 Apr 2017 10:59:35 -0700 Subject: [PATCH] Remove 40-streams criterion for flagging dormant streams. Before this change, we would move "dormant" streams to the bottom of your stream sidebar, but only if you had 40+ streams. Now we do this in all cases to be more consistent. This commit also changes the redraw strategy when we remove rows. Before this change, we were doing incremental updates, but now we call build_stream_list to do a complete rebuild. This was partly motivated by adding the new divider, which would have complicated the incrememental approach when you removed the last remaining dormant stream. --- frontend_tests/node_tests/stream_list.js | 74 ++++++------------------ static/js/stream_list.js | 38 ++++-------- static/js/stream_sort.js | 5 +- 3 files changed, 29 insertions(+), 88 deletions(-) diff --git a/frontend_tests/node_tests/stream_list.js b/frontend_tests/node_tests/stream_list.js index df7ad68c5b..4c6354e5d7 100644 --- a/frontend_tests/node_tests/stream_list.js +++ b/frontend_tests/node_tests/stream_list.js @@ -165,6 +165,24 @@ function clear_filters() { stream_list.build_stream_list(); + global.stream_data.is_active = function (stream_name) { + return stream_name !== 'social'; + }; + + var streams = global.stream_sort.get_streams().slice(0, 6); + + assert.deepEqual(streams, [ + // three groups: pinned, normal, dormant + 'devel', + 'Rome', + 'test', + // + 'announce', + 'Denmark', + // + 'social', + ]); + // verify pinned streams are sorted by lowercase stream name var devel_li = stream_list.stream_sidebar.get_row(develSub.stream_id).get_li(); assert.equal(devel_li.next().find('[ data-name="Rome"]').length, 1); @@ -177,60 +195,4 @@ function clear_filters() { var Denmark_li = stream_list.stream_sidebar.get_row(DenmarkSub.stream_id).get_li(); assert.equal(Denmark_li.next().find('[ data-name="social"]').length, 1); - // verify pinned streams are sorted before unpinned streams - // i.e. the last pinned stream (testSub) is before the first unpinned stream (announceSub) - var test_li = stream_list.stream_sidebar.get_row(testSub.stream_id).get_li(); - assert.equal(test_li.nextAll().find('[ data-name="announce"]').length, 1); - - // add another 40 dummy unpinned streams since sort_recent is set to true only when - // there are more than 40 subscribed streams - for (var i = 1; i <= 40; i += 1) { - var dummyName = 'dummy' + i; - var dummySub = { - name: dummyName, - stream_id: 7000 + i, - color: 'green', - id: 11 + i, - pin_to_top: false, - subscribed: true, - }; - stream_list.create_sidebar_row(dummySub); - global.stream_data.add_sub(dummyName, dummySub); - } - - stream_data.populate_stream_topics_for_tests( - // testSub is pinned, DenmarkSub and socialSub are unpinned - _.object([testSub.name, socialSub.name, DenmarkSub.name], [testSub, socialSub, DenmarkSub]) - ); - - stream_list.build_stream_list(); - - var streams = global.stream_sort.get_streams().slice(0, 6); - - assert.deepEqual(streams, [ - 'devel', - 'Rome', - 'test', - 'Denmark', - 'social', - 'announce', - ]); - - // verify pinned streams are still sorted by lowercase name - // i.e. not affected by sort_recent set to true - assert.equal(devel_li.next().find('[ data-name="Rome"]').length, 1); - assert.equal(Rome_li.next().find('[ data-name="test"]').length, 1); - - // verify DenmarkSub and socialSub are sorted at the top of unpinned streams - // because they are active - assert.equal(Denmark_li.next().find('[ data-name="social"]').length, 1); - var social_li = stream_list.stream_sidebar.get_row(socialSub.stream_id).get_li(); - assert.equal(social_li.next().find('[ data-name="announce"]').length, 1); - - // verify inactive unpinned streams are still sorted by lowercase stream name - assert.equal(announce_li.next().find('[ data-name="dummy1"]').length, 1); - - // verify pinned streams are still sorted before unpinned streams - // i.e. the last pinned stream (testSub) is before the first unpinned stream (socialSub) - assert.equal(test_li.nextAll().find('[ data-name="social"]').length, 1); }()); diff --git a/static/js/stream_list.js b/static/js/stream_list.js index 1e09fc7292..707f7f235e 100644 --- a/static/js/stream_list.js +++ b/static/js/stream_list.js @@ -43,21 +43,11 @@ exports.stream_sidebar = (function () { }; self.remove_row = function (stream_id) { - var widget = self.rows.get(stream_id); - if (!widget) { - blueslip.warn('Cannot remove stream id ' + stream_id); - return; - } - - widget.remove(); - - // This
separates pinned streams from unpinned streams, - // so when removing a row, we check whether this removed the - // last pinned stream, and thus we no longer need the divider. - var $pinned_streams_hr = $("#stream_filters hr.pinned-stream-split"); - if ($pinned_streams_hr.prev().length === 0) { - $pinned_streams_hr.remove(); - } + // This only removes the row from our data structure. + // Our caller should use build_stream_list() to re-draw + // the sidebar, so that we don't have to deal with edge + // cases like removing the last pinned stream (and removing + // the divider). self.rows.del(stream_id); }; @@ -73,13 +63,7 @@ function get_search_term() { exports.remove_sidebar_row = function (stream_id) { exports.stream_sidebar.remove_row(stream_id); - - // We call sort_groups to update our list of streams, - // even though we don't use the result, since we can - // just remove the row. (The list of streams will - // be used by things like typeahead and hotkeys to - // advance to the next stream.) - stream_sort.sort_groups(get_search_term()); + exports.build_stream_list(); }; exports.create_initial_sidebar_rows = function () { @@ -117,9 +101,7 @@ exports.build_stream_list = function () { function add_sidebar_li(stream) { var sub = stream_data.get_sub(stream); var sidebar_row = exports.stream_sidebar.get_row(sub.stream_id); - if (stream_groups.sort_recent) { - sidebar_row.update_whether_active(); - } + sidebar_row.update_whether_active(); elems.push(sidebar_row.get_li().get(0)); } @@ -128,7 +110,7 @@ exports.build_stream_list = function () { _.each(stream_groups.pinned_streams, add_sidebar_li); if (stream_groups.pinned_streams.length > 0) { - elems.push($('
').get(0)); + elems.push($('
').get(0)); } _.each(stream_groups.normal_streams, add_sidebar_li); @@ -166,7 +148,7 @@ function zoom_in() { $(".stream-filters-label").each(function () { $(this).hide(); }); - $(".pinned-stream-split").each(function () { + $(".stream-split").each(function () { $(this).hide(); }); @@ -189,7 +171,7 @@ function zoom_out(options) { $(".stream-filters-label").each(function () { $(this).show(); }); - $(".pinned-stream-split").each(function () { + $(".stream-split").each(function () { $(this).show(); }); diff --git a/static/js/stream_sort.js b/static/js/stream_sort.js index 8f700650d2..df2c0f67c1 100644 --- a/static/js/stream_sort.js +++ b/static/js/stream_sort.js @@ -43,10 +43,8 @@ exports.sort_groups = function (search_term) { streams = filter_streams_by_search(streams, search_term); - var sort_recent = (streams.length > 40); - function is_normal(stream) { - return !sort_recent || stream_data.is_active(stream); + return stream_data.is_active(stream); } var pinned_streams = []; @@ -87,7 +85,6 @@ exports.sort_groups = function (search_term) { pinned_streams: pinned_streams, normal_streams: normal_streams, dormant_streams: dormant_streams, - sort_recent: sort_recent, }; };