This just puts the style more clearly that one shouldn't be using
`this` to refer to the outer MessageListview object, because that
breaks unexpectedly when inside a loop.
Previously, if you were in the process of editing the last message in
a narrow and a new message came in, we'd rerender that second-to-last
message, causing your editing widget to lose focus (and thus the next
few keys you typed to be interpreted as keyboard shortcuts, which
had a good chance of resulting in your navigating somewhere random).
This rerendering was essentially unnecessary; the only change to state
going into the rendering process was the next_is_same_sender CSS class
being toggled on the messagebox in the message. So, at most, we
should have been just toggling that CSS class (and this commit makes
us do precisely that).
It seems like we could further improve this code by just removing the
next_is_same_sender CSS class entirely and removing this block, but
I'm leaving that for follow-up work.
Fixes#11656.
The bug here was that when we rerendered messages following local echo
through the echo.process_from_server code path, the eventual call to
_rerender_header() made the implicit assumption that all messages in a
message group had the same date. As a result, it created a totally
new/fake message group and called the rendering logic on that group
without calling the functions for setting up recipient row dates,
which would always result in no recipient bar date being added. This
bug was latent/invisible before, because when introduced, the locally
echoed messages were always being added to a recipient group from
today, where the recipient bar's date area was by default empty anyway.
This latent bug was revealed when we modified the structure of the app
to do date dividers between individual messages within a message
group, rather than strictly between message groups.
For consistency, we should keep all the code that works with
@mentions in markdown.js. In this case, message_list_view was
rewriting the contents of the mentions in cases where users'
names had been changed since we rendered their mention.
This change should help people discover to distinguish
silent mentions in text as a part of Zulip syntax while
differentiating them from regular mentions.
This adds date dividers within a single message group when the only
reason we had previously been splitting apart two message groups is a
change of date. The overall effect is a cleaner message list user
experience.
The downside of this change would be that the recipient bars no longer
will always show a new date for date changes; to fix that, we rewrite
how the floating recipient bars both set the date field on the
floating recipient bar itself, as well as ensure that non-floating
recipient bars don't show duplicate dates.
In a future design update where we modify how message recipient bars
look, we may very well be able to simplify this logic by removing some
of the dynamic nature of the recipient bar calculations. But this is
a good implementation of what remains.
Tweaked significantly by tabbott from Steve Howell's original, both to
extract these changes from a larger PR as well as to modify the
first_visible_message logic to handle some tricky corner cases.
Fixes#10171.
We need to move the update_group_date_divider call to run when a
message group is created. This achieves a few things:
* Fixes calling this multiple times useless for long message groups.
* It will soon no longer be correct to assume that every message
within a group has the same date, and in that case, we want to process
the date of the first message in the group, not of the last.
We only generate message_containers in one place, and that code path
already calls update_timestr. And update_timestr's effect only
depends on the message. Thus, this code was useless.
Since the main autoscroll feature was implemeneted, the
maybe_advance_to_recently_sent_message logic had an unfortunate
structure, where the code for this potentially large scroll was
running AFTER the autoscroll decision was made, but before an actual
scroll could have occurred.
This resulted in code that was very difficult to reason about, as
there were 2 potential sources of scrolling when you send a new
message, with little connection between their implementations either
in location or implementation.
Moving this into the main autoscrolling code path clarifies the code,
with the added benefit of fixing a bug where we would report to the
user that they needed to scroll down when in fact we were just about
to scroll the bottom of the feed into view (via
maybe_advance_to_recently_sent_message).
With this change, we never display the "you need to scroll manually"
message in the cast that we just scrolled you there via selecting a
message.
When you just sent a large message, our logic for "you need to scroll"
notifications did not correctly take into account the height of the
compose box. This was easily reproduced when sending very long
messages. The correct solution requires a bit of math to compute what
the visible area will look like after the compose box is closed.
This should be the final fix to #11138.
We recently added a feature to warn users that they
may need to scroll down to view messages that they
just sent, but it was broken due to various complexities
in the rendering code path.
Now we compute it a bit more rigorously.
It requires us to pass some info about rendering up
and down the stack, which is why it's kind of a long
commit, but the bulk of the logic is in these JS files:
* message_list_view.js
* notifications.js
I choose to pass structs around instead of booleans,
because I anticipate we may eventually add more metadata
about rendering to it, plus bools are just kinda brittle.
(The exceptions are that `_maybe_autoscroll`, which
is at the bottom of the stack, just passes back a simple
boolean, and `notify_local_mixes`, also at the bottom
of the stack, just accepts a simple boolean.)
This errs on the side of warning the user, even if the
new message is partially visible.
Fixes#11138
In a recent commit we allowed for `scroll_amount`
to be zero (as an indirect consequence of letting
`scroll_limit` be zero without early exiting).
See 0f75be3e8e
We want to short circuit the call to
`system_initiated_animate_scroll`, partly to save
unnecessary computation, but in particular to avoid
invoking the suppress-pointer-update logic.
There's a subtle change here in how we handle the
hypothetical case that the selected message is above
the top of the feed. Instead of early-exiting
from _maybe_autoscroll(), we just treat the limit
as zero, which will have the same effect.
We also change a var name be just `scroll_limit`
instead of `available_space_for_scroll`. A longer
name would be valuable if it were somehow more
specific, but it was needlessly verbose.
We calculate a few values higher up in the function.
This reduces a bit of code duplication and removes
a somewhat janky expression that happens against
a mutated list.
Previously, messages with more than one line did not parse '/me' at
the beginning of the message. Since there's a reasonable way to
render multi-line messages, this commit adds support for doing so.
This change does potentially break with the expected behavior of other
slash commands, but it seems worth providing useful functionality over
a blind focus on consistency.
Fixes#11025.
Previously, we were searching the whole message_row object for emoji,
mentions, etc., which has a bunch of UI elements that can't contain
the syntax we want to modify. This should be a slight improvement in
the performance of message post-processing, which runs a lot of times
and thus is fairly important.
This check caused us to only run the code inside that block if the
message mentioned the current user (since that's when the `mention`
class is added to the main message row).
While this was a useful performance optimization, it probably was a
small one, not worth it for the correctness cost.
If a user has an old mention and has since been renamed, there's
really nothing for us to do to render it; we should just return as
though we have no data.
The stream_list test that was fixed here was sort of
broken. It accomplished the main goal of verifying
what gets rendered, but now the data setup part is
more like the actual app code (and simpler, too).
When a user's name is edited, currently we still show the old name is
mentions (though clicking on the item does the right thing).
However, at present, it creates a new problem in search results, where
the highlighting is removed by this substitution.
Previously, Topic editing was offered in the UI even to message
senders and organizations admins only if the message was no more than
one day old. This was correct for the "community topic editing" case,
but not for message senders and organization admins.
While we're at it, this also centralizes some previously haphazard
logic to always call message_edit.is_topic_editable().
Tweaked significantly by tabbott to fix the logic.
Closes#10568.
For message groups, I just changed the internal name
to "topic_links".
For uses of "subject_links" that are tied to how the
server names fields, I introduced these wrappers:
* util.set_topic_links(obj, topic_links)
* util.get_topic_links(obj)
These can be used for either messages or events.
We split out two new functions and call them
everywhere that we used to call add_display_time():
- `update_group_time_display`
- `update_timestr`
We also make some of the local vars more consistent,
as well as doing more explicit clearing of vars than
`delete`.
Splitting these functions will allow us to muck with date
dividers without affecting the `update_str` functionality.
This allows several modules to no longer need
to import `narrow` (or, in our current pre-import
world, to not have to use that global).
The broken dependencies are reflected in the node
tests, which should now run slightly faster.
The values of this dictionary used to be raw DOM elements,
but get_row() wraps them again, so there's not a huge
reason to store them as raw DOM elements internally. It
is slightly easier to reason about the code if everything
stays at the jQuery level.
To preserve the old behavior here, we have to do something
that is kind of ugly, but at least it's explicit now. In
the old code, our cache was DOM elements, and if an id
wasn't in the cache, we would sneakily return $(undefined)
with this code in get_row():
return $(this._rows[id]);
And it turns out that $(undefined) is basically just a
zero-element jQuery object. A lot of our code depends
on this behavior and just works around the zero-element
objects as needed with checks like this:
if (this.selected_row()).length === 0) {
// don't try to get offset
}
For now we just preserve this behavior. We could eventually
be more strict here, or at least have aggressive warnings
on cache misses, but we'd need to retrofit code to be
able to call something like `has_rendered_selection()`
and/or deal with `undefined` as the return value for the case
where the selection hasn't been rendered.
Here is some example code that would cause tracebacks if
we just returned `undefined` for cache misses:
rerender_preserving_scrolltop: function () {
// old_offset is the number of pixels between the top of the
// viewable window and the selected message
var old_offset;
var selected_row = this.selected_row();
var selected_in_view = selected_row.length > 0;
if (selected_in_view) {
old_offset = selected_row.offset().top;
}
return this.rerender_with_target_scrolltop(selected_row,
old_offset);
},
This function is more cohesive and always takes in
a jQuery object containing exactly one DOM element,
and it does all stuff at the jQuery level of
abstraction (no raw DOM).
It's a pretty simple extraction--removing the level
of indentation makes the diff a bit noisy.
Tim Abbott
Steve Howell
Rohitt Vashishtha
Vaibhav
varunvaruns9
onkar shedge