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

Don't register channel subscription / unsubscription / vote messages as 'recent activity' #642

Merged
merged 1 commit into from Oct 10, 2017

Conversation

Projects
None yet
3 participants
@Happy0
Contributor

Happy0 commented Oct 7, 2017

Often I click one of the channels that are 'active' to the left of the patchwork client, only to discover the only activity was a subscription / unsubscription message.

I would prefer if a channel was only considered 'active' when there's been a recent message. I'm not sure how many other people have that opinion.

Is this a change you'd be interested in?

This needs more testing, but I thought I'd start a pull request early to make sure it's something you're interested in having first :).

@Happy0 Happy0 changed the title from Don't register channel subscription / unsubscription messages as 'recent activity' to Don't register channel subscription / unsubscription / vote messages as 'recent activity' Oct 7, 2017

@cryptix

This comment has been minimized.

Show comment
Hide comment
@cryptix

cryptix Oct 9, 2017

Member

Very much in favor of this and the change itself is small also looks sound.

Member

cryptix commented Oct 9, 2017

Very much in favor of this and the change itself is small also looks sound.

@cryptix

This comment has been minimized.

Show comment
Hide comment
@cryptix

cryptix Oct 9, 2017

Member

hmm..

i tried the branch and subscribed to a new channel which then jumped to the top of my feed. I thought it shouldn't do that? will try to debug it later.

Member

cryptix commented Oct 9, 2017

hmm..

i tried the branch and subscribed to a new channel which then jumped to the top of my feed. I thought it shouldn't do that? will try to debug it later.

@Happy0

This comment has been minimized.

Show comment
Hide comment
@Happy0

Happy0 Oct 9, 2017

Contributor

I think when you subscribe to a new channel it might be a different code path from somebody else subscribes to it. It's certainly distinguished in the UI by the green bar at the left.

I'll see what I can find about this later when I get home too :).

I've been able to see it not updating the list by adding print statements about whether it thinks something should update the 'active' timestamp or not (especially at startup when catching up with a bunch of missed messages.)

One thing I'm not 100% sure about is whether you still get channels being bumped on the left when you happen to be using the client as someone votes / subscribes. I don't know if that triggers a different code path or something or whether my eyes were deceiving me. This is also something I need to investigate more.

Contributor

Happy0 commented Oct 9, 2017

I think when you subscribe to a new channel it might be a different code path from somebody else subscribes to it. It's certainly distinguished in the UI by the green bar at the left.

I'll see what I can find about this later when I get home too :).

I've been able to see it not updating the list by adding print statements about whether it thinks something should update the 'active' timestamp or not (especially at startup when catching up with a bunch of missed messages.)

One thing I'm not 100% sure about is whether you still get channels being bumped on the left when you happen to be using the client as someone votes / subscribes. I don't know if that triggers a different code path or something or whether my eyes were deceiving me. This is also something I need to investigate more.

@mmckegg

This comment has been minimized.

Show comment
Hide comment
@mmckegg

mmckegg Oct 9, 2017

Member

This is a good approach @Happy0.

The reason that I did not implement this was I thought it could be a cool way to find out about new channels that you don't yet subscribe to. But now that I think about it, just seeing this in the feed is probably good enough.

I'm going to go ahead and merge this.

Member

mmckegg commented Oct 9, 2017

This is a good approach @Happy0.

The reason that I did not implement this was I thought it could be a cool way to find out about new channels that you don't yet subscribe to. But now that I think about it, just seeing this in the feed is probably good enough.

I'm going to go ahead and merge this.

@Happy0

This comment has been minimized.

Show comment
Hide comment
@Happy0

Happy0 Oct 9, 2017

Contributor

Nice :D

Contributor

Happy0 commented Oct 9, 2017

Nice :D

@mmckegg

This comment has been minimized.

Show comment
Hide comment
@mmckegg

mmckegg Oct 9, 2017

Member

Oh with this approach, you'd also need to modify https://github.com/Happy0/patchwork/blob/abb85fa0947760cd74602bc9f246265ae015e3e1/plugs/channel/obs/recent.js#L42 to filter out isActivityMessage.

I think another method to do this a little more simply would be to just filter out the activity messages all together. They don't need to be in here.

function map (msg) {
  if (msg.value.content && !isActivityMessage(msg)) {
    var channel = normalizeChannel(msg.value.content.channel)
    if (channel) {
      return {
        [channel]: {timestamp: msg.timestamp}
      }
    }
  }
}

I'm going to implement it like this.

Member

mmckegg commented Oct 9, 2017

Oh with this approach, you'd also need to modify https://github.com/Happy0/patchwork/blob/abb85fa0947760cd74602bc9f246265ae015e3e1/plugs/channel/obs/recent.js#L42 to filter out isActivityMessage.

I think another method to do this a little more simply would be to just filter out the activity messages all together. They don't need to be in here.

function map (msg) {
  if (msg.value.content && !isActivityMessage(msg)) {
    var channel = normalizeChannel(msg.value.content.channel)
    if (channel) {
      return {
        [channel]: {timestamp: msg.timestamp}
      }
    }
  }
}

I'm going to implement it like this.

@mmckegg mmckegg merged commit abb85fa into ssbc:master Oct 10, 2017

mmckegg added a commit that referenced this pull request Oct 10, 2017

@Happy0

This comment has been minimized.

Show comment
Hide comment
@Happy0

Happy0 Oct 10, 2017

Contributor

Good point. I actually looked for a 'filter' when I started doing it, but I didn't realise you could just return null from the map and check for it in the reduce function (which I see is actually already the case.)

Anyway, thanks for the merge. It's nice to have s commit in the patchwork git log :D

Contributor

Happy0 commented Oct 10, 2017

Good point. I actually looked for a 'filter' when I started doing it, but I didn't realise you could just return null from the map and check for it in the reduce function (which I see is actually already the case.)

Anyway, thanks for the merge. It's nice to have s commit in the patchwork git log :D

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