New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Mute topics #42

Merged
merged 15 commits into from Apr 18, 2016

Conversation

Projects
None yet
4 participants
@kunall17
Collaborator

kunall17 commented Mar 31, 2016

#9,#10

Muting topics in the HomeView removes them from the homeView, and are visible in their respective streams.
Topics can be unmuted from the context menu of their streams.

Screenshots:
Muting Topics by context menu in the Homeview

Unmuting Topics by context menu in the Streams

@smarx

This comment has been minimized.

smarx commented Mar 31, 2016

Automated message from Dropbox CLA bot

@kunall17, it looks like you've already signed the Dropbox CLA. Thanks!

@timabbott

This comment has been minimized.

Member

timabbott commented Mar 31, 2016

Just to clarify, in Zulip we have two concepts, muted streams and muted topics, and the way they currently work is that messages muted either way don't show up on the home view, while messages to muted topics shouldn't show up either in the home view or in the stream view (in either caes, unless the user is mentioned). static/js/muting.js is a helpful reference for topic muting, while grepping the server/web codebase for in_home_view will find the code for stream muting, since that is what it is called in the code.

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Mar 31, 2016

@timabbott but if a user want's to see the muted messages even if they are not tagged then where will they see?
In the web there is listing of the topics in a stream where muted topics are displayed as well and if we unmute them we can view them, But in android we don't have that currently.

Possible solution is we could list the topics in a stream in the sidebar like web and can unmute from their?

@timabbott

This comment has been minimized.

Member

timabbott commented Mar 31, 2016

Yeah, that's a possible solution; if we can't make that work from a UI perspective, then perhaps your approach may be a reasonable hack for now, but I think it's won't solve the full problem, and if we do that, we should make it clear in code comments how the model differs from how muting works on the web.

@timabbott

This comment has been minimized.

Member

timabbott commented Mar 31, 2016

