Automated message from Dropbox CLA bot

@laurenzlong, thanks for the pull request! It looks like you haven't yet signed the Dropbox CLA. Please sign it here and update the thread so we can consider merging your code.

Just signed Dropbox CLA.

I haven't had a chance to review yet, but If it's easy, can you post a screenshot showing this feature in action so others can look at the design sign of it? Thanks!

And this is what it looks like if you don't have any starred streams.

Thanks for writing this @laurenzlong!

I looked at the implementation, and it looks pretty good! I posted a bunch of comments on small improvements to the code, and was inspired to write a lint rule that we should have probably added a while ago.

I haven't had a chance to play with it, but there's two significant pieces that should probably be included:

• The ability to see and toggle this setting on the main "Manage streams" page (/#subscriptions).
• Some frontend tests (frontend_tests/; see docs/testing.rst for some docs) for the feature.

Great, thanks for the improvement suggestions, just added a commit that address those, and will now work on adding a setting to the manage streams page and adding front end tests

The test failures are the following lint issues:

Missing space between ) and { at static/js/stream_list.js line 29
Missing space between ) and { at static/js/stream_list.js line 36
Missing space between ) and { at static/js/stream_list.js line 81
Missing space between ) and { at static/js/stream_list.js line 430
Missing space between ) and { at static/js/subs.js line 396

These are newly flagged by the linter I mentioned adding in my comments.

I had just rebased my branch to get the latest version of the linter, and I'm getting this error message when I run ./tools/lint-all:

Traceback (most recent call last):
File "./tools/lint-all", line 247, in
failed = run_parallel()
File "./tools/lint-all", line 198, in run_parallel
result = func()
File "./tools/lint-all", line 234, in puppet
result = subprocess.call(['puppet', 'parser', 'validate'] + by_lang['.pp'])
File "/usr/lib/python2.7/subprocess.py", line 522, in call
return Popen(_popenargs, *_kwargs).wait()
File "/usr/lib/python2.7/subprocess.py", line 710, in init
errread, errwrite)
File "/usr/lib/python2.7/subprocess.py", line 1327, in _execute_child
raise child_exception
OSError: [Errno 2] No such file or directory
Traceback (most recent call last):
File "./tools/lint-all", line 247, in
failed = run_parallel()
File "./tools/lint-all", line 198, in run_parallel
result = func()
File "./tools/lint-all", line 244, in pyflakes
failed = check_pyflakes()
File "./tools/lint-all", line 152, in check_pyflakes
stderr = subprocess.PIPE)
File "/usr/lib/python2.7/subprocess.py", line 710, in init
errread, errwrite)
File "/usr/lib/python2.7/subprocess.py", line 1327, in _execute_child
raise child_exception
OSError: [Errno 2] No such file or directory
lauren@lauren-UX305FA:~/Documents/code/zulip$ ./tools/lint-all
Traceback (most recent call last):
File "./tools/lint-all", line 247, in
failed = run_parallel()
File "./tools/lint-all", line 198, in run_parallel
result = func()
File "./tools/lint-all", line 234, in puppet
result = subprocess.call(['puppet', 'parser', 'validate'] + by_lang['.pp'])
File "/usr/lib/python2.7/subprocess.py", line 522, in call
return Popen(_popenargs, *_kwargs).wait()
File "/usr/lib/python2.7/subprocess.py", line 710, in init
Traceback (most recent call last):
File "./tools/lint-all", line 247, in
errread, errwrite)
failed = run_parallel()
File "./tools/lint-all", line 198, in run_parallel
File "/usr/lib/python2.7/subprocess.py", line 1327, in _execute_child
result = func()
File "./tools/lint-all", line 244, in pyflakes
failed = check_pyflakes()
File "./tools/lint-all", line 152, in check_pyflakes
stderr = subprocess.PIPE)
File "/usr/lib/python2.7/subprocess.py", line 710, in init
errread, errwrite)
File "/usr/lib/python2.7/subprocess.py", line 1327, in _execute_child
raise child_exception
raise child_exception
OSError: [Errno 2] No such file or directory
OSError: [Errno 2] No such file or directory

Silly me, that was the issue. Thanks!

BTW, from a design perspective, I find the new super-bold STREAMS with the HR underneath it pretty loud, especially when no streams are starred. (USERS seems to have changed in the same way).

What do you think about keeping the same styling as exists today and just having two sections, like this?:

STARRED STREAMS
abc
def
ghi

STREAMS
jkl
mno
pqr

@wdaher I've revised the design, what do you think now? STREAMS is no longer bold, and matches the visual hierarchy of the other titles like "Home" and "Private Messages".

"Streams" looks a little funny and inconsistent with the others — it has an HR underneath it, for one.

How about something like this?

In other words, I don't think the whole section needs to explicitly be labeled "STREAMS" - if you've starred a stream, you get what it's about and we can save space.

