From 7ea60ea1abfabdc33022d81d897480fac1164fa6 Mon Sep 17 00:00:00 2001 From: YashRE42 <33805964+YashRE42@users.noreply.github.com> Date: Mon, 15 Jun 2020 22:27:26 +0000 Subject: [PATCH] filter: Store reference to `_sub` instead of `_stream_params`. In commit 4f6377d49384bb63c30828d36a5dfae40349f822 we added `_stream_params` as a way of storing attributes such as stream name and stream privacy, this involved adding a few calls within functions that updated these values (in order to maintain consistency). This commit replaces `_stream_params` with an always consistent `_sub` object and removes unnecessary `_stream_params` related code. Once the `_sub` object is available, calls to `stream_data` may be considered suspicious as they can often be avoided by just picking the desired attribute off of the `_sub` object. --- static/js/filter.js | 45 +++++++++++++------------------------------- static/js/subs.js | 24 +++++------------------ static/js/tab_bar.js | 5 ++--- 3 files changed, 20 insertions(+), 54 deletions(-) diff --git a/static/js/filter.js b/static/js/filter.js index dc2046856c..231e1ef0f3 100644 --- a/static/js/filter.js +++ b/static/js/filter.js @@ -151,10 +151,12 @@ function message_matches_search_term(message, operator, operand) { function Filter(operators) { if (operators === undefined) { this._operators = []; - this._stream_params = undefined; + this._sub = undefined; } else { this._operators = this.fix_operators(operators); - this.fix_stream_params(); + if (this.has_operator('stream')) { + this._sub = stream_data.get_sub_by_name(this.operands('stream')[0]); + } } } @@ -480,7 +482,7 @@ Filter.prototype = { // this comes first because it has 3 term_types but is not a "complex filter" if (_.isEqual(term_types, ['stream', 'topic', 'search'])) { // if stream does not exist, redirect to All - if (!this._stream_params) { + if (!this._sub) { return "#"; } return '/#narrow/stream/' + stream_data.name_to_slug(this.operands('stream')[0]) + '/topic/' + this.operands('topic')[0]; @@ -495,7 +497,7 @@ Filter.prototype = { switch (term_types[0]) { case 'stream': // if stream does not exist, redirect to All - if (!this._stream_params) { + if (!this._sub) { return "#"; } return '/#narrow/stream/' + stream_data.name_to_slug(this.operands('stream')[0]); @@ -520,27 +522,6 @@ Filter.prototype = { return "#"; // redirect to All }, - fix_stream_params: function () { - this._stream_params = this.get_stream_params(); - }, - - get_stream_params: function () { - // we return undefined when we are unable to get these parameters - if (!this.has_operator('stream')) { - return; - } - const stream_name_from_search = this.operands('stream')[0]; - const sub = stream_data.get_sub_by_name(stream_name_from_search); - if (!sub) { - return; - } - return { - _stream_name: sub.name, - _is_stream_private: sub.invite_only, - _is_web_public: sub.is_web_public, - }; - }, - get_icon: function () { // We have special icons for the simple narrows available for the via sidebars. const term_types = this.sorted_term_types(); @@ -549,13 +530,13 @@ Filter.prototype = { case 'in-all': return 'home'; case 'stream': - if (!this._stream_params) { + if (!this._sub) { return 'question-circle-o'; } - if (this._stream_params._is_stream_private) { + if (this._sub.invite_only) { return 'lock'; } - if (this._stream_params._is_web_public) { + if (this._sub.is_web_public) { return 'globe'; } return 'hashtag'; @@ -575,10 +556,10 @@ Filter.prototype = { const term_types = this.sorted_term_types(); if (term_types.length === 3 && _.isEqual(term_types, ['stream', 'topic', 'search']) || term_types.length === 2 && _.isEqual(term_types, ['stream', 'topic'])) { - if (!this._stream_params) { + if (!this._sub) { return i18n.t('Unknown stream'); } - return this._stream_params._stream_name; + return this._sub.name; } if (term_types.length === 1 || term_types.length === 2 && term_types[1] === 'search') { switch (term_types[0]) { @@ -589,10 +570,10 @@ Filter.prototype = { case 'streams-public': return i18n.t('Public stream messages in organization'); case 'stream': - if (!this._stream_params) { + if (!this._sub) { return i18n.t('Unknown stream'); } - return this._stream_params._stream_name; + return this._sub.name; case 'is-starred': return i18n.t('Starred messages'); case 'is-mentioned': diff --git a/static/js/subs.js b/static/js/subs.js index 6922ef3ebf..2466b0261d 100644 --- a/static/js/subs.js +++ b/static/js/subs.js @@ -140,20 +140,9 @@ exports.update_stream_name = function (sub, new_name) { compose_state.stream_name(new_name); } - // Update navbar stream name if needed + // Update navbar if needed const filter = narrow_state.filter(); - if (filter && filter.operands("stream")[0] === old_name) { - // This works, but it relies on `filter.fix_stream_params` masking - // some bad behaviour in the Filter object. In particular, the fact - // that the Filter object relies on the search box which doesn't - // rename the currently focused stream. - // - // This will likely be improved as we migrate to using search pills - // and then a stream ID based representation of the stream in Filter. - - // update the stream_params stored in the filter object - filter.fix_stream_params(); - // use these to update the navbar + if (filter && filter._sub && filter._sub.stream_id === sub.stream_id) { tab_bar.render_title_area(); } }; @@ -171,7 +160,7 @@ exports.update_stream_description = function (sub, description, rendered_descrip // Update navbar if needed const filter = narrow_state.filter(); - if (filter && filter.operands("stream")[0] === sub.name) { + if (filter && filter._sub && filter._sub.stream_id === sub.stream_id) { tab_bar.render_title_area(); } }; @@ -189,12 +178,9 @@ exports.update_stream_privacy = function (sub, values) { stream_ui_updates.update_add_subscriptions_elements(sub); stream_list.redraw_stream_privacy(sub); - // Update navbar stream name if needed + // Update navbar if needed const filter = narrow_state.filter(); - if (filter && filter.operands("stream")[0] === sub.name) { - // update the stream_params stored in the filter object - filter.fix_stream_params(); - // use these to update the navbar + if (filter && filter._sub && filter._sub.stream_id === sub.stream_id) { tab_bar.render_title_area(); } }; diff --git a/static/js/tab_bar.js b/static/js/tab_bar.js index 8909c0ff8f..df8a87bdef 100644 --- a/static/js/tab_bar.js +++ b/static/js/tab_bar.js @@ -49,9 +49,8 @@ function make_tab_data(filter) { exports.colorize_tab_bar = function () { const filter = narrow_state.filter(); - if (filter === undefined || !filter.has_operator('stream') || !filter._stream_params) {return;} - const color_for_stream = stream_data.get_color(filter._stream_params._stream_name); - $("#tab_list .stream > .fa").css('color', color_for_stream); + if (filter === undefined || !filter._sub) {return;} + $("#tab_list .stream > .fa").css('color', filter._sub.color); }; function append_and_display_title_area(tab_bar_data) {