filter: Implement has: filters on client side. #15492

thedeveloperr · 2020-06-20T22:11:27Z

Prior to this commit has:link, has:attachment, has:image
filter couldn't be applied locally and deferred filtering to
web server. This commits make sure client filters all messages
it can instead of completely deferring to the server and hence
improve speed.

A tradeoff is also made to turn off local echo for has: narrows
as messages with link sent to has:link narrow were locally echoing
to another narrow and not appearing in the active has:link narrow.

Fixes: #6186.

Testing Plan:

GIFs or Screenshots:

thedeveloperr · 2020-06-20T22:13:47Z

@timabbott created a separate PR for implementing the way you suggested in #15363 (comment)

I want to ask one thing, emoji like :zulip: are also images should the filter include them as well in has:image ? Zulip chat discussion for same: https://chat.zulip.org/#narrow/stream/6-frontend/topic/client.20side.20has.3A.20filter/near/910461

aero31aero · 2020-06-20T22:35:36Z

static/js/echo.js

+    // We dont want to locally echo those has: messages
+    // because:
+    //  1. has:image, has:attachment are backend only syntax.
+    //  2. has:link do not get displayed in current narrow


has: link might not get displayed is the case, I think.

I am able to send messages with link when has:link narrow is active.

aero31aero · 2020-06-20T22:38:00Z

static/js/message_util.js

+    return $('<div>').append(message.content).find(`${tagName}:first`).length > 0;
+}
+
+exports.does_message_have_link = function (message) {


These functions can be better named as exports.message_has_link, etc.

aero31aero · 2020-06-20T22:43:02Z

static/js/message_util.js

@@ -16,6 +16,23 @@ function add_messages(messages, msg_list, opts) {
    return render_info;
 }

+function does_message_content_have_html_tag(message, tagName) {
+    // creating a virtual dom to check for html tag in message.content.
+    return $('<div>').append(message.content).find(`${tagName}:first`).length > 0;


I'm curious about GC related issues with this. I added a similar check in one of my PRs but I've a feeling JQuery might keep a reference to the created object internally and as a result we could end up with a lot of Query objects each time this function is called.

Do you know if that isn't the case here? I couldn't find anything relevant online so far.

I will write a quick jsfiddle and monitor it's memory usage to see if it's the case.

https://github.com/jquery/jquery/blob/master/src/core/init.js
https://github.com/jquery/jquery/blob/master/src/core.js

I think it just return object and it should be garbage collected.

aero31aero

This looks nice! The changes make sense, but I'm confused about the tests as they exist now. Maybe they could use some comments to explain what's the intention?

aero31aero · 2020-06-20T22:43:54Z

frontend_tests/node_tests/filter.js

 zrequire('Filter', 'js/filter');
-


Accidentally removed this line?

aero31aero · 2020-06-20T22:45:21Z

frontend_tests/node_tests/filter.js

@@ -715,6 +717,50 @@ run_test('predicate_basics', () => {
        display_recipient: [{id: steve.user_id}, {id: me.user_id}],
    }));
    assert(!predicate({type: 'stream'}));
+
+    const img_msg = {
+        content: "<p><a href=\"\/user_uploads\/randompath\/test.jpeg\">test.jpeg<\/a><\/p>\n<div class=\"message_inline_image\"><a href=\"\/user_uploads\/randompath\/test.jpeg\" title=\"test.jpeg\"><img src=\"\/user_uploads\/randompath\/test.jpeg\"><\/a><\/div>",


We could use strings contained within single quotes or backticks to avoid the escaping here... unless it was actually simpler to use these from our markdown_test_cases.json directly.

I think back-tick approach is better.

aero31aero · 2020-06-20T22:58:01Z

frontend_tests/node_tests/filter.js

+
+    predicate = get_predicate([['has', 'link']]);
+    let mockElem = $("<div>").append(link_msg.content);
+    mockElem.set_find_results("a:first", [$("<a>")]);


I'm confused; how are these tests working? What is calling find on a div in the DOM to which you're appending these?

Umm wait I will clean it up. I think I mixed up.

aero31aero · 2020-06-20T22:58:47Z

frontend_tests/node_tests/filter.js

+    assert(!predicate(no_has_filter_matching_msg));
+
+    predicate = get_predicate([['has', 'attachment']]);
+    mockElem = $("<div>").append(non_img_attachment_msg.content);


Is it intentional that you want to append to the existing JQuery object?

thedeveloperr · 2020-06-21T00:06:05Z

@aero31aero I have pushed new changes.

timabbott · 2020-06-21T06:07:57Z

static/js/echo.js

+        narrow_state.filter().has_operator('has')
+    ) {
+        return;
+    }


I'd write the if statement with fewer lines

timabbott · 2020-06-21T06:09:08Z

frontend_tests/node_tests/filter.js

+    };
+
+    const non_img_attachment_msg = {
+        content: `<p><a href="/user_uploads/randompath/attachemnt.ext">attachment.ext</a></p>`,


attachment is mispelled

timabbott · 2020-06-21T06:12:36Z

frontend_tests/node_tests/filter.js

+    $(link_msg.content).set_find_results(".message_inline_image", false);
+    assert(!predicate(link_msg));
+    $(no_has_filter_matching_msg.content).set_find_results(".message_inline_image", false);
+    assert(!predicate(no_has_filter_matching_msg));


I think this test might be more readable if saved the predicates like this:

check_has_link = get_predicate([['has', 'link']]); ...

And then setup 6 or so messages and just assertions like this:

assert(has_link(img_msg)) assert(has_attachment(img_msg)) assert(has_image(img_msg))

timabbott · 2020-06-21T06:13:02Z

static/js/filter.js

+        } else if (operand === 'attachment') {
+            return message_util.message_has_attachment(message);
+        }
+        return false; // is:whatever returns true


