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

Implemented Expandable ListView in the streamsDrawer #85

Merged
merged 6 commits into from Jul 19, 2016

Conversation

Projects
None yet
4 participants
@kunall17
Collaborator

kunall17 commented Jul 12, 2016

This PR changes the current listView used in the streamsDrawer and insert child View's to display the corresponding Subjects/Topics to the related streams

Preview:
expandlistview

@smarx

This comment has been minimized.

smarx commented Jul 12, 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 Jul 12, 2016

Just wanted to say this looks awesome!

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Jul 12, 2016

@timabbott This does have a problem, currently I'm extracting the subjects/topics from the received messages but at the initial stage not many messages are received hence, there are less number of topics in the database than the server.

Is there an API to fetch the topics separately or any other possible way to solve this problem?

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Jul 12, 2016

@timabbott thanks! :)
And this is the initial implementation to display the unread counts, in a further updates I will update them to display the unread counts in the right space left in the listView.
So how to determine the unread counts of messages, any API routes?

@timabbott

This comment has been minimized.

Member

timabbott commented Jul 12, 2016

There isn't an API for that, but we could consider adding one. The other thing we could do is consider fetching more messages in the initial batch when the client starts up. How many messages does the app currently fetch? For the webapp, the (compressed) fetch is about 167KB for 1000 messages (which is plenty of history); I wonder if that's small enough that it could be the simplest solution...

@timabbott

This comment has been minimized.

Member

timabbott commented Jul 12, 2016

For unread counts, there's no convenient API; the current mechanism on web is for the frontend to just compute that data after fetching the message history. I guess that would be another argument in favor of just trying to fetch data how the website does (if it's fast enough, which is a real concern)

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Jul 13, 2016

@timabbott Currently the AsyncGetOldMessages recieves 101 messages which was of 56.75 KB and mostly the messages contained random one line chat messages, the size is very big than the JSON received in the web UI, api route used is "v1/messages" to fetch the old messages.

I have uploaded the JSON I receive.
messages.txt

But fetching 1000 messages can be inaccurate as well because there can be more subjects in the remaining messages!

@timabbott

This comment has been minimized.

Member

timabbott commented Jul 13, 2016

The sizes I was giving are gzip-compressed sizes, not the uncompressed sizes.

It's possible you're doing this already, but if not, you should make sure your API requests are sending the appropriate headers to indicate that you can accept gzip-compressed content (on web the server and client do the compression transparently...).

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Jul 13, 2016

Well I'm not thorough with the compression of okHttp Network library but it supports "Transparent GZIP shrinks download sizes." as stated in their website

I checked the response headers of the POST request of the "v1/messages" and I see a header Transfer-Encoding:chunked,

Then I added a header "Accept-Encoding: gzip" in the POST request no new headers were there in the response headers!

@niftynei can you help in this thing, please?

@niftynei

This comment has been minimized.

Contributor

niftynei commented Jul 13, 2016

@kunall17

it looks like OkHTTP does Gzip without any prompting: https://github.com/square/okhttp/blob/988142cfe555547bc1df4370babe12c6be86cf3e/okhttp/src/main/java/okhttp3/internal/http/BridgeInterceptor.java#L78-L82

This thread might have some information that's useful: square/okhttp#2132

As a last resort, there's always searching thru the OkHttp source: https://github.com/square/okhttp/search?utf8=%E2%9C%93&q=gzip

Let me know if that doesn't help out.

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Jul 13, 2016

Yeah I had read this thread, anyways I think to test whether compression is working or not, I should test the size of JSON recieved or the total network usage after disabling the gzip here https://github.com/zulip/zulip/blob/master/puppet/zulip/files/nginx/nginx.conf#L31

Will do this test tomorrow!

