Mute topics with the M key / unmute with U #997

antifuchs · 2016-06-10T15:57:12Z

My (and I believe many other people's) Zulip workflow involves muting a bunch of Topics that they're not interested in. Since the quickest way to do it currently is context-dependent and takes a bunch of key presses (i, <down> <down> [<down>] enter), I decided to add keyboard shortcuts for muting/unmuting.

The mute action will show an alert about the muted topic that fades out over 4 seconds, and allows un-muting from the alert itself.

I hope you'll consider this for inclusion; I still need to sign the Dropbox CLA.

antifuchs · 2016-06-10T16:28:47Z

(Test failure is due to lack of test coverage on the handlebars file; I'll try to make up a test for it)

timabbott · 2016-06-10T17:19:21Z

Looks like you're getting a lot of lint failures from adding new global variables; the new functions should all move into the muting.js module. Also maybe there's a few from not declaring variables. You can run tools/lint-all to see the output.

And the "frontend" test failure is because you have a new template that doesn't have tests.

antifuchs · 2016-06-12T05:04:17Z

I managed to bring the tests back in line & add the frontend test, phew!

timabbott · 2016-06-13T01:44:16Z

Cool, I reviewed this and the code looks pretty clean; here are a few comments:

If you mute and then unmute a topic via the keyboard, the warning with the undo button should disappear. * If you try to mute something that's already muted, it should probably not just pop a second copy of the undo button warning. Probably best would be to just pop a variant warning that says "it's already muted".
I think the undo button disappears too quickly for anyone to actually hit it; maybe give it more like 5-10s than 3s?
It's a bit weird that the "undo" feature appears attached to the compose box, since this doesn't really have anything to do with composing messages. Perhaps we should instead use a new error area that drops down from the top of the screen? We'd likely want to reuse such an area for a good solution to The tutorial causes multiple notification permission panels in Safari. #883 as well.
You should rebase the last two commits into the commits that introduced the issues they resolve, so the tests and linter pass on each commit in this branch.
It'd be great to have some frontend tests for this feature; maybe add a new suite in the Casper test suite for it? Probably not a blocker for merging this, since we don't have existing frontend tests for muting (see http://zulip.readthedocs.io/en/latest/testing.html#writing-tests for docs on writing these), but I'd very much appreciate them since they dramatically decrease risk of regressions.

Otherwise, this looks great!

As a sidenote, I don't think fixing this should be part of this PR, but the backend implementation for muting isn't designed to handle a user having muted hundreds of topics. I opened #1019 for this issue.

timabbott · 2016-06-30T23:17:20Z

Hey @antifuchs, I just wanted to check on whether you've had a chance to work on finishing this. It'd be great to get it merged before a lot of merge conflicts develop :)

kartikmaji · 2016-07-02T07:27:16Z

I would suggest why not just user 'M' key for both muting and unmuting the topics. Just like streaming any video on youtube pressing 'space' button both pauses and resumes video.
@timabbott: Any thoughts?

antifuchs · 2016-07-03T17:01:33Z

Ahhh, sorry for the radio silence! I'll try and port it forward to latest master and address your comments this weekend.

@krtkmj - I feel like muting/unmuting a topic with only one key while showing a stream might be confusing. It's not extremely clear which way is active, so I think having one button per action would make it easier to predict what will happen.

kartikmaji · 2016-07-03T17:54:55Z

I was just checking this functionality and here are a few points.

Try narrowing to a stream with multiple topics. Now, using M key mutes any random topic which is pretty strange whereas using U key doesn't have any effect. In this case, more intuitive way will be to mute the stream.
It would be great to have success message for unmuting a topic.

EDIT: When a topic/stream is muted, it is also faded in left sidebar stream list. So, I don't having single button for both operation should be a problem. @neerajwahi what do you think?

antifuchs · 2016-07-05T00:35:13Z

I've rebased & added a Casper test, but sadly it doesn't pass on travis (but the whole casper test suite passes locally, ugh!)

Additionally, I've adjusted the timeout to 5s and made the "muted topic X in stream Y" message only appear once & get dismissed if you unmute while it's visible.

I took no action on the other points you two raised (especially the note about putting the message into the compose box, which I agree with but feel unequipped to handle the design implications of). I am happy to give you all push permissions on my branch so we can get somewhere!

timabbott · 2016-07-06T21:56:34Z

Great, thanks for working on this again @antifuchs!

Usually cases where a Casper test passes locally but not on Travis is because Travis' machines are slower than yours. Sometimes you can have good luck reproducing these by getting a super-cheap VM from e.g. Scaleway and running the tests there. But regardless the problem is almost certainly that you are missing a wait (or are waiting on a thing that loads before the element you're about to use) in a test, and thus clicking on something before its ready. I'll take a quick look at the code to see if I can spot something suspiciuos.

timabbott · 2016-07-06T21:57:23Z

@antifuchs I actually think we should stick with M/U; I think the expectations are a bit clearer that way.

timabbott · 2016-07-06T21:58:34Z

frontend_tests/casper_tests/12-muting.js

+        common.keypress(27); // Escape to deactivate the message edit box
+        casper.page.sendEvent('keypress', 'U');
+        common.un_narrow();
+        common.un_narrow();


why does this unnarrow twice?

Oh, I bed that's there because you were getting failures with the unnarrow hadn't been completed yet; I think my proposed fix to the wait line below is the right way to address this (and you can delete the extra un_narrow)

Being in streams that generate a lot of topics that have a medium chance of being uninteresting can make it hard to keep up - muting a topic can help, but without a key binding, it's pretty tedious. This change adds the "M" key binding that allows muting a topic very quickly, and an "U" key binding that allows unmuting it from the topic's stream.

This tests that muting via the M key and un-muting via the button, as well as un-muting via the U key in a narrowed (muted) topic work.

No longer show more than one "topic X in stream Y" is muted message anymore. Also, dismiss the message if the topic gets unmuted while the message is showing (I don't expect this to be very common, but it's definitely possible to do).

antifuchs · 2016-07-10T00:07:37Z

Yay! Your hint about the #zhome selector was entirely right - I've removed the second un_narrow call, and rebased to latest master once more, to make the backend suite succeed. We've tests & we're green now (:

timabbott · 2016-07-11T00:40:54Z

Great! Just to make sure I've kept track of the discussion correctly, are these the outstanding issues from above that you haven't resolved @antifuchs?

It's a bit weird that the "undo" feature appears attached to the compose box, since this doesn't really have anything to do with composing messages. Perhaps we should instead use a new error area that drops down from the top of the screen? We'd likely want to reuse such an area for a good solution to The tutorial causes multiple notification permission panels in Safari. #883 as well. (Not resolved yet since we don't have a convenient way to create such alerts)
Try narrowing to a stream with multiple topics. Now, using M key mutes any random topic which is pretty strange whereas using U key doesn't have any effect. In this case, more intuitive way will be to mute the stream.
It would be great to have success message for unmuting a topic.