Comment should use has.

and also say false, since that's what we're doing here :)

timabbott · 2020-06-21T06:13:59Z

Posted a batch of comments; but this looks great otherwise!

thedeveloperr · 2020-06-21T13:27:45Z

@timabbott not sure why this test failed https://app.circleci.com/pipelines/github/zulip/zulip/8798/workflows/c93e31cd-380c-4b27-b552-cf5c0c385bcf/jobs/50312/steps

Prior to this commit has:link, has:attachment, has:image filter couldn't be applied locally and deferred filtering to web server. This commits make sure client filters all messages it can instead of completely deferring to the server and hence improve speed. A tradeoff is also made to turn off local echo for has: narrows as messages with link sent to has:link narrow were locally echoing to another narrow and not appearing in the active has:link narrow. Fixes: zulip#6186.

thedeveloperr · 2020-06-21T14:11:35Z

@timabbott not sure why this test failed https://app.circleci.com/pipelines/github/zulip/zulip/8798/workflows/c93e31cd-380c-4b27-b552-cf5c0c385bcf/jobs/50312/steps

Re-run test by force pushing.

timabbott · 2020-06-21T17:27:26Z

I merged as 02ea52f, after reworking the local echo logic to feel more intuitive; take a look and let me know what you think.

Thanks @thedeveloperr!

thedeveloperr · 2020-06-21T17:34:15Z

@timabbott I want to understand more about the reason of local echo change. I mean it feels counter intuitive and logic more hidden.

timabbott · 2020-06-22T19:29:42Z

Oh, that was because filter.can_apply_locally is the authoritative function for deciding whether the client supports correctly appending a message to a narrow, and it's a better abstraction boundary to have that function handle all questions around that, rather than having another function handle this corner case around has:. Which is somewhat visible with how complex the if statement you needed to add to do it in try_deliver_locally, duplicating some logic around can_apply_locally.

zulipbot added the size: L label Jun 20, 2020

aero31aero reviewed Jun 20, 2020

View reviewed changes

thedeveloperr force-pushed the client_has_filter branch from 1cba481 to 3326c0f Compare June 21, 2020 00:02

zulipbot added size: XL and removed size: L labels Jun 21, 2020

thedeveloperr force-pushed the client_has_filter branch from 3326c0f to a922885 Compare June 21, 2020 00:05

timabbott reviewed Jun 21, 2020

View reviewed changes

static/js/echo.js

narrow_state.filter().has_operator('has')

) {

return;

}

Copy link

Member

timabbott Jun 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd write the if statement with fewer lines

timabbott reviewed Jun 21, 2020

View reviewed changes

thedeveloperr force-pushed the client_has_filter branch from a922885 to 86a60ed Compare June 21, 2020 13:00

zulipbot added size: L and removed size: XL labels Jun 21, 2020

thedeveloperr changed the title ~~[WIP] filter: Implement has: filters on client side.~~ filter: Implement has: filters on client side. Jun 21, 2020

thedeveloperr requested a review from timabbott June 21, 2020 13:01

thedeveloperr force-pushed the client_has_filter branch from 86a60ed to 030c1a6 Compare June 21, 2020 14:05

timabbott closed this Jun 21, 2020

thedeveloperr mentioned this pull request Jun 22, 2020

[WIP] Implement has: client side filter #15363

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

filter: Implement has: filters on client side. #15492

filter: Implement has: filters on client side. #15492

thedeveloperr commented Jun 20, 2020

thedeveloperr commented Jun 20, 2020 •

edited

Loading

aero31aero Jun 20, 2020

thedeveloperr Jun 20, 2020

aero31aero Jun 20, 2020

aero31aero Jun 20, 2020

thedeveloperr Jun 20, 2020

thedeveloperr Jun 20, 2020

aero31aero left a comment

aero31aero Jun 20, 2020

thedeveloperr Jun 20, 2020

aero31aero Jun 20, 2020

thedeveloperr Jun 20, 2020

aero31aero Jun 20, 2020

thedeveloperr Jun 20, 2020

aero31aero Jun 20, 2020

thedeveloperr commented Jun 21, 2020

timabbott Jun 21, 2020

timabbott Jun 21, 2020

timabbott Jun 21, 2020

timabbott Jun 21, 2020

timabbott Jun 21, 2020

timabbott commented Jun 21, 2020

thedeveloperr commented Jun 21, 2020

thedeveloperr commented Jun 21, 2020

timabbott commented Jun 21, 2020

thedeveloperr commented Jun 21, 2020

timabbott commented Jun 22, 2020

filter: Implement has: filters on client side. #15492

filter: Implement has: filters on client side. #15492

Conversation

thedeveloperr commented Jun 20, 2020

thedeveloperr commented Jun 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aero31aero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thedeveloperr commented Jun 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabbott commented Jun 21, 2020

thedeveloperr commented Jun 21, 2020

thedeveloperr commented Jun 21, 2020

timabbott commented Jun 21, 2020

thedeveloperr commented Jun 21, 2020

timabbott commented Jun 22, 2020

thedeveloperr commented Jun 20, 2020 •

edited

Loading