for (String[] result : results) {
try {
matrixCursor.addRow(new String[]{result[0], groupCursor.getInt(0) + ""});
} catch (Exception e) {

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

it's a good idea to be explicit about what types of exceptions you're catching.

}
MatrixCursor matrixCursor = new MatrixCursor(new String[]{"subject", "_id"});
for (String[] result : results) {

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

If results is null, this will throw an exception. The best thing to do would be to initialize the results to an empty List.

results = ZulipApp.get().getDao(Message.class).queryRaw("SELECT DISTINCT subject FROM messages " +
"JOIN streams ON streams.id=messages.stream " +
"WHERE streams.id=" + groupCursor.getInt(0)).getResults();
} catch (Exception e) {

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

it's a good idea to be explicit about what types of exceptions you're catching.

"JOIN streams ON streams.id=messages.stream " +
"WHERE streams.id=" + groupCursor.getInt(0)).getResults();
} catch (Exception e) {
e.printStackTrace();

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

We should use ZLog or Log

try {
matrixCursor.addRow(new String[]{result[0], groupCursor.getInt(0) + ""});
} catch (Exception e) {
e.printStackTrace();

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

See note above about printing things.

R.layout.stream_tile_new, groupFrom,
groupTo, R.layout.stream_tile_child, childFrom,
childTo);
} catch (Exception e) {

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

See above comments about catching exceptions & printing to the logs.

String subjectName = ((TextView) v).getText().toString();
onNarrow(new NarrowFilterStream(streamName, subjectName));
onNarrowFillSendBoxStream(streamName, subjectName);
break;

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

including a default case is best practice, even if it's empty.

final String streamName = cursor.getString(columnIndex);
name.setText(streamName);
//Change color in the drawer if this stream is inHomeView only.
if (!Stream.getByName(app, streamName).getInHomeView())

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

if / else statements should always have brackets {}

public void checkAndSetupStreamsDrawer() {
try {
if (streamsDrawer.getAdapter().getCount() != 0) return;

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

this is potentially confusing, please add brakcet to the if statement.

topicActv.setText(subject);
if ("".equals(subject)) {
topicActv.requestFocus();
} else messageEt.requestFocus();

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

please use {} brackets on if - else statements.

for (String[] result : results) {
try {
matrixCursor.addRow(new String[]{result[0], groupCursor.getInt(0) + ""});

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

String.valueOf(groupCursor.getInt(0)) is better than using + "".

@niftynei

This comment has been minimized.

Contributor

niftynei commented Jul 18, 2016

hey @kunall17 this looks really good!

There's 1 bug that's a blocker for merge: clicking on a stream header no longer narrows to just that stream. This makes it impossible to browse through all of the topics in a stream. It also means that if a stream has no topics listed, it's impossible to narrow to that stream.

Another small thing I noticed: muted streams subtopics are not greyed out in color (they're black). It'd be nice if they were the same font color as the stream is (a lighter grey).

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Jul 19, 2016

@niftynei updated..:)

@niftynei

This comment has been minimized.

Contributor

niftynei commented Jul 19, 2016

@kunall17 thanks kunal! Did you have a chance to implement on the top level stream button yet?

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Jul 19, 2016

@niftynei top level stream button, can you elaborate little more please??

@niftynei

This comment has been minimized.

Contributor

niftynei commented Jul 19, 2016

Sure! For example, on our zulip instance this stream doesn't have any topics under it. When I tap on it, it doesn't seem to do anything (in the current client it narrows to the broadcasts stream). Ideally it would both open the subtopics panel & narrow to the stream. :)

screenshot_20160719-190332

@niftynei

This comment has been minimized.

Contributor

niftynei commented Jul 19, 2016

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Jul 19, 2016

Yeah, currently it does nothing, so you want to if there are no subtopics/subjects it should narrow to the stream?

@niftynei

This comment has been minimized.

Contributor

niftynei commented Jul 19, 2016

Yeah, it should still always narrow to stream (even if there are sub-topics to expand).

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Jul 19, 2016

decba2e: narrows on group click, btw we could click on the textView to narrow it separately as well!

@niftynei

This comment has been minimized.

Contributor

niftynei commented Jul 19, 2016

Cool. Looks good to me as is. 👍

@niftynei niftynei merged commit decba2e into zulip:master Jul 19, 2016

@niftynei

This comment has been minimized.

Contributor

niftynei commented Jul 19, 2016

I realize this doesn't include un-read counts -- a separate PR for that is good :)

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Jul 19, 2016

yeah, that will be in another PR!

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