(One of our Android experts should review this branch, I just wanted to mention the details on how muting works on web since it's not well-documented that this is the model)

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Apr 1, 2016

@timabbott so should i start working on this solution?

@niftynei niftynei added the enhancement label Apr 1, 2016

@timabbott

This comment has been minimized.

Member

timabbott commented Apr 1, 2016

Yeah, I think so. My current thinking is the right approach is to mimic the model on web (with muted streams only visible when narrowed to the stream, and muted topics only visible when narrowed to the topic), but adding a way to see the list of topics on a stream.

@niftynei, do you have ideas for how to display the topics within a stream? One idea would be to display it in the left drawer, with the topics for the stream one is currently narrowed to indented a bit.

(As a sidenote, we should probably add some sort of highlighting of the currently narrowed stream in the left sidebar if you're narrowed, since it's pretty lame that we don't do that).

@@ -53,6 +54,9 @@
void openCompose(final MessageType type, String stream, String topic,
String pmRecipients);
void addToList(Message message);
void unMuteTopic(int StreamID,String subject);
void MuteTopic(int StreamID,String subject);

This comment has been minimized.

@niftynei

niftynei Apr 4, 2016

Contributor

please use standard Java conventions for method names here.

Compose method names using mixed case letters, beginning with a lower case letter and starting each subsequent word with an upper case letter.

source: https://www.cwu.edu/~gellenbe/javastyle/method.html

@Override
public void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
if (getArguments() != null) {
filter = getArguments().getParcelable(PARAM_FILTER);
}
muted_messages = new ArrayList<>();

This comment has been minimized.

@niftynei

niftynei Apr 4, 2016

Contributor

Please use conventional Java variable names here. ie mutedMessages instead of muted_messages.

@@ -275,6 +281,13 @@ public void onCreateContextMenu(ContextMenu menu, View v,
if (msg.getType().equals(MessageType.STREAM_MESSAGE)) {
MenuInflater inflater = getActivity().getMenuInflater();
inflater.inflate(R.menu.context_stream, menu);
if ((app.isTopicMute(msg.getStream().getId(), msg.getSubject()))) {
//Muted Topic
menu.findItem(R.id.mute_topic).setTitle("Unmute Topic");

This comment has been minimized.

@niftynei

niftynei Apr 4, 2016

Contributor

User visible strings belong in strings.xml

@@ -4,6 +4,7 @@
<item android:id="@+id/reply_to_sender" android:title="Reply to Sender"></item>
<item android:id="@+id/narrow_to_stream" android:title="Narrow to this stream"></item>
<item android:id="@+id/narrow_to_subject" android:title="Narrow to this topic"></item>
<item android:id="@+id/mute_topic" android:title="Mute Topic"></item>

This comment has been minimized.

@niftynei

niftynei Apr 4, 2016

Contributor

User visible strings should be put in strings.xml.

@@ -261,4 +264,29 @@ public void markMessageAsRead(Message message) {
unreadMessageHandler.sendEmptyMessageDelayed(0, 2000);
}
}
public void MuteTopic(int streamId, String subject) {

This comment has been minimized.

@niftynei

niftynei Apr 4, 2016

Contributor

See above comment about using proper Java naming conventions for method names.

@@ -261,4 +264,29 @@ public void markMessageAsRead(Message message) {
unreadMessageHandler.sendEmptyMessageDelayed(0, 2000);
}
}
public void MuteTopic(int streamId, String subject) {
System.out.println((streamId + "-" + subject));

This comment has been minimized.

@niftynei

niftynei Apr 4, 2016

Contributor

Can you remove this log line so it won't get merged into master?

muted_topics.remove(streamId + "-" + subject);
SharedPreferences.Editor editor = settings.edit();
editor.putStringSet("muted_topics", muted_topics);
editor.commit();

This comment has been minimized.

@niftynei

niftynei Apr 4, 2016

Contributor

commit is fine, but it's generally best practice to use the async command apply. See http://stackoverflow.com/a/5960732

return true;
return false;
}
public boolean isTopicMute(String idAndsubject) {

This comment has been minimized.

@niftynei

niftynei Apr 4, 2016

Contributor

Capitalization on idAndsubject should be idAndSubject.

}
public boolean isTopicMute(String idAndsubject) {
if (muted_topics.contains(idAndsubject))
return true;

This comment has been minimized.

@niftynei

niftynei Apr 4, 2016

Contributor

the if here is superfluous. you can simply return ie:

return muted_topics.contains(idAndSubject)
}
public void UnMuteTopic(int streamId, String subject) {
muted_topics.remove(streamId + "-" + subject);

This comment has been minimized.

@niftynei

niftynei Apr 4, 2016

Contributor

It seems like adding a "-" is a generic operation that you're doing every time you put/fetch a streamID & subject to the Shared Preferences. It's a good idea to break this out into a function and use the function every place that this operation is required.

public void UnMuteTopic(int streamId, String subject) {
muted_topics.remove(streamId + "-" + subject);
SharedPreferences.Editor editor = settings.edit();
editor.putStringSet("muted_topics", muted_topics);

This comment has been minimized.

@niftynei

niftynei Apr 4, 2016

Contributor

putStringSet is buggy when used in this manner. See http://developer.android.com/reference/android/content/SharedPreferences.html#getStringSet%28java.lang.String,%20java.util.Set%3Cjava.lang.String%3E%29. You'll need to either create a new object to pass in to save, or clear out the value before saving.

}
public boolean isTopicMute(int streamId, String subject) {
if (muted_topics.contains(streamId + "-" + subject))

This comment has been minimized.

@niftynei

niftynei Apr 4, 2016

Contributor

the if here is superfluous. you can simply return ie:

return muted_topics.contains(streamId + "-" + subject)
}
@Override
public void MuteTopic(int StreamID, String subject) {

This comment has been minimized.

@niftynei

niftynei Apr 4, 2016

Contributor

Per Java parameter naming conventions, StreamID should be streamId.

@@ -54,6 +57,48 @@
MessageListFragment.Listener {
ZulipApp app;
List<Message> muted_topics;

This comment has been minimized.

@niftynei

niftynei Apr 4, 2016

Contributor

muted_topics should be mMutedTopics or mutedTopics.

}
@Override
public void unMuteTopic(int StreamID, String subject) {

This comment has been minimized.

@niftynei

niftynei Apr 4, 2016

Contributor

Per Java conventions, StreamID should be renamed streamId.

public void unMuteTopic(int StreamID, String subject) {
app.UnMuteTopic(StreamID, subject);
int added = 0;
for (Iterator<Message> messageIterator = muted_topics.iterator(); messageIterator.hasNext(); ) {

This comment has been minimized.

@niftynei

niftynei Apr 4, 2016

Contributor

A cleaner way to do this would be by implementing equals and hashCode on the Message object; this would enable you to be able to call muted_topics.remove(message) or muted_objects.contains(message).

}
}
if (added > 0) {
Collections.sort(homeList.messageList, new Comparator<Message>() {

This comment has been minimized.

@niftynei

niftynei Apr 4, 2016

Contributor

This might be nicer to have Message implement the Comparable interface and move this logic into the Message class.

@@ -315,6 +328,13 @@ public boolean onContextItemSelected(MenuItem item) {
case R.id.copy_message:
copyMessage(message);
return true;
case R.id.mute_topic:
if (app.isTopicMute(message.getStream().getId(), message.getSubject())) { //Unmute topics

This comment has been minimized.

@niftynei

niftynei Apr 4, 2016

Contributor

A cleaner API might be to have isTopicMute accept a Message object.

@@ -181,6 +226,7 @@ protected void onCreate(Bundle savedInstanceState) {
setContentView(R.layout.main);
muted_topics = new ArrayList<>();

This comment has been minimized.

@niftynei

niftynei Apr 4, 2016

Contributor

Spacing here seems off. Can you indent appropriately?

@Override
public void MuteTopic(int StreamID, String subject) {
app.MuteTopic(StreamID, subject);
for (int i = homeList.adapter.getCount() - 1; i >= 0; i--) {

This comment has been minimized.

@niftynei

niftynei Apr 4, 2016

Contributor

depending on how large the homeList, this might be a bit of an expensive operation. it might be worthwhile to add some type of indexed look up for message topics to the homeList adapter. not strictly necessary for this PR tho :)

@niftynei

This comment has been minimized.

Contributor

niftynei commented Apr 4, 2016

Showing the topics in the drawer (same as web) seems like a great way to fix the UI issue, but that's a larger task than this PR. My inclination is that it's ok in the interim to force users to use the web UI to unmute topics, with the idea that mobile apps are more streamlined (so it's ok to see less information in the short-period while we work out the best way to display it).

Alternatively, I'd propose adding a separate activity/screen to view/edit all muted topics & streams. You could add a link to it in the home activity's overflow menu.

@niftynei

This comment has been minimized.

Contributor

niftynei commented Apr 4, 2016

Also, I don't really know much about how muting works on Zulip, but @kunall17 I don't see anywhere that you're pre-populating the list of muted topics/streams. Is this something that you'd need to fetch from the server? As is it seems that your muted topics/streams would only be app-local, which has a lot of potential for confusion for users that use both the Web and app clients.

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Apr 7, 2016

@niftynei
Updated the Code.

And I'm not sure if there is a way to post Mute topic action on the server.

@niftynei

This comment has been minimized.

Contributor

niftynei commented Apr 7, 2016

@kunall17 if that's the case, then we should remove the button allowing the mute/unmute action.

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Apr 7, 2016

@timabbott can you help please?
I want to send a POST request to the server for muting/unmuting a topic, how can i do this?

I tried to find out I got this file which shows mute topics for the webClient

@timabbott

This comment has been minimized.

Member

timabbott commented Apr 7, 2016

There's no current API route available for changing topic muting; see
zulip/zulip#611 for what needs to be done to change that.

The mechanism for stream muting involves setting the in_home_view flag on a subscription object, so that should be possible with the current API via PATCH users/me/subscriptions

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Apr 8, 2016

@timabbott thankyou!
So should we have non-editable muting topics, streams for the time being??
Or only allow them to mute/unmute stream and mute all the topics received from the server?

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Apr 11, 2016

@timabbott bump, please..

@timabbott

This comment has been minimized.

Member

timabbott commented Apr 11, 2016

Ahh OK @kunall17, I hadn't noticed you'd edited your previous message.

I defer to @niftynei on how to manage this PR. I think ultimately we probably want to both display streams/topics correctly on Android as well as control the muting status on Android, but often it can be a good idea to try to finish the first part really well (and get it merged) as a separate project from the second part.

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Apr 12, 2016

Now I have,

  • Removed muting/unmuting topics from the context menu and muted topics are hidden from homeView.
  • Change TextColor of the muted streams in the Stream Drawer.
  • Rebased this branch to master.

I am still using the SharedPreferences for saving the muted topics, as in when API routes are developed, I will add unmute/mute topics and streams so there it will be useful.

@niftynei

This comment has been minimized.

Contributor

niftynei commented Apr 12, 2016

@kunall17 {deleted}. this looks pretty good! my last nit-pick would be that it's impossible to tell what topics you've muted from within a stream. Is there some way we can grey these out or mark them as "muted". Maybe with a tag in the header to the far left? I realize this is different from what's on the web but it'd help prevent confusion.

I'll leave a comment in line as well, but also worth noting that the text-color changing you've implemented for streams in the drawer is buggy -- it's greying out streams other than the ones I've muted.

Great work on this. Let's get the bug fixed up and add a label to muted topic headers and we'll get this merged in. 👍

@@ -88,6 +112,9 @@ public boolean setViewValue(View arg0, Cursor arg1, int arg2) {
case R.id.name:
TextView name = (TextView) arg0;
name.setText(arg1.getString(arg2));
//Change color in the drawer if this stream is inHomeView only.
if (!Stream.getByName(app, arg1.getString(arg2)).getInHomeView())
name.setTextColor(ContextCompat.getColor(ZulipActivity.this, android.R.color.tertiary_text_light));

This comment has been minimized.

@niftynei

niftynei Apr 12, 2016

Contributor

When these views get recycled, the text stays grey (so streams that aren't mute have grey labels 😞 ). One way to fix this is to reset the text back to the default color if the Stream isInHomeView().

This comment has been minimized.

@kunall17

kunall17 Apr 13, 2016

Collaborator

Ohh yeah sorry, didn't notice this one as I muted all the streams in my server.!

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Apr 13, 2016

@niftynei in these new commits I have added a icon of muted topics to the header of the message tile.

@niftynei niftynei merged commit edeb73a into zulip:master Apr 18, 2016

@niftynei

This comment has been minimized.

Contributor

niftynei commented Apr 18, 2016

looks good to me. thanks for all the work on this @kunall17! 👍

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Apr 19, 2016

@niftynei thankyou for merging this one. :)

@niftynei niftynei referenced this pull request Apr 24, 2016

Closed

Muted topics are not hidden #9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment