I found a highly reproducible bug using the "Mark as unread" feature,
which triggers a rerender via message_list.rerender_view().
The reproducer was as follows:
* Navigate to a narrow by going to All messages and using the `S` hotkey.
* Mark as unread to mark several messages as unread in that view.
* Notice that the message that had been selected in home_message_list
is immediately marked as read again.
What was happening is that the reselect_selected_id call for
message_lists.home (All messages) was incorrectly re-marking the
currently selected message as read, even though (1) that view was not
visible and (2) this was an internal rendering change that could not
be the first time the message was selected.
Because only the current message_list has marking messages as read
blocked, it's still able to mark the currently selected message as
read.
All the callers of reselect_selected_id are internal rendering code
paths that are not intended to be user-visible; as a result, they
should not change the unread state either.
The bug fixed here is a potential root cause of #16986, but I haven't
had a chacne to confirm it.
redo_selection and reselect_selected_id did the same thing: Reselect
the currently selected message as a result of internal rendering. I
combine the useful features of both -- not crashing if no message is
selected (id -1) and passing the `from_rendering` option, to avoid
potentially triggering an additional rerender.
When viewing a narrow such as a search or `sender:` view, where
consecutive messages in the view may not be consecutive in their
original stream/topic context, we should avoid displaying the messages
with a shared sender/recipient bar header, as that creates the
incorrect perception that they are consecutive.
Back in 2013 (bc8bc8567b), we
implemented this via the collapse_messages flag, but it appears more
recent refactoring (no more recent than
dbffb2a614) made it always true.
The original logic was incorrect, in that it only considered full-text
search views, and not other views with this property.
I originally planned to use the existing logic for
can_mark_message_read designed for this purpose, but I think there
might be product reasons why might want the logic to be independent.
We move the stream subscribed/unsubscribed bookend info from
js files to bookend handlebar.
Tweaked by tabbott to override the check-templates indentation logic.
* We hide 1:1 PMs from and to muted users throughout
the UI, because doing so will not lead to loss of
conversational context. The "to" part is also important,
because the last few messages sent to a user before
muting them would probably be asking them to stop
spamming.
* After this change, we will need to do filtering for either
user or topic muting in pretty much all narrows, so we need
to keep the `_all_items` list in MessageListData always
up-to-date.
* A further commit will relax this and make it possible to
view these messages only when in a `pm-with/muted_user`
narrow.
This basically reverts 4bd7ec7c36 and
3a9dfc02e6.
The plan earlier was to have compeletely different codepaths
for user and topic muting, so that we could call seperate
functions in the message list class on receiving the respective
events.
However, this cannot be done, because if we, for example, on
receiving a `muted_users` event, filter `_all_items` based on
just user mutes, and store the result in `_items`, then, that
result may still contain topic-muted messages, which is
undesirable. Hence whenever we filter messages, we must do so
based on both user as well as topic muting.
(The code for the former will be added in further commits.)
So, we will have a single function which will handle updating
the message lists for muting.
Earlier, a user can only mute a topic from its recipient bar but can't
unmute it from there (and in fact we displayed an option to mute even
if the topic was already muted!). This commit fixes that bug and
allows a user also to unmute the topic from its recipient bar.
There are two core issues here;
* We did not have code, an icon, etc. for the "already muted" case in
the recipient bar logic at all.
* We did not rerender messages in !excludes_muted_topics views when
muting state changed.
See: 660475bd0c for background on when
we started only rerendering the streams with excludes_muted_topics
after muting changes. Rerendering of newly muted topics are important
for live rendering if a user is narrowed to that topic itself, which
are essentially all excludes_muted_topics narrows anyway.
Hence, now, we rerender by calling the `rerender` function for muted
topics (which is done just before we update the items for muting via
the function: `update_items_for_topic_muting`).
Tweaked by tabbott to add comments explaining the reasoning and
long-term plans.
Fixes#15223.
There is no need to store this field (and make sure
that it's the same in both the message_list and
message_list_data objects) on message_list objects,
because we can anyways access it easily with
`message_list.data.excludes_muted_topics`, and storing
it just on the data object seems more intuitive.
This also simplifies the constructor for the `MessageList`
class.
In fe66aef0ad, this call to
blueslip.fatal was ported incorrectly to `throw new Error` despite the
latter not supporting the extra_data parameter.
We address this by just doing blueslip logging with the extra data first.
Fixes#18273.
The bug happens in the case when the topic name is not set and the
user clicks on always_visible_topic_edit button results into unusual
behaviour of the always_visible_topic_edit button. To fix this, this
commit fix the behaviour by hiding and showing the
always_visible_topic_edit button in the appropiate situations, at the
same time we hide/show similar buttons.
Fixes#17813.
This is a prep commit for implementing mute users feature, which
renames this function to be more specific. This function cannot be
used as-is for user mutes, because for user-muted messages, we always
want to rerender the message list, irrespective of what
`excludes_muted_users` is.
In muting_ui.js, we also remove an unnecessary conditional that is
already handled by update_topic_muting_and_rerender itself.
This data structure has never been one that we actually render into
the DOM; instead, its role is to support clicking into view that
contain muted streams and topics quickly.
This downgrade makes that situation much more explicit, and is also
useful refactoring to help simpify the upcoming changes in #16746.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
This is a mostly verbatim extraction.
I re-phrased one line of code to work around a lint
false alarm. (Look for `preamble` in the diff.)
There are about 8 lines missing coverage here, so
the new module might be a good candidate to get
100% line coverage on.
Before this change, you would need to remove 74
edges from our dependency graph to make it
acyclic. Now it's 72.
Previously, the `muting_enabled` property of
MessageListData class was used to indicate whether
some messages in the message list need to be
filtered due to topic muting, depending on the
narrow. For example, we exclude messages belonging
to muted topics from stream narrows, but not from
search narrows.
The name `muting_enabled` is a bit confusing, and hence is
changed to `excludes_muted_topics`.
It is also important that the name be specific, since
a similar new property will be added for user mutes
in future commits.
The changes made in this commit are as follows:
* The `remove_messages` is moved to the `message_events.js`
file from `ui.js`.
* We refactor `MessageListData.change_message_id` to no
longer require an `opts` parameter as this function
just returns whether we need to rerender or not.
The blueslip error block can be removed since we made
the change to no long defer the data updates in
commit 3b5ba6b2c1,
this case can no longer occur.
The changes made in this commit are as follows:
* We remove the now unused `ui.find_message` which was added
in commit 1666403850.
* We change the function paramter to now accept message ids
instead of messages to eliminate redundant message ids to
message convertion as only the id is required.
* The remove method in MessageListData did not remove the
messages from the hash, it removed only from the items,
this fixes it.
* This commit also fixes a bug where messages are not added
to the current message list if an event is recieved where
messages are moved to this current narrow.
Only the message removal logic was present, which has been
refactored in this commit.
We were not updating the trailing bookend on deactivation of stream
if the user was narrowed to deactivated stream and this commit fixes
this.
For subscribed streams, we just show the trailing bookend with
content as 'This stream has been deactivated' and hide the
Unsubscribe button.
For unsubscribed streams, we change the content of trailing bookend
to 'This stream has been deactivated' and hide the Subscribe button.
Fixes#15999.
ES and TypeScript modules are strict by default and don’t need this
directive. ESLint will remind us to add it to new CommonJS files and
remove it from ES and TypeScript modules.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
This is done to decouple our message view related update events
from MessageListData as there are plans to create multiple
MessageListData objects. Instead we update the `stored_messages`
which tracks the complete data for all messages.
Tim Abbott
Anders Kaseorg
Aman Agrawal
Abhijeet Prasad Bodas
akshatdalton
Nikhil Maske
Steve Howell
ryanreh99
sahil839