The other advantage to this layout is — if you don't star anything at all, the design remains totally unchanged from its current form.

Thoughts?

My attitude is that if you star a stream, you know what it is, and that therefore that word is unnecessary (and less is more).

Keeping it as "starred" woudl also allow us to move PMs or group PMs into that same section someday if we allowed starring of those.

@wdaher Currently, when you hover over "STREAMS" the gear icon is displayed. If we get rid of "STREAMS", and have "STARRED" as the first title, then would it make sense for the gear icon to pop up when user hovers over it? The downside to this design is that the user may think that the gear icon holds settings that only applies to the starred streams, when in fact it holds settings that apply to all the streams.

From my design perspective, this lgtm. @timabbott in case there are any additional comments about the code? If not, Tim, can you please go ahead and merge?

So one design related question we should think about is whether we want to use a differnet name from "starred". One challenge with the name "starred" is that if we wanted to be able to narrow to just these streams, the natural syntax might be something like "is:starred", except that already means something related to the stars on individual messages. Do we want to consider using a name like "pinned" instead? Or renaming the feature for individual messages?

As part of reviewing this, I squashed the commits, rebased it on top of master and factored out one of the CSS changes from the rest of the changeset as a preliminary commit to make it a bit easier to read the logical changes here. I also added a commit on top that we'll probably want to squash into @laurenzlong's main commit to make the STREAMS and STARRED headers still be generated by templates; but that seems to have a bug that causes the STARRED text to not appear at all.

The resulting branch is: https://github.com/timabbott/zulip/commits/starring

When I tested it, I noticed that there are definitely some bugs; for example if you star a random stream, the left sidebar doesn't update properly:

@timabbott I don't have strong feelings, but I don't think there's a real conflict here between starred messages & starred streams— they're both indications that "Hey, this thing is important to me", and I don't mind not having a search operator for "search within starred streams."

(If this ends up being confusing we can always rename it later.)

Yeah, I guess my concern is that (1) database migrations to rename columns are messy to deploy and (2) I think that feature is inevitable, because the "show only messages on starred streams narrow" seems like it'd address the main use case of barnowl-style filters that we don't support.

Which means it'd be best if we got the best column name in the first place.

I agree with the rationale of renaming the column, "pinned" sounds good, "faved" could be another possibility. Tim, the bug where starring a stream doesn't update the left sidebar right away seems to be a regression, since I had tested that. Something may have happened during the refactoring, I'll look into it. I'm currently trying to wrap up a few other projects, so I'll have to put this on hold, and come back to it over the Holidays. But if anyone else would like to inherit this and finish it up, feel free to do so. Thanks for all of your feedback so far.

"pinned" works for me.

And @laurenzlong , from a meta-perspective, sorry it's taken so long for us to get this thing merged, and thanks for helping out :)

Not a problem! Happy to help. :)

@laurenzlong just wanted to check in on whether you've had a chance to take a look at this again -- I'd be excited to get this merged before more changes that it conflicts with happen :)

Hey @timabbott sorry for the uber late reply! I was travelling for the last month in China. I'm now playing catch-up, and hope to get to this soon!

Awesome!

-Tim Abbott (mobile)
On Feb 19, 2016 2:14 PM, "Lauren Long" notifications@github.com wrote:

Hey @timabbott https://github.com/timabbott sorry for the uber late
reply! I was travelling for the last month in China. I'm now playing
catch-up, and hope to get to this soon!

—
Reply to this email directly or view it on GitHub
#285 (comment).

@laurenzlong just wanted to check in on this.

@laurenzlong have you had time to look at this? I'm a Zulip user and if I could star a stream I would probably subscribe to more streams. :) Thanks!

Thanks @brainwane ! I have not forgotten about this, but am really not sure when I can get around to this. Feel free to pick this up if you are passionate about the feature!

I think @krtkmj might end up picking this up as part of his Google Summer of Code project.

That'd be fantastic! @krtkmj let me know if you have any questions or need any help!

@laurenzlong Thanks. I will let you know if I come across any problem.

@laurenzlong I was going through your code. And, I realised Starring some stream(in my case, stream all) doesn't do anything but add a new stream in stream list(Screenshot attached).

Unstaring the same stream, again adds its below stream list. This strange behavior is with some other streams too.

Though reloading the page fixes the problem. But, unstaring the stream again shows incorrect behaviour.

@krtkmj that issue mostly likely means that the backend code to update the database is working correctly, but the process for updating a running browser (which starts with a send_event call in the backend, continues through static/js/server_events.js and ultimately should update the UI) is not -- I'd recommend tracking that code path with print/console.log statements to figure out where the issue is.

Closing this in favor of #819. Thanks @laurenzlong for all your work on this!

Thanks @timabbott , glad to see the feature getting wrapped up!