@antifuchs what are your thoughts on the last two?

timabbott · 2016-07-22T21:16:53Z

@antifuchs just wanted to check in -- it'd be great to get these last bits fixed up and merged before merge conflicts develop :)

brainwane · 2016-08-12T21:24:37Z

@antifuchs: @aakash-cr7 is interested in collaborating with you on this and helping finish it!

aakashtyg · 2016-08-18T19:39:47Z

@brainwane @timabbott I have been working on this issue. I want to show my latest work. What should I do, should I create a new PR?

timabbott · 2016-08-18T19:43:25Z

That's reasonable; the other approach is you can push your branch to your GitHub fork and post a link to it.

aakashtyg · 2016-08-24T05:05:21Z

@timabbott I was wondering about the success messages for muting/unmuting topics. Currently it shows up in the compose box, so how should I handle this for now? As you suggested should I add a new error area that drops down from the top of the screen?

timabbott · 2016-08-24T05:26:01Z

We now have a top-of-screen error area being worked on in #1692; we might be able to use that for this. I'm not entirely sure where the best place to put the notifications for this feature are; perhaps that's a good discussion to bring up at this Thursday's group hangout? Seems worth getting a lot of people's ideas on it...

aakashtyg · 2016-08-24T08:12:24Z

I was thinking of showing the success messages on the bottom left side of the screen (just like the new stream message notification appears on the right part of the screen). But yes it's wise to discuss the issue first in Thursday's group hangout.

