Skip to content
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

Only fetch unread messages when connecting #75

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mhalden
Copy link
Contributor

@mhalden mhalden commented Mar 19, 2016

Currently unread messages will be fetched when connecting and when changing to/from away when show_unread is true. This can lead to duplicate messages being delivered if messages are not automatically marked as read.

With this patch unread messages are only fetched when connecting when show_unread is true.


ic = fb_data_get_connection(fata);
acct = ic->acc;

show_unread = !(ic->flags & OPT_LOGGED_IN);
Copy link
Member

Choose a reason for hiding this comment

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

The name of this variable doesn't really describe it properly. I'm not sure why you need a variable for this either

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree the name isn't the best, but it is needed. icmb_connected sets the flag OPT_LOGGED_IN in ic->flags if it's not already set. So I need to store if this flag was set before icmb_connected is called.

I'll change the name of the variable to something more describing.

When `show_unread` is true, unread messages will only be fetched when
connecting and not when changing to/from away.
@mhalden
Copy link
Contributor Author

mhalden commented Mar 19, 2016

The variable should be better named now.

@jgeboski
Copy link
Member

I think this might be a bad idea. If a user is silently reconnected (the only time this patch is useful), and a message is received while being reconnected, the message is lost. This patch really needs to factor in the current value of the mark_read set. This is also a bug in the current code as well, as it does not factor in the value of mark_read.

@mhalden
Copy link
Contributor Author

mhalden commented Mar 19, 2016

You have a point about reconnects, I didn't think about silent reconnects for other reasons than changing from available to away and back.

Maybe there somewhere I can store a flag to be used when reconnecting because of changing to/from away? It's not perfect, but it's a lot easier than keeping track of the last message received.

I'm not sure how you mean I should take the value of mark_read into account here, the only way I can imagine is to only "mask out" these messages when mark_read is false. I don't know if that's what you meant though.

@jgeboski
Copy link
Member

Maybe there somewhere I can store a flag to be used when reconnecting because of changing to/from away? It's not perfect, but it's a lot easier than keeping track of the last message received.

If you only want this functionality as a result of switching the away state, then I would just wait for the protocol update. Facebook has added a way to switch the visibility state without having to reconnect the MQTT stream. See purple-facebook/issues#227 (patches are cross applied to this plugin).

But to answer the question, you could add a fb_api_is_retrying() function which returns priv->retrying, then factor that into the conditional expression. The value of priv->retrying will be TRUE in the fb_cb_api_connect() function if the call is the result of a silent reconnect.

I'm not sure how you mean I should take the value of mark_read into account here, the only way I can imagine is to only "mask out" these messages when mark_read is false. I don't know if that's what you meant though.

Disregard. I think the best solution here is to just have the set values check the other set values, and ensure that if show_unread is true that mark_read is not false. I mentioned this in #69, but again, outside the scope of both of the PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants