Due to added opacity of 0.5 through the readonly property, the button
for copy and close was not clickable. Increasing z-index of the button
solves it.
New colors are introduced in msg edit history modal for better readability
These color will have better constrat with their background colors
Fixes#13622
This change adds a toggle widget to the "add streams" page that
lets the user change the sort order of the streams list. So far,
this supports sorting by stream name, by number of subscribers,
or by estimated weekly traffic.
Previously, in narrow viewports, the "filter"
option would disappear, which was very confusing.
This commit moves the filter streams input to the
next line, making it visible at all viewport widths.
@showell modified the commit message and got Casper
tests passing.
Fixes#12898.
This removes the messy click-or-drag detection algorithm originally
added in fe8f63c389, which fixed a messy
bug in an earlier algorithm from ~2013, whose sole purpose as to check
whether we're doing a selection and if so, not trigger the
click-on-message-body click handler.
The right fix is of course to do that check correctly.
We can use getSelection to distinguish between clicks and drags while
accessing the body of a saved draft. Previously, the draft would be
restored when trying to select draft text to copy/paste.
Fixes#14447.
Before this change, on clicking a checkbox to toggle subscription to a
stream no UI feedback was shown and users could toggle the checkbox
multiple times to send multiple requests causing bugs. This commit
initializes a spinner on clicking the checkbox, to provide a UI feedback
to the user. This commit also disables the checkbox once a request for
subscription has been sent and re-enables the checkbox only after a
response.
This change has been accomplished by introducing a div to display the
spinner in subscription.hbs. The corresponding styles for the spinner
have been added in subscriptions.scss. The ajaxSubscribe &
ajaxUnsubscribe functions in subs.js have been updated to show & hide
the spinners for the time the request is in process. An additional
parameter, the concerned stream object is passed to these functions(
through the sub_or_unsub function) to get the location where the spinner
is to be displayed. Finally, the checkbox click handler is updated to
support these changes.
The testing for this has been done by adding a wait of 2 secs in
actions.py for the response. This gives sufficient time to test the
working manually. Also, for error cases an error has been sent from
action.py and the behaviour has been manually observed.
Fixes#14481.
Some uploads can be rejected in the frontend, like when the file
size is too big, without sending the file to the server. Remove the
'Uploading file...' message from the compose box in such cases.
Now, the system uses word='' and an editing=True for rendering an
form for addition of alert words. This is a very vulnerable
way to implement said feature and this commit fixes that.
The addition form has been moved to alert_word_settings.hbs
thereby rendering it only once but always. Now, we do not have
to manually add an empty word and editing for the form to be
rendered.
As part of refactoring, the editing parameter has also been
removed as there is no purpose left.
This change allows us to align the search icon to the right end of the
navbar, without having to rely on the `margin-left: auto` trick. This
is better because it's more convenient to let flex handle the
positioning and will, hopefully, be more resilient to breakages.
The motivation behind this change is that it is more sensible to have
search_icon styles appear first and then be overridden by
search_closed or search_open styles which appear afterwards. This is a
prep commit to correcting a bug that caused some browsers to
render the width (and hence margin and position) of search_closed
incorrectly.
This updates the logged-in top navbar to display the stream/message
name, number of users, and description. It also replaces the search
bar with a search icon that expands into a full-width search bar.
Co-authored-by: Max Nussenbaum <max@maxnuss.com>
Fixes: #164.
Fixes: #5198.
In ee0d4541b4, we renamed the topic_date
-> stream_topic_history, and in the process renamed some local object
properties from .name => .topic_name, and accidentally change the
type for the data from the server as well.
The test fixtures were incorrectly migrated in the same way, so we fix
that as well.
While Hovering over some cell of the tables in settings Tab
increase the cell width since we are using the pseudo-Element `:: after`
the bug is fixed by adding `position: absolute;`.
fixes#14559
The disabled property actually prevented text selection, so it seems
better to use CSS through the `readonly="readonly"` property.
For this, swapped .prop() with .attr() since .prop() was setting it as
`readonly=""`.
`stream_topic_history` is a more appropriate name as this
module will contain information about last message of a
stream in upcoming commits. Function and variable names
are changed accordingly like:
* topic_history() -> per_stream_history()
* get_recent_names() -> get_recent_topic_names()
* name -> topic_name
If a file cannot be added for upload because of restrictions in frontend
we call cancelAll immediately in 'info-visible' callback. This would
prevent files that are already added to be cancelled but does not cancel
files that are yet to be added. So we use break to prevent any more files
from being added.
Calling uppy.cancelAll() when a batch of uploads is completed
result in the cancelation of any other batch of uploads that is
in progress. This case happens when a user uploads some files
and then tries to upload another bunch of files before the existing
upload is completed.
Removed the condition in the settings_save_discard_widget page which
allowed the page to be rendered only for admins. This change was
important so that this widget can be used on the user settings page
for displaying confirmation messages.
For non-admins, trying to access admin-only settings, behaviour remains
the same as before as these settings are disabled for non-admins.
The form for entering alert words has been moved above the list
of words.
The list of words will be presented alphabetically rather than
time of addition.
This refactors add_default_stream in zerver/views/streams.py to
take in stream_id as parameter instead of stream_name.
Minor changes have been made to test_subs.py and settings_streams.js
accordingly.
Added UI support for uploding the new profile picture by
clicking on the avatar rather than a button.
Added new spinner for loading indication while uploading
a new avatar over the avatar area.
Fixes#10255
The original commit here was sorting bot owners by
id, which is of course meaningless to users:
444ce74a8e
It was also returning 1/-1 in cases where the bot
owner on both sides of a comparison were missing,
which is a big no-no for sorting algorithms.
We want to avoid creating jQuery objects that just
get turned right back into strings by the list
widget, so we now have our template just include
`last_active_date` instead of kludging it in
after the fact, and we return the template
string in `modifier` rather than wrapping it.
To deal with plain HTML we switch to using
`render_now`.
Calling `render_now` leads to a more simple
codepath than `render_date`, beyond just dealing
with text.
The `render_date` function has special-case logic
that only applies to our time dividers in our
message view, which is why we were passing the
strange `undefined` parameter to it before this
fix.
The `render_date` function was also putting
the dates into `update_list` for once-a-day
updates, which is overkill for an admin screen.
We don't use this logic for drafts or attachments
either. I'm not sure how well tested that logic
is, and it's prone to slow leaks.
This commit sets us up to simplify the list
widget not to have bit-rot-prone code related
to jQuery objects.
We now:
- Skip the broken "Never" case. (The way
we were distinguishing "Unknown" from
"Never" was based on brittle checks that
were just wrong due to bitrot--see Steve
Shank on czo as an example. If we want
to make this distinction rigorous in the
future, we should have a clear mechanism.
If somebody's never actually been active,
we probably want to treat that more like
a dead-on-arrival login, anyway, and make
it easy to clean them up.)
- Use the `presence.last_active_date` instead
of reaching into private data structures.
- Avoid the unnecessary intermediate constants
of LAST_ACTIVE_NEVER and LAST_ACTIVE_UNKOWN.
- Avoid setting `last_active` in `populate_users`.
This commit was modified by @showell:
- I cleaned up the commit message.
- I simplified the diff a bit to avoid
some renaming and lexical moves.
We already know which list widget a `<th>`
tag is associated with when we set up the
event handler, so it's silly to read data
from the DOM to find that widget again
when the handler runs.
This commit eliminates a whole class of possible
errors and busy work.
For some widgets we now avoid duplicate redraw
events from this old pattern:
widget = list_render.create(..., {
}).init();
widget.sort(...);
The above code was wasteful and possibly
flicker-y due to the fact that `init` and
`sort` both render.
Now we do this:
widget = list_render.create(..., {
init_sort: [...],
});
For other widgets we just clean up the need
to call `init()` right after `create()`.
We also allow widgets to pass in `sort_fields`
during initialization (since you may want to
have `init_sort` use a custom sort before the
first render.)
Finally, we make the second and third calls
eliminate the prior updates from the previous
widget. This can prevent strange bugs with
double-reversing columns (although that's
been prevented in a better way with a recent
commit), as well as avoiding double work
with sorting.
This code has always been kind of convoluted
and buggy, starting with the first
sorting-related commit, which put filtering
before sorting for some reason:
3706e2c6ba
This should fix bugs like the fact that
changing filter text would not respect
reversed sorts.
Now the scheme is simple:
- external UI actions set `meta` values like
filter_value, reverse_mode, and
sorting_function, as needed, through
simple setters
- use `hard_redraw` to do a redraw and
trigger external actions
- all filtering/sorting/reverse logic on
the *data* happens in a single, simple
function called `filter_and_sort`
We don't use this anywhere. You can do
`git grep -A 40 list_render.create` to verify
this (with a little bit of noise in the grep).
A better strategy for generalizing
this code is to extract the useful logic
into a function that callers can use in their
own custom event handlers, which I'll do
in an upcoming commit.
We put this in `scroll_util` to make it more likely
we will eventually unify this with other scrolling
logic. (A big piece to move is ui.get_scroll_element,
but that's for another PR.)
And then the other tactical advantage is that we get
100% line coverage on it.
I changed the warning to an error, since I don't
think we ever expect scrolling at the `body` level,
and I don't bother with the preview node.
We extract a general purpose widget to create dropdown lists with
search. This widget is used for default code block language, but can
be easily extended to cover notifications_stream and similar options.
The current usage is:
```js
const widget = DropdownListWidget({
setting_name: 'realm_alpha_beta',
data: [{name: 'hello', value: 'world'}, {...}, ...],
subsection: 'msg-editing',
default_text: 'Nothing is selected',
});
```
and
```handlebars
{{> dropdown_list_widget
setting_name="realm_alpha_beta"
list_placeholder=(t 'Filter the data')
reset_button_text=(t '[Unset]')
label=admin_settings_label.realm_alpha_beta }}
```
This can further be refined by shifting more variables from handlebars
to javascript in the future.
By taking these functions out of exports.build_page, we can
reuse them for handling other widgets. We also declare
default_code_language_widget after the helper functions to
avoid the linter complaining.
I pushed this risk commit to the end of
a PR that had a bunch of harmless prep
commits at the front, and I didn't make
it clear enough that the last commit (this
one) hadn't been tested thoroughly.
For the list_render widget, we can simplify
the intialization pretty easily (avoid
extra sorts, for example), but the cache aspects
are still tricky on subsequent calls.
Changes .data() Jquery methods to .attr() to prevent unnecessary data
type conversions of the emoji name.
Tested the fix manually and verified the test-js-with-node test suite.
Fixes: #14377
For some widgets we now avoid duplicate redraw
events from this old pattern:
widget = list_render.create(..., {
}).init();
widget.sort(...);
The above code was wasteful and possibly
flicker-y due to the fact that `init` and
`sort` both render.
Now we do this:
widget = list_render.create(..., {
init_sort: [...],
});
For other widgets we just clean up the need
to call `init()` right after `create()`.
We also allow widgets to pass in `sort_fields`
during initialization (since you may want to
have `init_sort` use a custom sort before the
first render.)
The get() logic here was broken, because
when you enter the settings panel for invites
on the 2nd or 3rd time, the text filter
would not work.
This commit doesn't intend to fix the problem; it
just simplifies the code for a later commit
that fixes this holistically.
The way that we update `list_render` objects
is janky with respect to events, so we can end
up double-sorting lists (which puts them back
to normal) and strange things like that.
This is all cosmetic.
Instead of:
const.widget = {
foo: function () = {
},
bar: function () {
},
};
We have:
const widget = {};
widget.foo = function () = {
};
widget.bar = function () {
};
Before this fix, we'd get a traceback if you looked
at invites in the settings (and if one of them was
a multi-user invite link). This commit fixes
that problem by adding a custom sort.
We also rename the "Email" column to "Invitee",
since it's often the case the invitee isn't an
actually an email, but it's instead a multi-use
link.
Note that the invites UI only works the first time you
enter settings. Many of the controls break the second
time you enter it. You can't sort by column header
or use the text filter.
I'll fix that in a subsequent commit.
Giving these functions a name and moving them to
the top-level scope has a couple tactical advantages:
- names show in tracebacks
- code is less indented
- setup code is less cluttered
- will be easier to add unit tests
- will make some upcoming diffs nicer
These are technically more `compare_foo` than `sort_foo`,
but we already had a naming convention that was sort of
in place.
We had a bug where if your peer mentioned you in
message, but then edited the message not to mention
you, the latter wouldn't reset your unread counts
for "Mentions". And the same problem would happen
vice versa.
The fix basically extracts `update_message_for_mention`
and makes sure it handles all combinations of
unread/mentioned flags, instead of assuming
any invariants about which directions of change
are possible.
And then we call that new function from
`message_events.js` whenever we get message
edit events.
Fixes#14544
Earlier, the non-editable text-boxes(on clicking view source/edit
topic) were not so apparent due to absence of `disabled` attribute.
Adding the `disabled` attribute makes them consistent with the approach
for non-editable text-boxes and text-areas in organization settings
(for non-admins).
Fixes: #14375
The `send_message_ajax` function was a relic
of us having an alternative way to send messages
(web sockets) to the server, but now the indirection
is more confusing than helpful.
This also fixes trying to cancel a resend of a
local message.
The problem were was type confusion between
strings and ints.
The function in `rows.js` may feel like overkill,
but I really want to enforce type safety here,
as we usually treat message ids as floats, but
for the local-echo case we're gonna get
strings. I put it in `rows` because we mostly
do a good job of encapsulating the "zid" role
in the DOM there.
By going directly to the DOM here, we avoid
parsing a string to a float and then converting
it right back to a string, which always make
me queasy about float rounding, so one less
moving part.
Due to type confusion, we were silently failing
to delete local_id values for messages that were
being acked by the server.
This used to work when we kept values in our
old Dict data structure, since client_message.id
and message.local_id are really the same value,
just the former is a float and the latter's a
string, and Dict never cared.
We can avoid all this confusion, though, by just
consistently using `local_id`, which I extract
to a local var.
The function message_send_error was messing up
on calls to message.get when we were passing in
string versions of `local_id`. Now we pass in
float ids.
This fixes a traceback where we tried to set
`.failed_request` on to an `undefined` value
that we had instead expected to be a locally
echoed message from our message store.
This will allow us access to the float version of the
message's id in an upcoming commit, without us having
to do possibly brittle string-to-float translations.
Option is added to video_chat_provider settings for disabling
video calls.
Video call icon is hidden in two cases-
1. video_chat_provider is set to disabled.
2. video_chat_provider is set to Jitsi and settings.JITSI_SERVER_URL
is none.
Relevant tests are added and modified.
Fixes#14483
This adds a new realm setting: default_code_block_language.
This PR also adds a new widget to specify a language, which
behaves somewhat differently from other widgets of the same
kind; instead of exposing methods to the whole module, we
just create a single IIFE that handles all the interactions
with the DOM for the widget.
We also move the code for remapping languages to format_code
function since we want to preserve the original language to
decide if we override it using default_code_clock_language.
Fixes#14404.
The filter-linkifier input box was disabled which prevented users from
filtering through the linkifiers list. Removed the part of code which
caused the input box to be disabled. This allows users to edit the input
and so filter linkifiers.
With EMAIL_ADDRESS_VISIBILITY_NOBODY (or as a non-admin with
EMAIL_ADDRESS_VISIBILITY_HIDDEN), we were incorrectly generating
zuliprc files containing the shareable email address, which naturally
didn't work.
Most failures result from invalid emoji names, so this makes it easier
to recover without re-uploading a file.
Previously, this model would have been problematic, but now that we
have the visual preview, this is clearly better behavior.
This adds a preview of the uploaded emoji image while uploading custom
emoji right below the upload form.
Modified upload_widget.build_widget() to take in the preview
span text and image. In case a parameter isn't passed
for preview text, it defaults to null and the snippets in
build_widgets() related to preview don't run.
Fixes#9229.
Styling tweaked by tabbott.
We now only compute idx on the outbound side,
instead of spreading out the responsibility.
We just iterate through all our items to find
the next available number.
The name here is a bit more precise, as we're
not checking whether a task exists so much
as whether just a particular name is in use.
We also move the function out of the `check_task`
layer, which feels a bit overkill in terms
of nesting (plus, we're gonna remove the other
function inside of `check_task` soon).
This is a prep commit for changes to the top navbar, it adds helpers
to filter.js which will help control the behavior of some aspects of
the redesigned navbar.
Modified by tabbott to add comments, internationalization tags on the
strings, support streams:public, and change various title strings.
This is a prep commit to the change of the navbar UI, in the new UI
the navbar and search box are toggled by icons and exist in the same
space on the UI. This commit only moves the search_box styles to be
near the tab_bar styles, so that future changes are easier to make,
read and maintain.
We fix this by adding a more expressive data function, with tests, for
whether a filter is on UserMessage data, which would mean that
streams:public could never add additional matches.
We now use `assert.throws()` to test that we're
properly calling `blueslip.fatal`.
In order to not break line coverage here, we have
to remove an unreachable `return` in `stream_data.js`.
Usually we test `fatal` for line coverage reasons.
Most places where we use `blueslip.fatal` fall in
these categories:
* the code is theoretically unreachable, but
we have `blueslip.fatal` for defensive reasons
* we have some upstream bug that we should just
fix
* the code should recover gracefully and just
use blueslip.errors()
It's possible that we should eliminate `blueslip.fatal`
from our API and just throw errors when really important
invariants get broken. This will make it more obvious
to somebody reading the code that we're not going to
continue after the call, and `blueslip` already knows
how to catch exceptions and report them.
The todo_widget was using the using a counter to store the key value of
every task. This would cause assiging multiple tasks the same key value
in a race condition. To avoid this we make "sender_id" a part of the key
along with the counter.
Also the `key` now not being a integer value, we can't use it to find the
index of the task using it. Thus, a function is made that will find the
index of task whose key is sent by the user to strike.
This is part of #6427, adding support for live-updating the Zulip UI
to move messages to a new topic.
As noted in the comments, there is still a bug to be fixed here
involving guest users, but the overall implementation is pretty well
tested manually (which is how we test most message-edit UI work since
there's so much complexity involved).
Co-Authored-By: Wbert Adrián Castro Vera <wbertc@gmail.com>
We no longer delete existing drafts if you happen
to clear the text in your compose box for a message
that was restored from an existing draft. This
prevents folks from losing drafts when they accidentally
delete selected text.
There are still two ways to delete a draft:
* send the message (obviously not always desirable)
* use the drafts UI (with `d` as a shortcut to bring it up)
See https://chat.zulip.org/#narrow/stream/9-issues/topic/lost.20draft
for more discussion.
This commit modifies the padding and margin of the input selector so
that is uses sane values such as 25ps top margin, 5px bottom margin
and 10px top and bottom padding rather than trying to make uneven
values balance each other out. (old values are 25px top margin, 4px
bottom margin, 9px top padding, 11px bottom padding)
If the subscription data was changed from the left sidebar,
we previously would attempt to display the savings indicator
in the stream edit page which wasn't rendered yet. The bug was
introduced in commit 39577b58ba.
This approach is used to harden the codepath against bugs by
keeping the expectOne check in `settings_ui.do_settings_change`
function.
Fixes#14467.
Enhance visibility of "x" to dismiss the dialog box of "You have nothing
to send!" message.
To achieve this:
Added class 'compose-send-status-close' with new color attribute in
file night_mode.scss.
Fixes: #14459
Co-authored-by: @MariaGkoulta <43913366+MariaGkoulta@users.noreply.github.com>
When we redraw the left sidebar, we need to tell the
topic list to clear its data structures (and do other
stuff like hiding its popover), since we are clearing
its parent container.
The commit f0e18b3b3e
introduced this regression in late January 2020.
That commit made topic_list use a vdom to avoid
unnecessary updates. Before that, topic_list did
a lot of brute-force redraws, which covered up the
fact that we weren't having stream_list telling it
when the rug was being pulled out from under it.
The boundary between stream_list and topic_list
has always been kind of complicated code, since
topic lists get embedded into the stream list.
The main interactions, though, are basically:
* topic_zoom.clear_topics() - you're leaving
a narrow that may or may not be zoomed
* topic_list.clear() - you're about to redraw
stream items in the unzoomed stream list
* topic_list.rebuild(stream_li, stream_id) -
you're building or updating a topic list
for the newly active stream
Fixes#14465
Users are unable to modify organization's profile picture, but
disabled buttons for the same are being shown to the user on the
organization profile settings page. This commit removes those
buttons. The file realm-logo-widget.hbs renders those buttons only
if the user is an admin and realm_logo.js has been updated to allow
operations(like click) on the buttons only to admins.
Users are unable to modify organization's logos, but disabled
buttons for the same are being shown to the user on the organization
settings page. This commit removes those buttons. The file
realm-logo-widget.hbs renders those buttons only if the user is an
admin and realm_logo.js has been updated to allow operations
(like click) on the buttons only to admins.
The `options` parameter is not being passed in any call
of `lightbox.open()` and it uses the same option i.e.
`lightbox_canvas` everytime which is now computed inside
`display_image()` directly.
`image` passed to lightbox.open() is already a jQuery object,
so we don't need to convert it explicitly. Also, the parameter
is renamed from `image` to `$image`.
Previously, lightbox.open() was responsible for retrieving
the image data from the DOM, saving it in `asset_map` and
finally displaying the image using that data. This
implementation wasn't correct for image list at bottom of
the lightbox because the `image` parameter passed to
lightbox.open() could contain more than one instances of
the image that had to be opened.
Now, the metadata of all the images in image-list is stored
in the `asset_map` while rendering the `image-list` inside
`render_lightbox_list_images()` and `lightbox.open()` only
looks for the metadata from `asset_map`.
Fixes#14152.
In case of video embeds, the previous logic used
`data-src-fullsize` or `src` as a key to look
for the metadata of video in `lightbox.open()`,
but while parsing, the key used while storing
the metadata was the video ID.
This doesn't make any sense because video's data
could never be accessed from `asset_map` and we
always needed to lookup the DOM for this.
This commit fixes this by using $img.attr('src')
as a key for `asset_map` for both, images and
videos. Since `src` is the link of preview image
in case of video embeds, it will always uniquely
determine the video ID and we won't loose
anything with the change in how videos handle
things.
Part of #14152.
The value of `canvas.parentNode` in `sizeCanvas()`
appears to be `null` sometimes and it throwed an
exception specially when you switch images from
the images-list quickly.
This changes the payload that is used
to populate `page_params` for the webapp,
as well as responses to the once-every-50-seconds
presence pings.
Now our dictionary of users only has these
two fields in the value:
- activity_timestamp
- idle_timestamp
Example data:
{
6: Object { idle_timestamp: 1585746028 },
7: Object { active_timestamp: 1585745774 },
8: Object { active_timestamp: 1585745578,
idle_timestamp: 1585745400}
}
We only send the slimmer type of payload
to clients that have set `slim_presence`
to True.
Note that this commit does not change the format
of the event data, which still looks like this:
{
website: {
client: 'website',
pushable: false,
status: 'active',
timestamp: 1585745225
}
}
Next to the checkbox of "Show previews of linked websites" added a
documentation link in order to help users not confuse its function.
To do this:
- Added the field 'help' which is the link for the documentation in
organization_settings_admin.hbs
- Added the if statement in settings_checkbox.hbs to check if the above
field exists.In case it exists, a help icon which leads to the documentation
link appears.
The specific field was added in order to be able to add a help link in
other settings as well.
Co-authored-by: Katerina Perikou
<44238834+kPerikou@users.noreply.github.com>
Fixes: #13450
Fixes the problem of recipient_bar_icons being too close to each
other. To improve spacing between them, classes are added, namely
recipient_bar_icon_link (for link icon) and reciepient_bar_icon
(for other icons). CSS for spacing these classes correctly, using
padding-left and padding-right, has been added zulip.scss
Manually tested for cases with single and multiple links present.
Fixes#14364.
When we tried to copy/paste multiple rows up to
and including the last row in our view, we'd have
a blueslip error when the `for` loop checked the
condition `rows.id(row) <= ...` after we had
called `row = rows.next_visible(row)` on the last
row. Basically, `rows.id()` would complain
about a non-existent row.
Now we extract that code into `visible_range`, so
that our `while` loop can exit as soon as we found
the last row in the range.
The selector we were passing to `condense_and_collapse`
included rows from our drafts UI, which don't have
zids and don't play nice with condense/collapse code
(which expects message ids for settings things like
`.condense` flags).
Now we just use a better selector.
If folks use an overly broad selector for message rows,
they will accidentally include drafts from the drafts
dialog, which won't have zids. More specific selectors
will be more efficient and possibly prevent strange
behaviors.
For testing convenience, we extract the message.
We now handle the esc key completely within the
keydown handler that we already have for message
editing. We allow escape to work no matter what
the focused element is within an edited message,
and we blur that element properly and end the
edit.
We remove all the strange, duplicated logic
from hotkey.js.
This should also fix a blueslip error where the
hotkey code was passing message_edit a jQuery
element with zero length.
Fixes the traceback reported in #14151, though we should still look at
the DOM cleanup discussed there.
The UI in the `#settings/notifications` page is updated similarly
to what is done in the `update_global_notifications` path present
in the `server_events_dispatch` file.
We have an alert for when the stream name is changed.
This also adds an alert when subscription settings
are updated and the widget is similar to that used in
the settings page.
This is also necessary because the stream specific
notification settings UI updation goes through this
path and it is necessary to display a confirmation
to match with other settings confirmation pattern.
Since each element containing the `.alert-notification`
class has a predefined area that wont overlap with any
other element, we make changes to the CSS so that it
just stays hidden until the text appears.
This setting is being overridden by the frontend since the last
commit, and the security model is clearer and more robust if we don't
make it appear as though the markdown processor is handling this
issue.
Co-authored-by: Tim Abbott <tabbott@zulipchat.com>
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
While we could fix this issue by changing the markdown processor,
doing so is not a robust solution, because even a momentary bug in the
markdown processor could allow cached messages that do not follow our
security policy.
This change ensures that even if our markdown processor has bugs that
result in rendered content that does not properly follow our policy of
using rel="noopener noreferrer" on links, we'll still do something
reasonable.
Co-authored-by: Tim Abbott <tabbott@zulipchat.com>
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
If we can't find data on a mentioned user to update its full_name to
the current value, we'll have to go with the value in the message
itself.
This can happen if e.g. we hard-deleted the originally mentioned user
from the database (which can sometimes happen after a "delete my
account completely" request).
The user has an option for setting global
notification settings as well as the same settings
for individual streams. Currently the user has to
keep track of each unmatched stream and then visit
each individual stream whose settings he wants to
update.
Thus this adds a dedicated UI table allowing the user
to view and update the notifications of the specific
streams which differs from the global settings.
It is located on the same page where the user defined
global notification settings can be modified.
Fixes#9228.
Currently we are updating the checkbox UI as soon as the user clicks.
This block is removed to match with the pattern of rest of the
properties in the stream edit page where `stream_events.update_property`
is responsible for updating the UI after a successful server response.
This function returns a list of objects to create a
list_render object, and each item contains the streams
whose atleast one notification setting differs from the
default set by the user.
This is done by comparing the global settings in the
`#settings/notifications` page with those settings
present in the subscribed streams.
Work towards #9228.
This flag was used to delay unread count updates while the bankruptcy
modal was visible. Now that bankrupcty is no longer a modal, we don't
need this flag at all.
Switched to top-of-page prompt to make it natural to fit in with other
notifications. As we switch to panel-based prompt, templates for the
bankruptcy modal are moved along with its usage in application's
homepage.
We include a bit of delay before reloading to make it easy for the
user to read the "Marking all messages as read" banner before it is
covered by the "Reloading..." notice in environments where the reload
is fast.
Fixes#3347.
When stream_post_policy modal is closed either after saving or using
cancel button or cross button, the pointer-events is set to none which
does not allow to close the stream settings overlay on one click.
Added overlay.close_modal on saving such that pointer-events:none is
removed.
Added line which removes pointer-events:none again on clicking cancel
button or close icon.
This is a prep commit which extracts the part of the code in open_modal
and close_modal to separate methods which adds inline style of
pointer-events to enable/disable the background mouse events.
Block comments are added for easy understanding of reader.
This commit removes "font-weight: 500;" from landing-page.scss so as
to fix a bug on landing pages that used the `markdown` class to
format content. The bug was caused by "a:hover" from landing-page.scss
overriding the font-weight (600) on links as set by the markdown
class, this caused the text to seem jumpy when one hovered over links.
Note from tabbott: The original code was added in
d7f5f31f6a, which doesn't explain it's
purpose, but it predates the more complete "markdown" CSS, was part of
an early prototype that had unfortunate hover behavior more generally,
and makes sense to remove.
Fixes: #14387.
If a non-author user clicked on view source in a poll and then close it,
the edit question icon would incorrectly get visible. This made changing
the question in local echo possible for non-author users.
Fixes: #14299
Starred messages from muted topics were not shown in the starred
messages view. Condition for muting_enabled is modified accordingly
such that the starred messages from muted topics is shown in the
starred messages narrowed view.
Node tests are updated accordingly.
Fixes#13548
The previous system for documenting arguments was very ugly if any of
the examples or descriptions were wrong. After thinking about this
for a while, I concluded the core problem was that a table was the
wrong design element to use for API parameters, and we'd be much
better off with individual card-type widgets instead.
This rewrites the API arguments documentation implementation to use a
basic sort of card-like system with some basic styling; I think the
result is a lot more readable, and it's a lot more clear how we would
add additional OpenAPI details (like parameter types) to the
documentation.
The previous logic avoided updating the setting for
non-administrators, because their value was always true, but removing
those if statements results in better test coverage and is more likely
correct if we ever try to support live-update for whether the user is
an administrator.
We've noticed that many production organizations don't set either an
organization description or profile picture, even large open source
organizations that could definitely take advantage of this feature.
This adds a top-of-page banner that bugs organization administrators
to add an organization description and profile picture, generally
starting on the second login (as we only do it on page load after
notifications are configured).
Significantly tweaked by tabbott to get the right user experience.
Fixes#14019.
The original implementation of panels.js was just for notifications,
and ended up running a bunch of notifications-specific code, including
registration click handlers and some localstorage-related
notifications logic, every time a panel was supposed to be opened.
This refactoring makes the panels library make sense -- we now
initialize all click handlers in the initialize() method, and do the
notifications check in a single, coherent place scoped to notifications.
Commit 68335d9124 removed the ability to tab
into this field, since it was a hidden field. This field is no longer
hidden, and this commit restores the ability to tab into it.
In continuation to #13250
CHANGES:
-the stream name edit button is now visible for long names too.
-ellipsis are removed when you click on edit name option.
-added border while editing name to give a text-box feel.
REASONS:
-added border while editing the name to give a textbox-esque feel.
-text overflow was changed from ellipsis to clip (while editing) as
ellipsis prevented editing the entire name (clip provides better
functionality).
The last two changes are reverted back to original (i.e. ellipsis and
no border) once you finish editing the stream name.
P.S.- clicking on anywhere else updates the new name perfectly
Here we have migrated checkboxes of all general notifications to the table.
By general notifications we mean, Mobile, Email, Desktop audio, and visual
notifications.
This is a part of a bigger migration to simply our notifications setting
changing infrastructure for all streams and individual streams. Later we
will add more row to this for different categories of notifications in
addition to the current ones ("Streams" and "PMs, mentions, alerts").
Fixes: #12182.
When you select a typeahead, it shouldn't
immediately do the action for you; you should
have to hit enter first. Even though 99% of
the time you're gonna confirm the typeahead,
it's jarring when you don't expect it.
You can still add a bunch of default streams
quickly, using only the keyboard, because
we have always had support for the enter
key saving. (and tab and enter also works)
This is a full-stack change:
- server
- JS code
- templates
It's all pretty simple--just use stream_id instead
of stream_name.
I am 99% sure we don't document this API nor use it
in mobile, so it should be a safe change.
We now only use `page_params.realm_default_streams` during
initialization, and then after that we use `stream_data`
APIs to get default stream ids and related info. (And
for the event that replace the data, we just update our
internal data structures as well.)
Long term we should have the server just send us ids here,
since we are now hydrating info from stream data in all places.
This code is a bit simpler.
The previous code was concatenating two lists
and then removing duplicates by calling filter().
Now we just have two loops that append to a single
list, and the second loop detects duplicates
before inserting into the list.
We also now use `default_stream_ids` instead of
`page_params` data, which is convenient for two
reasons:
- working with sets of ids is convenient
- we don't need to maintain `page_params`
data any more
We now use the up-to-date info from stream_data
to hydrate the default stream ids. All we need
here in the template is `invite_only` and `name`.
Since we are no longer using data from `page_params`,
we can remove `maybe_update_realm_default_stream_name`.
(If you are wondering if we still get live updates,
we get that via a more upstream call to
update_default_streams_table in the event
dispatching codepath.)
We only used get_default_stream_names() in a
test, so now it's being replaced with a function
that just gets ids.
We'll have use for get_default_streams_ids()
in an upcoming commit.
Now if a default stream gets deleted, we just
redraw the table. We always have a small number
of default streams, and the way that we were removing
rows without the actual consent of `list_render` was
really janky (and just a vestige of pre-list-render
code that never got fully ported).
This also makes us consistent with how we handle
added streams (i.e. just call
`update_default_streams_table`).
ASIDE:
Ideally we will update `list_render` at some point to
have an API for adding and removing elements. It does
allow you now to call `data()` to reset its data, but
for now we just build a new `list_render` object every
time.
Commit 03393631bd (#14142) regressed the
keyboard accessibility of the keyboard shortcuts modal. Fix it by
moving tabindex="0" to the scrolling element of the SimpleBar.
Fixes#14320.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
We stopped needing this with
0329b67048
(Dec 2016).
The function sets `bot.can_admin`,
which was only used in `bot_data.get_editable`.
We removed two tests (and then put back
some test setup that needed to leak down
to the last test).
This is code simplification motivated
by a recent bug that we fixed with some
server changes, but which was really
caused in some sense by our client code
using an overly finicky
condition to check falsiness.
For cross-realm bots, the value of
`user.bot_owner_id` may be `null`, or it
may simply be `undefined`, depending
on whether the server passes `None`
or simply omits the field.
We don't want out client code to be
coupled to that rather arbitrary
decision.
We were doing a `!== null` check instead
of checking for falsiness, which led to
blueslip errors in the past. Because a
bot owner id could be plausibly 0, a falsiness
check would be brittle in a different way.
Now we avoid that ugliness by calling
`get_bot_owner_user`, which either returns
an object or `undefined`.
And then the caller can just do a concise
check for whether `bot_owner` exists.
And we also fix up the crufty code that
was putting `bot_owner_full_name` on to
the object instead of using a local.
We have a bug report for this again, although
it might be on an old branch.
Fixes#13621.
This allows us to block use of the desktop app with insecure versions
(we simply fail to load the Zulip webapp at all, instead rendering an
error page).
For now we block only versions that are known to be both insecure and
not auto-updating, but we can easily adjust these parameters in the
future.
We know that by default the value of `enable_stream_push_notifications` is
true, so on some server if push notifications are disabled then due to
```
{{#unless show_push_notifications_tooltip}}
```
the `enable_stream_push_notifications` checkbox is rendered as unchecked
and when we update any other setting in the "Stream notifications" section,
due to `settings_notifications.update_page()` all checkboxes are changed to
current values which means above checkbox get checked.
We can deal with this bug either making changes in
`settings_notifications.update_page()` but we should show check/uncheck
state of checkboxes anyway so removing this `unless` condition fixes this
bug.
Instead of having logical expressions in templates, it's always preferred
to calculating them in javascript and pass the results as a context. It
also enhances the readability of templates and testing of such logic is
easier in js over templates.
`all_notification_settings_labels` is misleading that this variable is a
list of notifications setting labels so changed it to
`all_notification_settings`.
The reason for extracting this function is that getting the text, integer,
boolean value from the input elements (like checkboxes, dropdowns) is a
common task, and later we can use this function to get the input element
value in `settings_notifications` in the upcoming commit.
This is a bug fix where, if a list_render
object with the given name exists and it's items
have been sorted, then the filtered_list's data
does not get updated on re-rendering.
This line was present in the original commit
9576d5caef.
The use case for this are small or fixed tables, which do not need
filtering support. Thus we are able to not include the unnecessary
search input inside the html parent container.
It is not used at present, but will be required when we refactor
the settings pages.
We also split out exports.validate_filter function for
unit testing the above condition.
As a consequence of too many options in the bottom `Other permissions`
subsection, the `Save` button could end up too far up from the bottom,
such that it might appear offscreen on low-height laptops.
We fix this by reorganizing the settings in a way that is both more
intuitive and also ensures that none of the subsections are too tall.
Fixes: #14274.
Before this commit, the reactions code would
take the `message.reactions` structure from
the server and try to "collapse" all the reactions
for the same users into the same reactions,
but with each reaction having a list of user_ids.
It was a strangely denormalized structure that
was awkward to work with, and it made it really
hard to reason about whether the data was in
the original structure that the server sent or
the modified structure.
Now we use a cleaner, normalized Map to keep
each reaction (i.e. one per emoji), and we
write that to `message.clean_reactions`.
The `clean_reactions` structure is now the
authoritatize source for all reaction-related
operations. As soon as you try to do anything
with reactions, we build the `clean_reactions`
data on the fly from the server data.
In particular, when we process events, we just
directly manipulate the `clean_reactions` data,
which is much easier to work with, since it's
a Map and doesn't duplicate any data.
This rewrite should avoid some obscure bugs.
I use `r` as shorthand for the clean reaction
structures, so as not to confuse it with
data from the server's message.reactions.
It also avoids some confusion where we use
`reaction` as a var name for the reaction
elements.
We've often gotten the complaint that Zulip's emoji are a bit too big;
this should address the worst consequences of that (line-wrapping
being off with large emoji present) while still making it possible to
easily see what a given emoji is.
The right place to change this is in rendered_markdown.scss, not the
main emoji definition in zulip.scss, as the latter is also used in
places like the emoji picker where a larger size is valuable.
Closes#12731, an older PR that did this with slightly different
parameters (and without a comment).
Fixes#14254
You can test this on dev:
* do "-stream:Verona" in the search bar (the minus
sign negates the search here)
* reload the browser
You should see the same search (all streams besides Verona).
We simplify the code for deciding whether
we show a subscribe button or not, and in
doing so avoid a blueslip error where we
were passing `undefined` into `get_sub()`.
We had this API:
people.add_in_realm = full-fledged user
people.add = not necessarily in realm
Now the API is this:
people.add = full-fledged user
people._add_user = internal API for cross-realm bots
and deactivated users
I think in most of our tests the distinction between
people.add() and people.add_in_realm() was just an
accident of history and didn't reflect any real intention.
And if I had to guess the intention in 99% of the cases,
folks probably thought they were just creating ordinary,
active users in the current realm.
In places where the distinction was obviously important
(because a test failed), I deactivated the user via
`people.deactivate`.
For the 'basics' test in the people test suite, I clean
up the test setup for Isaac. Before this commit I was
adding him first as a non-realm user then as a full-fledged
user, but this was contrived and confusing, and we
didn't really need it for test coverage purposes.
We want to move more logic to stream_data to facilitate
testing.
Both before and after this commit, we essentially build a
new list of users for typeahead, but now the new list
excludes subscribed users. We can do even better than
this in a follow-up commit.
Before this commit, presence used get_realm_count()
to determine whether a realm was "small" (and thus
should show all human users in the buddy list, even
humans that had not been active in a while).
The `get_realm_count` function--despite a very wrong,
misleading comment--was including bots in its count.
The new function truly counts only active humans
(and no bots).
Because we were overcounting users before this change,
we should technically adjust `BIG_REALM_COUNT` down
by some amount to reflect our original intention there
on the parameter. I'm leaving it alone for now, though,
since we've improved the performance of the buddy list
over time, and it's probably fine if a few "big" realms
get re-classified as small realms (and show more users)
by virtue of this change.
(Also note that this cutoff value only affects the
"normal" view of the buddy list; both small realms
and large realms will show long-inactive users if you
do searches.)
Fixes#14215
This is a prep commit for the new navbar, since the new navbar switches
between a search bar and stream descriptions, it's easier to have the
border defined in an outer div. Due to the way the changesets is
generated, this may seem like a large diff, however, the only change to
navbar.html is to add an opening div with the ".top-navbar-border" class
and a corrseponding closing div to wrap around "#search_box" and
"#search_box_legacy". Apart from this, a few styles have been edited in
zulip.scss and night_mode.scss.
Punctuate marketing headings with a period. Fix a couple of
title-cased headings to sentense case. Consistently use curly
apostrophes, curly quotation marks, and Unicode ellipses.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Given that can_mark_messages_read is called whenever the blue box
cursor stops on a message and that it is calculated purely on the
basis of sorted_term_types, it makes sense to cache the result.
Previously, when list_render.create was called, if a list_render
object with the given name existed, it returned the existing
list_render object with the previous properties, without the property
to sort the lists added. The root cause of the bug was that when we
added the sorting click handlers, we put them just in the constructor,
not in __set_events, the function we call from appropriate code paths
to add the other necessary click handlers.
Fix this by moving the code to add the sorting properties into
__set_events().
Fixes#14175.
If you were in the "Starred messages" narrow and
your pointer was on a message with the stream/topic
of "social/lunch", we wouldn't move you to the unread
messages for that topic.
I fixed this by removing the code that looked at
the current message's topic. Instead, we only look
at the active narrow to figure out the "next" topic
to go to.
Fixes#14120.
Follow an upstream adjustment to the styling of the vertical
scrollbar (but not the horizontal scrollbar).
https://github.com/Grsmto/simplebar/issues/420
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
The user can pass description along with the task name by splitting the input string with hyphen.
Eg: Task Title - Task Description
todo_list: Add index numbers to task.
Original email address is shown to admin users in subscriber list when
email_address_visibilty is set to "Admins only" by passing delivery_email
at required places. Email address are not shown to non-admin users when
visibility is set to "Admins only".
Tweaked by tabbott to fix a few bugs and dead code.
Fixes a part of #13541.
User IDs are more robust than email addresses as they don't change
with time, and also don't have complications with
different email_address_visibility settings.
This is a common UX pattern for forms - a user would expect the
input to be submitted on hitting enter.
So, create a 'keypress' event listener on the input field for the
new status, which calls 'submit_new_status' on enter key press.
This intent is that we'll be able to reuse this when editing streams
as well.
* Rename method: filter_with_new_topic to filter_with_new_param.
* Fix tests and method calls.
This extends our email address visibility settings to deny access to
user email addresses even to organization administrators.
At the moment, they can of course change the setting (which leaves an
audit trail), but in the future only organization owners will be able
to change that setting.
While we're at this, we rewrite the settings_data.js test to cover all
the cases in a more consistent way.
Fixes#14111.
This updates update the download android and ios app button on
/apps/android and /apps/ios routes respectively to use the official
badges provided by the google and apple.
We also clean up some of the JavaScript implementing the page.
Fixes#14061.
The example regexes for linkifier settings are not themed according
to the user's dark theme setting.
Modify 'night_mode.scss' to render all 'code' elements with dark
theme.
The copy of the styling for users_hover_info:
```
-#users_hover_info {
- left: 25px;
- top: -40px;
-}
```
Looks less good than the common one with #hoverinfo, so we remove it.
Currently, the cursor for the date input field in the settings page
is 'not-allowed' as it has the disabled attribute because we want
users to pick the date from the date picker. But this leads to
confusion whether the field is editable at all.
Change the cursor to 'pointer' to make it clear that the field has
a click action associated with it.
Clicking on the 'Owner' value for a row in the list of bots does
nothing, and causes a blueslip error.
This is because the map object in which we store the users have
integer keys, while we pass the owner id as string.
This is fixed by parsing the owner id to integer before passing it
on.
Fixes#14107.
The file populates `windows.i18n`, so now
the file name matches our convention.
Note that the module really just initializes
`i18next` and then does this:
window.i18n = i18next;
It doesn't really add any functionality to
third party library.
Before 2018, we used a feature of i18next where
we would cache translations in local storage
for up to two weeks:
var cacheOptions = {
// ...
prefix: 'i18next:' + page_params.server_generation + ':',
expirationTime: 2*7*24*60*60*1000, // 2 weeks
};
i18next.init({
/// ...
cache: cacheOptions
}
Because `server_generation` would change each time you
upgraded a server, a frequently upgraded server like
chat.zulip.org would cause its active users to start
to accumulate lots of obsolete key/value pairs in local
storage over the two weeks.
See #4443 for more details.
We eventually reduced the cache life to 2 days. And then
on top of that, newer versions of the server would start
to clean up after themselves using this commit from
April 2017:
e3f1d025ae
We then removed the caching option altogether a year
later in May 2018:
cff40c557b
We kept around the code to remove all the old keys, though.
This was particularly important for users who may have
been hitting servers that did an upgrade to the new
version from some older version that didn't have the
key-fixing code.
But mostly the problem takes care of itself after
either two days or two weeks, even on really out-of-date
servers.
The original problem was most likely to affect server
admins that did a lot of upgrades (and possibly only really
affected chat.zulip.org), so as long as those server
admins continued their patterns, it's highly likely that
they've done several upgrades since May 2018 that would
have cleaned these keys out for good.
And, again, even if there is some strange straggler here,
they probably only have one set of keys that will expire
either two days or two weeks after an upgrade, depending
on how long ago the prior upgrade was. (All of their
keys based on older versions of `server_generation` would
have long since expired.)
Finally, any upgrade certainly won't make the problem
worse for any users under this hypothetical situation,
since the new server won't be writing new keys.
So I am removing the cleanup code.
This extracts a new module with three
functions, which we will test with 100%
line coverage:
- show_email
- email_for_user_settings
- get_time_preferences
The first two break several dependencies
in the codebase on `settings_org.js`. The
`get_time_preferences` breaks an annoying
dependency on `page_params` within people.
The module is pretty cohesive, in terms that
all three functions are just light wrappers
around `page_params` and/or `settings_config`.
Now all the modules that want to call show_email()
only have to require `settings_data`, instead of
having a dependency on the much heavier
`settings_org.js` module.
I also make some of the unit tests here be more
full-stack, where instead of stubbing show_email,
I basically just toggle `page_params.is_admin`.
Users who are using ZulipDesktop or haven't managed to auto-update to
ZulipElectron should be strongly encouraged to upgrade.
We'll likely want to move to something even stricter that blocks
loading the app at all, but this is a good start.
This follows the convention of other code calling into
add_sub_to_table of checking whether the stream settings overlay is
open (and thus in the DOM) before trying to rerender it.
This fixes a bug where you can’t open the same overlay twice in a row
in IE 11, which doesn’t support HashChangeEvent.oldURL; it was exposed
by commit 05be16e051 (late 2018).
While here, parse the hash from oldURL in a less ad-hoc way.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
We add these two functions to the API,
so that we no longer have `alert_words_ui`
using private data from `alert_word`:
alert_words.has_alert_word()
alert_words.get_word_list()
And to initialize the data, we have a proper
`initialize` method that is passed in only
the parameters that it needs from `ui_init`.
(We also move the step of deleting `alert_words`
from `page_params` to the `ui_init` module.)
Because it's a bit less cumbersome to initialize
`alert_words`, we now just it directly in the
node tests for `alert_words_ui`.
This is follow up to da79fd206a
I accidentally skipped over pm_conversations. Same
ideas as the bigger previous commit--we pass in params
to the initialize function and do the delete cleanup
within ui_init.
Calling a function with hundreds of thousands to millions of
arguments, depending on the browser, can throw a RangeError. This was
true of both ids.push(...a) and the [].concat.apply construction that
it replaced in commit 59d55d1e06,
although the old one was less likely to overflow due to bucketing.
Use a loop instead.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This cleans up the handoff of page_params
data between ui_init and modules that
take over ownership of page_params-derived
data.
Read the long comment in ui_init for a bit
more context.
Most of this diff is actually test cleanup.
And a lot of the diff to "real" code is
just glorified `s/page_params/params/`
in the `initialize` functions.
One little oddity is that we don't actually
surrender ownership of `page_params.user_id`
to `people.js`. We could plausibly sweep
the rest of the codebase to just use
`people.my_user_id()` consistently, but it's
not a super high priority thing to fix,
since the value never changes.
The stream_data situation is a bit messy,
since we consume `page_params` data in the
initialize() function in addition to the
`params` data we "own". I added a comment
there and intend to follow up. I tried
to mostly avoid the "word soup" by extracting
three locals at the top.
Finally, I don't touch `alert_words` yet,
despite it also doing the delete-page-params-data
dance. The problem is that `alert_words`
doesn't have a proper `initialize()`. We
should clean that up and have it use a
`Map` internally, too.
This gives them cache-compatible URLs, and also avoids some extra
copies of the sprite sheet images.
Comments on the Octopus emoji added by tabbott.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This is not always a behavior-preserving translation: _.defaults
mutates its first argument. However, the code does not always appear
to have been written to expect that.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This is not always a behavior-preserving translation: _.extend mutates
its first argument. However, the code does not always appear to have
been written to expect that.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This is not always a behavior-preserving translation: $.extend mutates
its first argument. However, the code does not always appear to have
been written to expect that.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Bounce five times, once every 5 seconds, rather than forever every
0.75 seconds. This reduces annoying user distraction and idle CPU/GPU
consumption.
Fixes#13760.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Now the caller simply imports the debug ‘require’ function as a
module, deciding for itself how to expose it and with what name (in
our case, we expose it as ‘require’ with expose-loader). Also, remove
a stray console.log.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
We were incorrectly passing a string version of an integer ID,
e.g. "10", to a function expecting an integer, e.g. 10. Fix this by
using the common get_stream_id function intended for the purpose
rather than hand-written parsing.
This was likely broken in the recent Dict -> IntDict/Map migrations.
We were computing id_of_last_message_sent_by_us
for a valid reason before
fa44d2ea69
was committed in December 2017 to remove the
autoscroll_forever setting.
Since then the only thing that the
conditional for `id_of_last_message_sent_by_us`
short-circuits is a buggy computation of
`id_of_last_message_sent_by_us` itself.
Removing this dead code obviously makes the code
more clear, plus it does save some needless and
possibly bug-prone computation.
In particular, I am trying to lock down `rows.id` to
be more strict about receiving bogus elements, and
removing this code will help with that.
The bug was in complex `if` condition, which should mean that users should
be allowed to create a User group only when they are either admin or user
group creation policy is set to everyone.
Fixes: #13909.
We now no longer do local echo if a user has logged in or visited a
narrow so recently that we are still fetching new messages for them in
their current message list.
Since we want any message list we're displaying to show only
contiguous sequences of messages within that view, it's not correct to
append messages that were just sent at the end unless
fetch_status.has_found_newest shows that we are up to date with the
latest messages from the server.
While we have some logic aimed at correcting our-of-order message IDs
in Zulip, even a brief (few seconds) temporary display of that is a
bug that we should avoid.
This means that we should disable local echo when the user's current
narrow is not up to date. We can be sure that we'll get the message
the user sent from the server either during the catch-up process or
when we receive it back from th server via the events system.
That particular race window can be several seconds in situations where
somebody is in a narrow where their pointer (or equivalent) is far
behind the latest messages.
This commit only fixes the local echo race condition. There's a
related bug where new messages sent by (potentially other) users
delivered to the client via server_events might race with our fetching
until we get the latest messages in a given narrow, which we'll need
to deal with separately.
See https://github.com/zulip/zulip/issues/8989 for more details. It's
possible that we'll close the issue after this fix, since any
additional fixes would add a lot of complexity, and I'm not sure how
much of a problem this will really be in practice after this fix.
Note that we don't have great automated testing for
`try_deliver_locally` (or really `echo.js` in general). For
`try_deliver_locally` the node tests would probably be 8x more complex
than the code itself, since that function is basically "glue" code
touching several external dependencies. It's also kind of hard to
screw up this code without getting pretty obvious failures early in
the QA process.
Fixes#8989.
With the new Map, we want to make sure we
convert the square number into an int.
The symptom here was you'd click on the
square, and the data would get passed
around via the event system, but when
we went to draw the board, the idx value
was a string.
This moves some code from settings_display.js
into the new module settings_config.js.
Extracting this module breaks some dependencies
on settings_display.js (which has some annoying
transitive dependencies, including jQuery).
In particular this isolates stream_data from
from settings_display.js.
Two of the three structures that we moved here
weren't even directly used by settings_display.js,
since we do a lot of rendering in the modules
admin.js and setting.js.
We make get_all_display_settings() a function
to avoid a require-time dependency on page_params.
Breaking the dependencies simplifies a few
node tests.
Most of the node test complexity came from the
following commit in March 2019:
5a130097bf
The commit itself seems harmless enough, but
dependencies can have a somewhat "viral" nature,
where making stream_data depend on settings_display
caused us to modify four different node tests.
Create a new page for desktop auth flow, in which
users can select one from going to the app or
continue the flow in the browser.
Co-authored-by: Mateusz Mandera <mateusz.mandera@protonmail.com>
This refactoring is the first step toward sharing
our markdown code with mobile. This focuses on
the Zulip layer, not the underlying third party `marked`
library.
In this commit we do a one-time initialization to
wire up the markdown functions, but after further
discussions with Greg, it might make more sense
to just pass in helpers on every use of markdown
(which is generally only once per sent message).
I'll address that in follow-up commits.
Even though it looks like a pretty invasive change,
you will note that we barely needed to modify the
node tests to make this pass. And we have pretty
decent test coverage here.
All of the places where we used to depend on
other Zulip modules now use helper functions that
any client (e.g. mobile) can configure themselves.
Or course, in the webapp, we configure these from
modules like people/stream_data/hash_util/etc.
Even in places where markdown used to deal directly with
data structures from other modules, we now use functions.
We may revisit this in a future commit, and we might
just pass data directly for certain things.
I decided to keep the helpers data structure completely flat,
so we don't have ugly nested names like
`helpers.emoji.get_emoji_codepoint`. Because of this,
some of the names aren't 1:1, which I think is fine.
For example, we map `user_groups.is_member_of` to
`is_member_of_user_group`.
It's likely that mobile already has different names
for their versions of these functions, so trying for
fake consistency would only help the webapp. In some
cases, I think the webapp functions have names that
could be improved, but we can clean that up in future
commits, and since the names aren't coupled to markdown
itself (i.e. only the config), we will be less
constrained.
It's worth noting that `marked` has an `options`
data structure that it uses for configuration, but
I didn't piggyback onto it, since the `marked`
options are more at the lexing/parsing layer vs.
the app-data layer stuff that our helpers mostly
help with.
Hopefully it's obvious why I just put helpers in
the top-level namespace for the module rather than
passing it around through multiple layers of the
parser.
There were a couple places in markdown where we
were doing awkward `hasOwnProperty` checks for
emoji-related stuff. Now we use the Python
principle of ask-forgiveness-not-permission and
just handle the getters returning falsy data. (It
should be `undefined`, but any falsy value is
unworkable in the places I changed, so I use
the simpler, less brittle form.)
We also break our direct dependency on
`emoji_codes.json` (with some help from the
prior commit).
In one place I rename streamName to stream_name,
fixing up an ancient naming violation that goes
way back to before this code was even extracted
away from echo.js. I didn't bother to split this
out into a separate commit, since 2 of the 4
lines would be immediately re-modified in the
subsequent commit.
Note that we still depend on `fenced_code`
via the global namespace, instead of simply
requiring it directly or injecting it. The
reason I'm postponing any action there is that
we'll have to change things once we move
markdown into a shared library. (The most
likely outcome is that we'll rename/move both files
at the same time and fix the namespace/require
details as part of that commit.)
Also the markdown code still relies on `_` being
available in the global namespace. We aren't
quite ready to share code with mobile yet, but the
underscore dependency should not be problematic,
since mobile already uses underscore to use the
webapp's shared typing_status module.
This mostly moves logic into people.js.
The people functions added here are glorified
two-liners.
One thing that changes here is that we
are a bit more rigorous about duplicate
names.
The code is slightly awkward, because this
commit preserves the strange behavior
that if 'alice|42' doesn't match on
the user with the name "alice" and user_id
"42", we instead look for a user whose
name is "alice|42". That seems like a
misfeature to me, but there's a test for
it, so I want to check with Tim that it's not
intentional behavior before I simplify
the code.
We add this API to emoji.js, so that markdown
doesn't need to look at internal data structures
(or even need to understand any kind of record
format for results).
Here are the functions:
get_realm_emoji_url()
get_emoji_name()
get_emoji_codepoint()
We use the API now in markdown, which eliminates
the need for the markdown parser to require
the emoji JSON file.
Each function has a simple docstring:
get_emoji_name('1f384') === 'holiday_tree'
get_emoji_codepoint('avocado') === '1f951'
get_realm_emoji_url('shrug') === '/user_avatars/2/emoji/images/31.png'
Also we have simple test coverage for the API
(including tests that verify the docstrings).
This name was misleading, because we weren't
actually setting realm_filters (that's what
`page_params.realm_filters = realm_filters`
is for); we were instead updating our
realm filter rules.
Commit 612b237cec introduced a
regression that broke the “Discard” button, because
get_subsection_property_elements returns a jQuery object rather than
array, and jQuery objects don’t have a forEach method. Change it to
return an array.
[anders@zulipchat.com: Use Array.from instead of .toArray to avoid the
need for extra mocking.]
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
We are gonna phase out util.get_message_topic()
in our entire codebase eventually, but we
certainly don't need it here, since the local
echo codepath is using brand new objects that
we construct inside the compose code, and
there's no danger of legacy "subject" data.
My goal for the markdown code is to keep it
free of any accidental dependencies that we
can easily avoid, as I think there's some
possible future where we split out the code
as its own library for people who want to
render Zulip markdown in non-core projects.
These functions were just shims that were
used in the somewhat painful migration from
subject_* to topic_*.
The commit 4572be8c27
fixed it so that the client never needs to
deal with "subject_links".
So now we just go back to simpler code:
message.topic_links = links
links = message.topic_links
I am not quite ready to declare victory on
the subject/topic migration, but we are super
close. In this commit I bump a blueslip
warning to a blueslip error, so that we'll
be notified of any codepath that is still
using the janky fall-back-to-subject defensive
code here.
If we go a couple days without any errors, then
we can remove the blueslip warning and the
defensive code immediately and then inline
the callers at our leisure. I wouldn't be
wildly against keeping these wrappers in some
parts of the code, but that debate is out of
the scope of this immediate fix, and I haven't
thought hard about it yet.
We can basically sweep set_message_topic() now,
if we wanted to, since it's truly just a one-liner.
(At one point it was encapsulating something
like `message.subject = foo`).
This required a tiny change to compose_fade
test setup.
We now handle the all/everyone/stream case at
the top of userMentionHandler.
Previously the code would do strange things
in the case that some user had the name "all"
or "everyone" or "stream". It would only
affect local echo, and maybe we prevent users
from having those names, so I doubt there
were any real user-facing issues here.
But the new code is clearly more simple and
more correct.
Most of this logic is specific to markdown
message processing, so we move the code to
markdown.js.
The only responsibility that we leave with
`emoji.js` is to provide us with a list
of translations (regex and replacement text).
But now `markdown.js` actually (directly) executes
those translations against Zulip messages
as part of its preprocessing.
This should simplify the upcoming mobile conversion.
Instead of mobile needing to duplicate this fairly
complex function, they will just need to pass
us in a list similar to `emoji_translations` inside
of `emoji.js`. That code has a comment that shows
what the data structure looks like.
There are six emoticon regexes that allow us
make translations such as ":)" to ":slight_smile".
We now build these as soon as we read in the
JSON data, instead of rebuilding them every time
we convert a message to markdown.
It's possible that we should just hardcode this
data:
[
{ regex: /(\:\))/g, replacement_text: ':slight_smile:' },
{ regex: /(\(\:)/g, replacement_text: ':slight_smile:' },
{ regex: /(\:\/)/g, replacement_text: '😕' },
{ regex: /(<3)/g, replacement_text: '❤️' },
{ regex: /(\:\()/g, replacement_text: ':frown:' },
{ regex: /(\:\|)/g, replacement_text: '😑' }
]
OTOH I suppose it's possible that some server
admins will want to modify emoji_codes.json to
have custom emoticons.
I am 99% sure we can rely on trimRight() and
trim() being available in all browsers that
we support. I verified in FF.
This removes the util dependency from both
modules touched here.
We now treat util like a leaf module and
use "require" to import it everywhere it's used.
An earlier version of this commit moved
util into our "shared" library, but we
decided to wait on that. Once we're ready
to do that, we should only need to do a
simple search/replace on various
require/zrequire statements plus a small
tweak to one of the custom linter checks.
It turns out we don't really need util.js
for our most immediate code-sharing goal,
which is to reuse our markdown code on
mobile. There's a little bit of cleanup
still remaining to break the dependency,
but it's minor.
The util module still calls the global
blueslip module in one place, but that
code is about to be removed in the next
few commits.
I am pretty confident that once we start
sharing things like the typeahead code
more aggressively, we'll start having
dependencies on util. The module is barely
more than 300 lines long, so we'll probably
just move the whole thing into shared
rather than break it apart. Also, we
can continue to nibble away at the
cruftier parts of the module.
This generalizes existing code for the presence code path that is
generically useful for avoiding useless work that will be discarded.
We make an exception for the one type of request that needs to happen
while reloading, namely the one to clean up our event queue.
We used to have a block of code doing this just in the presence
endpoint because that's where we'd had error-handling problems with it
not being present, but it seems more correct for it to run
unconditionally on all HTTP requests.
This requires adding a dependency of channel on reload_state, which we
record in the webpack configuration for now.
The actual goal we have is that suspect_offline is correct so that we
can rely on that field when determining how to do error handling in
the presence system.
This should return us to a situation where we won't get blueslip
browser error reporting for users created while a device was offline
just before it reloads.
This avoids risk of logging blueslip errors for user IDs seen in the
presence response that we haven't heard about from the server_events
system because we're offline and in the process of reloading.
The issue only affected large realms; see
02bc630881 and `git log
-Ssuspect_offline` for details.
webpack optimizes JSON modules using JSON.parse("{…}"), which is
faster than the normal JavaScript parser.
Update the backend to use emoji_codes.json too instead of the three
separate JSON files.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
In the next commit we're going to change what the
server sends for the following:
- page_params
- server responses to /json/users/me/presence
We will **not** yet be changing the format of the data
that we get in events when users update their presence.
It's also just a bit in flux what our final formats
will be for various presence payloads, and different
optimizations may lead us to use different data
structures in different payloads.
So for now we decouple these two things:
raw_info: this is intended to represent a
snapshot of the latest data from the
server, including some data like
timestamps that are only used
in downstream calculations and not
user-facing
exports.presence_info: this is calculated
info for modules like buddy_data that
just need to know active vs. idle and
last_active_date
Another change that happens here is we rename
set_info_for_user to update_info_for_event,
which just makes it clear that the function
expects data in the "event" format (as opposed
to the format for page_params or server
responses).
As of now keeping the intermediate raw_info data
around feels slightly awkward, because we just
immediately calculate presence_info for any kind
of update. This may be sorta surprising if you
just skim the code and see the various timeout
constants. You would think we might be automatically
expiring "active" statuses in the client due to
the simple passage of time, but in fact the precise
places we do this are all triggered by new data
from the server and we re-calculate statuses
immediately.
(There are indirect ways that clients
have timing logic, since they ask the
server for new data at various intervals, but a
smarter client could simply expire users on its
own, or at least with a more efficient transfer
of info between it and the server. One of
the thing that complicates client-side logic
is that server and client clocks may be out
of sync. Also, it's not inherently super expensive
to get updates from the server.)
The _.each calls with an inline function expression have already been
converted to for…of loops. We could do that here, but using .forEach
when we’re just reusing an existing function seems like a good
guideline.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This should somewhat reduce the gravity of the failure mode for cases
where the message the user clicked cannot be found (which would be a
significant bug on its own merit in any case).
The keys for message_store are since the recent Map migration intended
to be integer message IDs, not strings (and likely were always
intended to be integers; the failure mode may simply have shifted).
This may just be a new bug, but this max also fix#9549; certainly
we'll want to redo any investigation with this fix in place.
Fixes#9549.
We just get the stream_name from the sub struct now.
This mostly affects node tests.
The only place in real code where we called add_sub()
was when we initialized data from the server.
We now require all of our unit tests to handle
blueslip errors for warn/error/fatal. This
simplifies the zblueslip code to not have any
options passed in.
Most of the places changed here fell into two
categories:
- We were just missing a random piece of
setup data in a happy path test.
- We were testing error handling in just
a lazy way to ensure 100% coverage. Often
these error codepaths were fairly
contrived.
The one place where we especially lazy was
the stream_data tests, and those are now
more thorough.
This saves a tiny bit of bandwidth, but more
importantly, it protects us against races for
stream name changes. There's some argument that
if the user is thinking they're sending to
old_stream_name, and unbeknownst to them, the
stream has changed to new_stream_name, then we
should fail. But I think 99% of the time the
user just wants the message to go that stream
despite any renames.
In order to verify the blueslip error, we
had to turn on error checking, which required
a tiny fix to a place where we left out
a stream_id for add_sub.
We avoid complicated code to update unread counts
by just using vdom.js.
One small change here is that if click on "more
topics", we replace it with the spinner instead
of putting the spinner after it. This saves us
a redraw under the new scheme.
Due to try-catch deoptimization, Babel strict mode for…of loops run
about 5× slower in Firefox than Babel loose mode for…of, native
for…of, or forEach (which are all about the same speed). Chrome
doesn’t seem to care.
For some reason we need to explicitly add the core-js Symbol polyfill
near the beginning of the common bundle. Otherwise it gets loaded at
the wrong time and the Casper tests fail.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Babel strict generates more code for [...x] than you’d like, while
Babel loose mode assumes x is an array.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
We had a plan at some point to use this to display a phone icon or
something for users who would receive push notifications if you
messaged them. IT's not clear that feature was a good idea in any
case, but it certainly shouldn't be synced as presence data; it would
change >100x less often than the rest of presence and so should likely
be synced differently, maybe as a property on user. So it's best to
delete this prototype.
The “Smileys & People” category has been split into “Smilys & Emotion”
and “People & Body”.
Also, fix generate_sha1sum_emoji to read the emoji-datasource-google
version from yarn.lock, since package.json only gives a version range.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
When quoting a message with fenced code blocks without a language,
we used to have ambiguity in which '```' fence terminates the quote.
This commit adds explicitly non-interfering fences, which fixes the
above issue as well as makes the raw message easier to quickly read.
Fixes#12446.
This commit includes a new `stream_post_policy` setting,
by replacing the `is_announcement_only` field from the Stream model,
which is done by mirroring the structure of the existing
`create_stream_policy`.
It includes the necessary schema and database migrations to migrate
the is_announcement_only boolean field to stream_post_policy,
a smallPositiveInteger field similar to many other settings.
This change is done to allow organization administrators to restrict
new members from creating and posting to a stream. However, this does
not affect admins who are new members.
With many tweaks by tabbott to documentation under /help, etc.
Fixes#13616.
This fixes the buggy behavior for streams which inherits the notification
setting from UserProfile, and are actively opened in "Streams > Stream
settings", if a user has opened two browser windows, and changes the
notification setting from "Settings > Notifications", then the changes
don't reflect such "Streams > Stream settings" notification setting
checkboxes for such stream.
Partially fixes: #12304.
Here we have attached our handler to `.sub_setting_checkbox` so
`e.currentTarget` will return element with class `.sub_setting_checkbox`
but `e.target` will return exactly which element we have clicked, which
could be a child of `.sub_setting_checkbox`. So instead of,
```
$(e.target).closest(".sub_setting_checkbox")
```
we can use
```
$(e.currentTarget)
```
which is more clean and intuitive.
- `e.currentTarget` is less popular which could be the reason behind using
two step hack to get the targetted element.
Rather than defining two different jquery event-handlers for two different
events, we can use a single jquery handler as the function is the same for
both handlers.
Since it took a lot of effort to debug the original issue that caused
us to introduce suspect_offline, it seems worth writing a comment
explaining why we won't see that issue here.
We now use user_ids for presence, so we don't need
to worry about races related to unknown emails
being sent to us. Now we just update the data
structure based on user_id, and
it will be there when we render the presence
widget for that user_id, or else it will
simply be ignored.
It's not clear to me whether we still need
dont_block here, so I didn't touch that code.
Here is the commit that added the suspect_offline
flag, for easy reference:
f207450cdb
This flag affects page_params and the
payload you get back from POSTs to this
url:
users/me/presence
The flag does not yet affect the
presence events that get sent to a
client.
If you look at info_for, it clearly never returns
`undefined`, so this defensive code isn't preventing
any bugs.
Also, we are doing a better job now of filtering
user_ids in upstream code.
This is defensive code for the scenario that we
have a user_id in presence but not people. This is
unlikely to occur by the time that we actually render
the buddy list, which is the context for this code.
We have previously been reporting an error here via
the people code, but we add an additional warning.
Also, we filter the user_id from the result.
This reverts commit d84646f091 (which
incorrectly assumed in unread_topic_counter that the messages were
present in the message store), while fixing the type confusion problem
by using IntDict for stream_id keys.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Fixes “TypeError: sourceContent.split is not a function” at
blueslip_stacktrace.ts:60 when there’s another error during page load.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
In the future, any property which doesn't have any dependent setting can be
added to `simple_dropdown_properties` list, which automates setting the
value of dropdowns on saving.
Earlier, on narrowing the window to some particular sizes,
long stream names used to overlap with the subscribe and view stream
buttons.
The issue was resolved by cutting the stream name short and putting
ellipses at the end. A title was provided to the stream name div so that
the entire stream name would be visible on hovering over it.
Fixes: #13139
Fixes type confusion in unread_topic_counter, which uses stream IDs as
keys.
Since unread_topic_counter calls message_store.get now, update the
mocks so that message_store.get knows about our mocked messages.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Previously the sender was not included in display_recipient when
a private message was locally echoed. This broke the copy conversation
link functionality, if the user try to copy the link immedeatly after
sending the message. This issue is present only during local echo.
This was fixed by including the recipient of the user during
local echo.
Fixes#13547.
Edited the warning to clearly state that most members/most stream members
will be notified on using wildcard mentions, along with the specific
mention (e.g. @ALL, @everyone and @stream).
Did a separate check for all wildcard mentions in util.js and stored the
corresponding mention in wildcard_mention inside compose.js.
Fixes: #13636
This change is in series of de-duplication of code in "Other permission"
section for various dropdowns.
Here rather than using "by_anyone" and "disabled" for the `value` attribute
of options, we use actual numeric values. As a result, we don't need to
manually handle to extract the data to be sent to the backend on saving.
This change is in series of de-duplication of code in "Other permission"
section for various dropdowns.
Here rather than using "by_admins_only" and "by_admins_only" for `value`
attribute of options, we use actual numeric values. This helps in
de-duplicating lot of code which is vulnerable to bugs.
For few settings like `waiting_period_threshold` it makes sense to have the
"value" attribute of option to have a value other than the actual setting
value because multiple settings are depending upon this dropdown, so
handling them in JS code makes more sense. But for many settings (which has
integer values), we have followed a wrong trend over the time of
representing every new dropdown with human-readable values and manually
handling them in JS Code, where it makes more sense to use actual setting
value. The result of which is code has become less concise, sensible and
less likely to be mistaken.
This is a preliminary commit for upcoming change where we will use
"bot_creation_policy_values" like approach for many other settings where
dropdown represents the only single setting of integer type.
We now use vdom-ish techniques to track the
list items for the pm list. When we go to update
the list, we only re-render nodes whose data
has changed, with two exceptions:
- Obviously, the first time we do a full render.
- If the keys for the items have changed (i.e.
a new node has come in or the order has changed),
we just re-render the whole list.
If the keys are the same since the last re-render, we
only re-render individual items if their data has
changed.
Most of the new code is in these two modules:
- pm_list_dom.js
- vdom.js
We remove all of the code in pm_list.js that is
related to updating DOM with unread counts.
For presence updates, we are now *never*
re-rendering the whole list, since presence
updates only change individual line items and
don't affect the keys. Instead, we just update
any changed elements in place.
The main thing that makes this all work is the
`update` method in `vdom`, which is totally generic
and essentially does a few simple jobs:
- detect if keys are different
- just render the whole ul as needed
- for items that change, do the appropriate
jQuery to update the item in place
Note that this code seems to play nice with simplebar.
Also, this code continues to use templates to render
the individual list items.
FWIW this code isn't radically different than list_render,
but it's got some key differences:
- There are fewer bells and whistles in this code.
Some of the stuff that list_render does is overkill
for the PM list.
- This code detects data changes.
Note that the vdom scheme is agnostic about templates;
it simply requires the child nodes to provide a render
method. (This is similar to list_render, which is also
technically agnostic about rendering, but which also
does use templates in most cases.)
These fixes are somewhat related to #13605, but we
haven't gotten a solid repro on that issue, and
the scrolling issues there may be orthogonal to the
redraws. But having fewer moving parts here should
help, and we won't get the rug pulled out from under
us on every presence update.
There are two possible extensions to this that are
somewhat overlapping in nature, but can be done
one a time.
* We can do a deeper vdom approach here that
gets us away from templates, and just have
nodes write to an AST. I have this on another
branch, but it might be overkill.
* We can avoid some redraws by detecting where
keys are moving up and down. I'm not completely
sure we need it for the PM list.
If this gets merged, we may want to try similar
things for the stream list, which also does a fairly
complicated mixture of big-hammer re-renders and
surgical updates-in-place (with custom code).
BTW we have 100% line coverage for vdom.js.
We mostly needed this for Casper tests, and that
usage was eliminated in the prior commit.
There was also some strange defensive code from
ecc42bc9f8 that
is really ancient and which I am eliminating:
const email = row.attr("data-email");
if ($("#deactivation_user_modal .email").html() !== email) {
blueslip.error("User deactivation canceled due to non-matching fields.");
ui_report.message(i18n.t("Deactivation encountered an error. Please reload and try again."),
$("#home-error"), 'alert-error');
}
If the code was there to protect against live
updates for email changes, then we no longer
have to worry about that, since we use user_ids
now as keys.
Or it might have to do with some ancient bug
where you could pop open two modals at once
or something. You can actually change users while
the modal is open (which is kinda strange, but ok),
and it works fine.
When testing this, I ran into the glitch that we
don't open redraw the Deactivated Users panel after
going into the User panel and deactivating a user.
Now that we have the type situation of having anchor support passing a
string, this is a much more natural way to implement
use_first_unread_anchor.
We still support the old interface to avoid breaking compatibility
with legacy versions of the mobile apps.
This makes the code more readable, by just passing the anchor through
without changing its field name back and forth.
There's no reason for this parameter to involve parsing and integer --
it should be a number in all incoming code paths.
The feature is used for editing stream descriptions as well, and in
any case, what's important is that it's a content-editable widget (aka
a form of input box).
This fix recently went on master, although it
hasn't actually been deployed yet (not even to czo),
so user impact should be zero:
0fa67c84d8
The fix mostly improved things, but it broke the
logic for pill containers. The symptom was that
if you tried to autocomplete "Cordelia" in the
pill box we'd instead invoke the "c" hotkey and
try to compose to a stream.
In templates we determine checkboxes are disabled by using the following
`if` clause,
```
{{#if (or (and is_muted notification_setting) realm_setting_disabled)}}
disabled="disabled"
{{/if}}
```
and it is more intuitive to do such calculation in javascript code, so we
added an `if_disabled` attribute in `settings` context which replaces
logical operations from `if` statement.
So for non-notification settings, it is
```
is_disabled: check_realm_setting[setting]
```
where check_realm_setting[setting] is same as realm_setting_disabled.
and for notifiaction settings it is,
```
ret.is_disabled = check_realm_setting[setting] || sub.is_muted;
```
Profiles of typing in the Zulip webapp's compose box after opening the
stream creation widget showed that hotkey.processing_text was a
significant expense. There's no good reason for this -- the function
just needs to inspect the focused element; it just was written with a
sloppy selector.
While there's a secondary issue that, there's no good reason for this
extremely latency-sensitive code path (typing an additional character)
to be doing something extremely inefficient.
We now give "slight smile" precedence over
"small airplane" if you type "sm".
More generally, we favor popularity over prefix
matches for emoji matches, as long as the popular
emoji matches on any of its pieces.
I removed a slightly confusing code comment, which I
will address in a follow up commit. Basically,
"slight smile" still doesn't win over "small airplane"
when you search for "sm", which kind of defeats the
purpose of having popular_emojis for the typeahead
use case. This is a problem with sort_emojis, though,
so when the comment was next to the list of popular
emojis, it wasn't really actionable.
We are sweeping the codebase to use startsWith
when possible. I kept this on a separate
commit due to us vendoring the library, just
to reduce some noise.
Using startsWith is faster than indexOf, especially for long strings
and short prefixes. It's also a lot more readable. The only reason
we weren't using it was when a lot of the code was originally written,
it wasn't available.
We only convert the query to lowercase outside the
loop for an Nx speedup, where N = number of items.
And then we use startsWith instead of indexOf, which
means we don't senselessly search entire strings
for matches.
(We've had startsWith polyfills for a while now.)
Unfortunately, unless a string start with the
exact casing of the query, we still create an
entire lowercase copy of the string for the case
insensitive match. For the English use case
(and many other languages), we could further
optimize this by slicing the string before
converting it to lowercase.
Unfortunately, you have languages like German
with the straße/STRASSE problem. It's not clear
to me how we handle them with the current code,
but I don't want to break that yet.
We use the nice es6 syntax to create the get_item
helpers (in the callers and for the default
value in the function).
Also we use better es6 style for the looping.
This extracts get_emoji_matcher and all the
functions it depended on, most of which were
in composebox_typeahead.js.
We also move remove_diacritics out of the people
module.
This is the first major step for #13728.
Adding invited users to the notifications stream unconditionally isn't
a correct behaviour for guest users, where the previous behavior of
including the notifications stream no longer makes sense. Therefore,
while inviting a new user, the notifications stream is listed along
with other streams with a message "recieves notifications for new
streams" in order to distinguish it from other streams.
Fixes#13645.
We used to put the user's email in a value, which was
redundant (we could find the value from
our parent's label) and brittle (would break
on email changes).
Now the DOM's a bit slimmer and more robust.
Also note that we now deal with user_ids, not emails,
in the call stack until we hit the "edge" and convert
to emails for the server.
This fixes some harmless type errors from the
following commit:
6ec5a1f306
The IntDict code automatically converts strings to
integers, so this was not a user-facing problem, but
we want to have our callers do the conversions
explicitly.
This legacy cross-realm bot hasn't been used in several years, as far
as I know. If we wanted to re-introduce it, I'd want to implement it
as an embedded bot using those common APIs, rather than the totally
custom hacky code used for it that involves unnecessary queue workers
and similar details.
Fixes#13533.
When a user clicked the current emoji format in "display settings",
we'd show an infinite loading spinner (basically as a side effect of
trying to tell the server to change the emoji format to what it
already was).
Fix this by aborting early if the emoji format is already the option
that the user clicked.
Fixes#13684.
We now only go the server if both of these
conditions are true:
- our message data seems incomplete for
the stream
- we haven't already fetched history
This function will make more sense when we start
tracking api calls that retrieve topic history.
The unit tests here are kinda duplicating what we
have in the stream_data tests. If we move the
function out of stream_data, we can kill off the
tests there, but for now I think a bit of duplicate
testing is fine here.
All the callers seem to have integer stream_ids
already, either from the message object or
some sub object.
We also use clear() inside the test-only reset()
method.
Previously, links to deleted streams would be incorrectly rendered as
stream's name).
Fixes an issue that was reported where after deleting the "general"
stream, the welcome turtle messages might appear as links to
This is mostly for tactical reasons. It's hard to
get 100% test coverage on topic_list.js, but it
should be easy to get 100% test coverage on this
very important function.
I considered just moving this code into topic_data.js,
but it just didn't feel quite right. I feel like
this is a pretty core piece of code that's nice
to be by itself and not be near other complicated
code that does stuff like build widgets or talk
to servers. (And, again, it's not just the actual
code here, which is pretty small, it's the unit
tests, which are inherently verbose to exercise
all the edge cases.)
There was an edge case with the old
code when you had exactly between 6 and 8
topics and all in cache, with a couple of
the topics being unread.
We would show "more topics" when you were
actually seeing all your possible topics.
To test this:
- create 7 topics on Venice
- as Iago, narrow to any of the Venice
topics
- as Aaron, send unreads to 3 or 4
of the other topics
Eventually Iago will have all possible
topics in the sidebar. On master we'll
show "more topics", whereas after this commit
we correctly avoid that.
It's a pretty harmless bug, since it just
leads to a useless zoom-in.
I have always felt we should zoom-in
regardless of how many topics you have,
just for consistency sake, but I also
understand the rationale behind our
current intentions.
This is basically trying to confine the
rendering logic to a smaller function,
since I want to work toward a better
approach for redrawing the topic list.
Also, since the new function is now
purely data-oriented, it will be a
bit easier to test various edge cases.
If you clicked for no more topics and then the server didn't find any,
we once had code that would say "No more topics" in light gray at the
bottom of the topic list.
The feature appears to have been broken by some detail in the
`self.dom` refactoring. More importantly, it's not clear it's useful
as opposed to clutter.
Since we added the `stream.first_message_id` feature, it's now very
rare for the `more topics` option to appear when there aren't in fact
older topics that could be fetched. In cases where there are not, the
UI is still clear about what's happening -- it shows a loading
indicator and then displays a list of topics that doesn't have
anything new.
So we're removing this feature; we can re-add it without too much
difficulty if user feedback in the future suggests it would be useful
after all.
The only place we ever set active-sub-filter is
right after we build the template, so there is
no reason to have it be a separate step.
(I made a similar fix to pm_list recently, and
this helps set the stage for doing vdom-like
stuff.)
The previous logic was a bit byzantine, making a lot of inferences
based on which conditionals had already been processed that made it
hard to read. This simple function approach promises to be more
readable.
This is for consistency with how we show unreads in muted topics at
the stream level, avoiding distracting users with the appearance of
unread messages in muted topics that they've made clear they are not
interested in.
Arguably, we should show a faded count if there are unreads on muted
topics (but none on unmuted topics), but that seems somewhat complex
to maintain, and we'd benefit from user feedback to make an effective
decision on whether it'd be an improvement.
Fixes#13676.
I think this probably matches users' expected behavior that muted
streams shouldn't get in their way unless the user is actively looking
for them. If a user has a lot of muted topics with active traffic
(e.g. because topics corresponding to channels in a mirrored Slack
instance), they would previously find their 5 slots cluttered with
those muted topics even if there were unmuted topics with unread
messages.
Fixes#13677.
We may revisit this in the future, but similar to is:private, the
current Zulip user experience makes users expect that in the
is:mentioned view, they should really be able to mark messages as
read.
Further, the practice use case for not marking them as read is very
low, since it's rare for someone to have so many mentions that
revisiting the mentions view isn't sufficient to see everything that
needs their attention.
Previously, is_exactly() had already been repalced with can_bucket_by().
This commit removes is_exactly() and replaces its usage in our tests
with can_bucket_by().
For Manage Streams, when we render the subscriptions
template, a significant amount of time is taken
by the "t" helper.
Obviously for the first call, we expect "t" to be
somewhat expensive, but subsuquent calls should be
fast, but i18next seems to have some overhead.
Also, we can save a tiny bit of overhead (marking it
as a safe string) that comes from our helper.
As an aside, are we sure it's ok to mark translations
as safe strings?
To test before and after, use blueslip.timings before
and after this commit. When I tested with about 300
streams, the difference is pretty striking:
without cache: 100ms
with cache: 20ms
This is particularly interesting, since the subscriptions
templates have long strings for things like the SVG-based
checkmarks, but they're not really the bottleneck.
Unfortunately, this doesn't seem to be a huge win
elsewhere. In some places we don't call "t", but of
course those might change in the future and benefit from
the cache. And in other places we have smart widgets
that avoid rendering all N objects at one (e.g. buddy
list and list_render).
So this might be too big a hammer to speed up one
screen (albeit a really slow one). It's possible
that we should simply move the i18n.t step **outside**
of certain templates to avoid doing them in a loop.
We now incorporate people.get_message_people() in our
logic for compose/PM typeaheads. This not only gives
users better results in some cases, but it will also
improve performance for large realms in some cases.
We'll use this in two places coming up, so it's
worth extracting, plus I wanted to add the
fairly lengthy comment here. (Tim, feel free
to edit down the comment as you see fit).
This is relatively unobtrusive, and we don't send
anything to the server.
But any user can now enter blueslip.timings in the
console to see a map of how long things take in
milliseconds. We only record one timing per
event label (i.e. the most recent).
It's pretty easy to test this by just clicking
around. For 300 users/streams most things are
fast except for:
- initialize_everything
- manage streams (render_subscriptions)
Both do lots of nontrivial work, although
"manage streams" is a bit surprising, since
we're only measuring how long to build the
HTML from the templates (whereas the real
time is probably browser rendering costs).
This change sets us up to optimize how we
filter users in the admin user settings.
See #13554 for more context on the user
facing issues.
This fix is basically three related things:
- Add filterer options to list_render.
- Add helper method to people.js.
- Use filterer in settings_users.js.
The filter "callback" was only a "callback" in the
most general sense of the word.
It's just a filter predicate that returns a bool.
This is to prepare for another filtering option,
where the caller can filter the whole list
themselves. I haven't figured out what I will name
the new option yet, but I know I want to make the
two options have specific names.
We are already providing callbacks everywhere, so
it would be nice to eliminate some dead code.
This also speeds things up ever so slightly (no
longer type-checking the option every time through
the loop).
We also split out exports.filter to make unit testing
easier. The function seems kinda silly now, being so
small, but I hope to add another filtering option soon.
It's a bit confusing when you read this code to know
where the original list was created. I'm not a huge
fan of the cache scheme here, but it does seem to
work for live updates.
Zulip has had a small use of WebSockets (specifically, for the code
path of sending messages, via the webapp only) since ~2013. We
originally added this use of WebSockets in the hope that the latency
benefits of doing so would allow us to avoid implementing a markdown
local echo; they were not. Further, HTTP/2 may have eliminated the
latency difference we hoped to exploit by using WebSockets in any
case.
While we’d originally imagined using WebSockets for other endpoints,
there was never a good justification for moving more components to the
WebSockets system.
This WebSockets code path had a lot of downsides/complexity,
including:
* The messy hack involving constructing an emulated request object to
hook into doing Django requests.
* The `message_senders` queue processor system, which increases RAM
needs and must be provisioned independently from the rest of the
server).
* A duplicate check_send_receive_time Nagios test specific to
WebSockets.
* The requirement for users to have their firewalls/NATs allow
WebSocket connections, and a setting to disable them for networks
where WebSockets don’t work.
* Dependencies on the SockJS family of libraries, which has at times
been poorly maintained, and periodically throws random JavaScript
exceptions in our production environments without a deep enough
traceback to effectively investigate.
* A total of about 1600 lines of our code related to the feature.
* Increased load on the Tornado system, especially around a Zulip
server restart, and especially for large installations like
zulipchat.com, resulting in extra delay before messages can be sent
again.
As detailed in
https://github.com/zulip/zulip/pull/12862#issuecomment-536152397, it
appears that removing WebSockets moderately increases the time it
takes for the `send_message` API query to return from the server, but
does not significantly change the time between when a message is sent
and when it is received by clients. We don’t understand the reason
for that change (suggesting the possibility of a measurement error),
and even if it is a real change, we consider that potential small
latency regression to be acceptable.
If we later want WebSockets, we’ll likely want to just use Django
Channels.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Currently, if we change stream we see the immediate saving of stream, but
it is more convenient to have "Save" and "Discard" buttons as we use
everywhere else in the organization setting subsystem.
This is a preliminary commit for further commits where we will be using the
newly created function `save_discard_widget_status_handler` in click
handler for changing the notification stream.
This refactors `discard_property_element_changes` and
`check_property_changed` function to move conditional statements of
properties that need to be handled separately. It's a preliminary commit in
the series of using save/discard widget for notification stream setting.
As the part of making notification stream settings to change using
"save/discard" widget instead of immediate saving, we need to access the
stream id which is being selected at the moment.
(This is another preliminary commit in the direction of having
"save/discard" widget show up rather than saving immediately.)
The code for selecting and processing the stream for both types of
notifications is almost the same, so de-duplicated.
For "New stream notifications" and "New user notifications" it is more
intuitive to just use the new system for showing success/saving status
feedback.
This fixes the error where we pass `user_id` of 'string' type as the
argument instead of 'integer' to `exports.get_person_from_user_id` which
further passes `user_id` to InDict.has() function which accepts integer
argument only.
Extracting the function makes it a bit easier to
test and use in a generic way.
Also, I wanted this to live in stream_data, so that
it's easier to find if we change how we model
subscriber data.
Finally, I use _.every to do the subset check
instead of `_.difference`, since _.difference
is actually N-squared:
_.difference = restArguments(function(array, rest) {
rest = flatten(rest, true, true);
return _.filter(array, function(value){
return !_.contains(rest, value);
});
});
And we don't actually want to build a list only
to check that it's zero vs. nonzero length.
We now do this, which short circuits as soon
as it finds any key that is only in sub1:
return _.every(sub1.subscribers.keys(), (key) => {
return sub2_set.has(key);
});
First, there are no more convoluted signals.
We also simplify the parameter to just the "mentioned"
object corresponding to either a user or a broadcast
mention.
For the user group scenario, this has always been dead
code, which you only realized when you got to the comment
at the bottom. Now we actually do nothing.
And I moved the relevant commment to the
the typeahead code (with new wording).
I also moved the is_silent check to the caller. I don't
feel too strongly about that either way. It's kind of silly
to call a function only to give that function an additional
responsibility to worry about. On the other hand, I see
the logic of that function enforcing everything. I went
with the former for now.
Arguably we should have a warning for silent mentions,
since doing a silent mention of somebody not on a stream
is a good indication of a typo. I do understand the use
case, but the user can always ignore the warning. Anyway,
we have decent test coverage on this.
This isn't really an extraction; it's more giving
a name to an anonymous function and moving it to
higher module scope.
We convert this to an ordinary function call, which
allows us to move it out of intialize().
Since there's just one simple parameter now (linked_stream),
we can avoid some error checking.
We also avoid the comment that describes the function,
since it now has a name.
And then one minor tweak is to do the inexpensive
`invite_only` higher in the function. This will be
a nice speedup when you link to really large public
streams.
The unit tests are also a bit easier to read now--less
setup and more explicit names.
This is easier to read and faster, because it
avoids some unnecessary encoding on the pm-with
part, plus just a lot of extra logic that amounts
to just appending the slug.
Performance for this function is relevant because it is used
for every user every time we rerender the right sidebar.
Previously, if you tried to invite a user whose account had been
deactivated, we didn't provide a clear path forward for reactivating
the users, which was confusing.
We fix this by plumbing through to the frontend the information that
there is an existing user account with that email address in this
organization, but that it's deactivated. For administrators, we
provide a link for how to reactivate the user.
Fixes#8144.
This experimental setting disables sending private messages in Zulip
in a crude way (i.e. users get an error when they try to send one).
It makes no effort to adjust the UI to avoid advertising the idea of
sending private messages.
Fixes#6617.
The sort_recipients helper is used for many different
typeaheads, such as compose PMs, compose mentions,
and some settings-related code.
We now avoid unnecessary sorting steps in cases
where we have plenty of results in the top buckets
(such as users who match on prefix).
This change should not have any user-facing
implications.
This method is a bit complex, but I think it's
worthwhile to force PM autocompletes and mention
autocompletes through the same code path.
We also kill off this method:
typeahead_helper.sort_people_and_user_groups
This fixes some regressions from a recent
commit that might not have been deployed.
9f7be51ce8
Even if that change had been deployed, it
should not have been user-facing, but it
would have spammed us with blueslip errors
every time somebody used the stream/topic
popovers in the left sidebar.
There's no reason any more to have a single function
filter both persons and groups. Instead, we just
filter each cohort with a more direct function.
This is a minor performance speedup (avoiding the
conditional in the loop), but I mostly wanted to
simplify the code.
The sort_people_and_user_groups function's only
value-add over sort_recipients is to split out
groups and users, but the caller already had
them split out, so it was kinda silly to concatenate
them back together.
I doubt this was a dumb decision at the time; I think
it was probably a consequence of how bootstrap's
normal approach is kinda inflexible when you're
using typeahead to pull data from multiple sources.
I wanted to kill off sort_people_and_user_groups
completely, but the mention/silent_mention autocompletes
are still a bit awkward to refactor.
For historical reasons pm_list was handling just
one possible edge case of where is:private was
combined with other search terms, namely the
pm-with operator.
The code was correct in realizing the is:private
was redundant there, but now we handle that
upstream in Filter.fix_operators (see previous
commit).
Now we just look for any is:private term.
This change makes these two functions more alike:
- get_search_result
- get_search_result_legacy
To test the UI modify zerver/views/home.py by
replacing `settings.SEARCH_PILLS_ENABLED` with
`True`. I only did a quick sanity check, since
any bugs with the new system are more likely due
to bitrot than any changes I have made here.
The history is this:
Tim cloned the code (before the smaller
helpers were extracted):
db4f6e278f
In 8b153f6452
Shubham removed get_operator_subset_suggestions but
accidentally left a `concat` statement in that got
misapplied to the previous suggestions:
- suggestions = get_operator_subset_suggestions(operators);
result = result.concat(suggestions);
The error there was carried over in some recent changes,
but this commit fixes that strangeness.
In 73e4f3b3fa
Shubham made this change, which makes sense only for
pills, and this code remains intact.
- if (operators.length > 0) {
- last = operators.slice(-1)[0];
+ if (query_operators.length > 0) {
+ last = query_operators.slice(-1)[0];
+ } else {
+ // If query_operators = [] then last will remain
+ // {operator: '', operand: '', negated: false}; from above.
+ // `last` has not yet been added to operators/query_operators.
+ // The code below adds last to operators/query_operators
+ operators.push(last);
+ query_operators.push(last);
}
Mohit made a couple changes to both old and new.
Anders made a couple non-substantive changes related to
the ES6 migration.
Steve (me) made several structural changes to the code. For
some of them I only changed the legacy code, not the pills
code. I didn't fix Shubham's mistake until this change.
Now the two functions should look similar except in the places
where they are intentionally different. I also added a comment
explaining the get_operator_subset_suggestions difference.
Fixes#13609
There may be a deeper issue that various JavaScript logic expects
every message to have a `.message_content` element, but we definitely
should have the `.rendered_markdown` class on any markdown content.
Fixes#13634.
The reason we use functions here will be clear
in the next change.
This is a "prefactor" commit that doesn't change
any user-facing behavior nor significantly change
performance.
A few reasons to extract it:
- we can shorten lines (and not repeat query
every time)
- we can scope the big block comment explaining
why util.prefix_sort is a strange name
- the name is better (it's an O(N) operation that
mostly partitions)
- we may want to swap it out with a true partition
function that's truly a partition (since the
case checks done by prefix_sort are possibly
either a non-feature or mostly overridden by
the other sorts)
The recip.id || recip.user_id idiom has only been
needed for some old unit tests.
It was previously required as a bad workaround for the
local echo issue fixed in dd1a6a97bd
where we would get `display_recipient` values added in an invalid format.
I want to be able to easily test this without
having to simulate all the jQuery side effects.
This simply preserves the old logic, which seems
to handle one edge case without handling every
possible edge case. The edge cases aren't super
important here, though, since the only thing it affects
is bolding "Private Messages", and when to do that
is somewhat up to personal tastes.
Having said that, we could definitely improve
this code and possibly should move some of this
logic to either narrow_state.js or filter.js.
Instead of doing various ad-hoc calculations of
which PM is "active" and plumbing it through various
functions and then updating it via jQuery instead of
just the template, we now just calculate `is_active`
in `_build_private_messages_list` with a little
helper function.
In 3cfc3ca24b I removed
the feature that limited PM conversations to five or
less (including the active conversation), but I
didn't clean up this parameter. I think lint was
confused by the fact that we did mutate it.
I am wondering if this started out as an experiment
and was never fully polished before the push? Or
maybe I was just careless. Anyway, I don't
think were any symptoms here--it was just dead code
that we didn't need.
This fixes a rebase issue between the int_dict introduction and use
for people.js with the introduce of filter_values on dict.js and use
inside people.js.
Note that we haven't fully swept this for Dict,
since some dicts are keyed by strings. For
example PM counts can have a huddle like
"101,102,103" as a key.
This should be slightly more performant, and we
often call this function N times, such as when
rendering the buddy list.
There's a minor change to pm_list to avoid
an unnecessary computation on huddles that would
otherwise trigger a blueslip warning for the
huddles case.
Once we get past the special check for fake
person objects already having `pm_recipient_count`,
we can rely on the object being a `person`
object with `user_id` set.
When we are pulling data from message.display_recipient
for private messages, the user_id field is always
called 'id', not 'user_id', so we can simplify
some defensive code.
This required lots of manual testing:
- search/navigate user presence
- send PM and mention user
- pay attention to compose fade
- send stream msg and mention user
- open Private Messages in top-left and click
- test unread counts
- invite user who already has account
- search for users in search bar
- check user settings
- User Groups
- Users
- Deactivated Users
- Bots
- create a bot
- mention user groups
- send group PM then click on lower right
- view/edit/create streams
If there are still pieces of code that don't convert
ids to ints, the code should still work but report
blueslip errors.
I try to mostly convert user_ids to ints in the callers,
since often the callers are dealing with small amounts
of data, like user ids from huddles.
We only ever show 3 or 4 people in search suggestions
(possibly w/a couple variations, like pm-with/sender/etc.),
so we can try to search a smaller subset of people
before going through the entire realm.
We use message_store.user_ids() for this, since you
typically want to search messages for people that
have sent messages recently, and we already sort
based on PM conversations.
This should avoid some memory allocations.
We also use build_person_matcher to avoid
repeating the same logic over and over
again to process the query into termlets.
We also remove people.get_all_persons() and
people.person_matches_query().
This may actually be a slowdown for the worst case
scenario, but it sets us up to be able to easily
short circuit the removal of diacritic characters
for users that have pure ascii names.
For example, czo has lots of names like this:
- Tim Abbott
- Steve Howell
Since they're pure ascii, we can do a one-time
check. A subsequent commit will show how we use
this.
This looks like simple code cleanup, but it's more
than that.
The code cleanup here is that we don't have three
callbacks to get a list of typeaheads for bootstrap.
Instead, we just have one function that does all the
main work.
And then the speedup comes from the fact we no longer
need to remove diacritics from the query for every
time through our loop of seeing if a person matches
the query.
It's a bit subtle to see in the diff, but these are
the relevant lines:
const matcher = exports.get_person_or_user_group_matcher(query);
const filtered_results = _.filter(people_and_groups, matcher);
Before this, bootstrap was doing $.grep, and we'd have
to reinitialize the matcher for every person.
If you profile this before and after, you'll see that
remove_diacritics gets called fewer times.
To profile this, you want to loads lots of users into
your DB and try to autocomplete "Extra", as in "Extra1 User".
If you try to autocomplete something else, then my patch
won't really help, and `remove_diacritics` will still
show up as expensive. Because it is that expensive a function.
These had to be done in tandem, since they were
both kinda coupled to the function that is now
called query_matches_name_description.
(This commit slightly negatively impacts PM
lookups, but this is addressed in the subsequent
commit, which makes PMs much faster. The impact
is super minimal--it's just an extra function
dispatch.)
This may seem silly now, since we are returning a function
that still dispatches over all flavors of search for
every item, but subsequent commits will make it obvious
why I'm doing this.
We want to do our own matching of items, rather than
just giving a callback to bootstrap, which does $.grep
on all the items.
Doing our own matching gives us flexibility for future
improvements like custom data structures for searching
through big amounts of data. Even in the short term
we can speed up searches by pulling expensive operations
outside the grep/filter call.
This architecture has been in place for our search
bar since ~2014.
We have ~5 years of proof that we'll probably never
extend Dict with more options.
Breaking the classes into makes both a little faster
(no options to check), and we remove some options
in FoldDict that are never used (from/from_array).
A possible next step is to fine-tune the Dict to use
Map internally.
Note that the TypeScript types for FoldDict are now
more specific (requiring string keys). Of course,
this isn't really enforced until we convert other
modules to TS.
We had a potentially nasty bug where we
weren't guaranteeing that all/stream/everyone
collated in consistent ways inside of
`compare_people_for_relevance`, which can
send certain types of sort algorithms into
an infinite loop. I doubt this ever happened
in practice, but it's obviously worth fixing.
Now we also have a clear tiebreaker between
any two all/everyone/stream mentions, which
is the idx field.
Finally, this should be a bit more efficient.
This name was misleading, since this code is used
in sort_recipients, which happens when you, for
example, autocomplete persons in the "To:" box
when composing (and has nothing to do with
mentioning).
vaibhavrajsingh2001
Siddharth Varshney
Roland Crosby
jiviteshjain
Pranav
Divyanshu Agrawal
Anders Kaseorg
Abhishek-Balaji
YashRE42
Tim Abbott
Jagan
Hashir Sarwar
Vishnu KS
Kartik Srivastava
Steve Howell
sahil839
Rohitt Vashishtha
Ankita Bansal
Puneeth Chaganti
YashRE42
majordwarf
Wyatt Hoodes
Ryan Rehman
Katerina Perikou
MariaGkoulta
Stefan Weil
Pragati Agrawal
shubhamgupta2956
BIraj
Wbert Adrian Castro Vera
Akash Nimare
vsvipul
Chris Heald
Dinesh
orientor
Jonathan Cobb
Tlazypanda