aakashtyg · 2016-08-29T05:53:09Z

@showell as @timabbott suggested that this PR should contain only the muting/unmuting functionality. That functionality is done but the work on notification is already committed so what should I do? Should I rebase to remove those changes or commit again removing those changes?

showell · 2016-08-29T20:22:54Z

I don't think the notifications stuff has actually hit master yet. Unless I'm mistaken, we still have the option to get yourself stuff to master first, and then Brock can rework his piece as needed.

kartikmaji · 2016-08-30T02:03:42Z

@aakash-cr7 This branch has some merge conflicts. Can you please rebase this branch with master?

aakashtyg · 2016-09-07T18:14:59Z

@timabbott in my latest commit I have made the topic to mute and unmute on pressing M and U respectively. I am having some issues while rebasing, though.

timabbott · 2016-09-08T23:41:57Z

@aakash-cr7 I rebased for you and posted the resulting branch at: https://github.com/timabbott/zulip/tree/muting-hotkeys

aakashtyg · 2016-09-15T12:06:19Z

@timabbott currently the code works for just muting and unmuting the topics, what's more to be done?

timabbott · 2016-09-15T17:40:47Z

@aakash-cr7 can you test out #1711 (both along and combined with this branch) to make sure things work properly?

aakashtyg · 2016-09-17T06:03:33Z

Sure @timabbott . I will do that and ping you once done.

brainwane · 2016-09-26T18:33:37Z

@aakash-cr7 how's this going?

timabbott · 2016-09-26T19:32:26Z

I think #1711 is looking really good, solves the notifications piece of the issue, and will merge in the next day. So, you should be able to start working on rebasing these changes on top of that.

timabbott · 2016-09-27T03:59:22Z

We just merged #1711, so now it should be possible to merge the new hotkeys, with only limited integration with the muting notifications code (we probably still want to dismiss the notification in the event that the user unmutes the stream while it is visible).

timabbott · 2016-10-05T01:18:50Z

@aakash-cr7 and @antifuchs do either of you have time to finish this up now that the notifications issue is resolved? I'd love to get this feature in.

aakashtyg · 2016-10-07T10:47:55Z

@timabbott sorry for the slow response. I had been busy lately so could not finish up the work. Though now I am mostly free and will be able to commit time to this. I will finish it up and get back to you ASAP.

timabbott · 2016-10-11T23:26:49Z

Cool, let us know if you need any help @aakash-cr7!

timabbott · 2016-10-18T01:27:33Z

@aakash-cr7 @antifuchs should we be looking for someone else to pick up this project?

timabbott · 2016-11-02T06:46:26Z

I guess someone new should pick this up. https://github.com/timabbott/zulip/tree/muting-hotkeys is the latest state; I believe the remaining work is basically to clean up the notifications code from this PR in some reasonable way, since that was completed separately in #1711.

aakashtyg · 2016-11-04T19:27:47Z

@timabbott in this PR the muting and unmuting of topics is working. There is no notification for muting/unmuting. The notification part is done in #1711. Shouldn't the two PR's be merged? Or what should be done?

timabbott · 2016-11-04T20:41:34Z

Right, #1711 was merged into master a while ago; what remains is that this PR needs to be rebased and then tested to make sure they work well together.

aakashtyg · 2016-11-06T06:49:48Z

@timabbott great! I have rebased the PR and everything seems to be working fine. One question though. What should be the functionality, if the topic is already muted and user presses 'M' again. I have thought about the following 2 scenarios:

Do nothing
Show in the popover notification that the topic is already muted

What do you think?

timabbott · 2016-11-07T05:23:04Z

I think probably showing the popover with a faster disappear time (like 1s, not 4s) and a notice that the topic is already muted is probably best?

(Can you push to this branch to update the PR?)

aakashtyg · 2016-11-07T09:21:20Z

Cool, showing the notice that the topic is already muted sounds good. One more thing, the latest code for this PR is in https://github.com/timabbott/zulip/tree/muting-hotkeys. And I rebased that branch after pulling from it. So @timabbott should I push to that branch itself?

timabbott · 2016-11-10T16:21:31Z

You don't have permission to push to that branch without my doing some setup, and there's also no real value to seeing so; just push to a branch either in your own repo or on the one this PR is based from (labeled at the top of this page, but I think @antifuchs gave you access to push to it?). It wouldn't be crazy to create a fresh PR for your new version of this.

aakashtyg · 2016-11-11T19:07:39Z

Updated the latest code in PR #2292.

timabbott · 2016-11-11T19:08:30Z

OK cool, we can close this one in favor of #2292 then. Thanks @aakash-cr7!

timabbott mentioned this pull request Jun 13, 2016

Redesign implementation of topic muting to scale well #1019

Closed

antifuchs force-pushed the mute-with-M-key branch 3 times, most recently from a97b50d to 44b4d26 Compare July 5, 2016 00:17

timabbott reviewed Jul 6, 2016
View reviewed changes

antifuchs force-pushed the mute-with-M-key branch 3 times, most recently from 18a31d3 to 116fe0f Compare July 9, 2016 23:55

antifuchs added 4 commits July 9, 2016 16:56

Add a casper test for muting / unmuting

f3a93dc

This tests that muting via the M key and un-muting via the button, as well as un-muting via the U key in a narrowed (muted) topic work.

Extend fadeout time for the unmute button to 5s

93ed0e6

Adjust mute message behavior

116fe0f

No longer show more than one "topic X in stream Y" is muted message anymore. Also, dismiss the message if the topic gets unmuted while the message is showing (I don't expect this to be very common, but it's definitely possible to do).

Make M/U, mute/unmute topics.

5b377b6

aakashtyg mentioned this pull request Nov 11, 2016

Muting/Unmuting topics using M/U #2292

Closed

timabbott closed this Nov 11, 2016

Mute topics with the M key / unmute with U #997

Mute topics with the M key / unmute with U #997

Conversation

antifuchs commented Jun 10, 2016

antifuchs commented Jun 10, 2016

timabbott commented Jun 10, 2016 • edited

antifuchs commented Jun 12, 2016

timabbott commented Jun 13, 2016

timabbott commented Jun 30, 2016

kartikmaji commented Jul 2, 2016 • edited

antifuchs commented Jul 3, 2016

kartikmaji commented Jul 3, 2016 • edited

antifuchs commented Jul 5, 2016

timabbott commented Jul 6, 2016

timabbott commented Jul 6, 2016

timabbott Jul 6, 2016

Choose a reason for hiding this comment

timabbott Jul 6, 2016

Choose a reason for hiding this comment

antifuchs commented Jul 10, 2016

timabbott commented Jul 11, 2016

timabbott commented Jul 22, 2016

brainwane commented Aug 12, 2016

aakashtyg commented Aug 18, 2016

timabbott commented Aug 18, 2016

aakashtyg commented Aug 24, 2016

timabbott commented Aug 24, 2016

aakashtyg commented Aug 24, 2016

aakashtyg commented Aug 29, 2016 • edited

showell commented Aug 29, 2016

kartikmaji commented Aug 30, 2016

aakashtyg commented Sep 7, 2016

timabbott commented Sep 8, 2016

aakashtyg commented Sep 15, 2016

timabbott commented Sep 15, 2016

aakashtyg commented Sep 17, 2016

brainwane commented Sep 26, 2016

timabbott commented Sep 26, 2016

timabbott commented Sep 27, 2016

timabbott commented Oct 5, 2016

aakashtyg commented Oct 7, 2016

timabbott commented Oct 11, 2016

timabbott commented Oct 18, 2016

timabbott commented Nov 2, 2016

aakashtyg commented Nov 4, 2016 • edited

timabbott commented Nov 4, 2016

aakashtyg commented Nov 6, 2016 • edited

timabbott commented Nov 7, 2016

aakashtyg commented Nov 7, 2016 • edited

timabbott commented Nov 10, 2016

aakashtyg commented Nov 11, 2016

timabbott commented Nov 11, 2016

timabbott commented Jun 10, 2016 •

edited

kartikmaji commented Jul 2, 2016 •

edited

kartikmaji commented Jul 3, 2016 •

edited

aakashtyg commented Aug 29, 2016 •

edited

aakashtyg commented Nov 4, 2016 •

edited

aakashtyg commented Nov 6, 2016 •

edited

aakashtyg commented Nov 7, 2016 •

edited